| Message ID | 20200312165431.82118-16-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : hw/i386/vmport: Bug fixes and improvements | expand |
On 3/12/20 5:54 PM, Liran Alon wrote: > This command returns to guest information on LAPIC bus frequency and TSC > frequency. > > One can see how this interface is used by Linux vmware_platform_setup() > introduced in Linux commit 88b094fb8d4f ("x86: Hypervisor detection and > get tsc_freq from hypervisor"). > > Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > hw/i386/vmport.c | 19 +++++++++++++++++++ > include/hw/i386/vmport.h | 1 + > 2 files changed, 20 insertions(+) > > diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c > index 1664a6b97332..9d3921cf418d 100644 > --- a/hw/i386/vmport.c > +++ b/hw/i386/vmport.c > @@ -176,6 +176,24 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) > return ram_size; > } > > +static uint32_t vmport_cmd_get_hz(void *opaque, uint32_t addr) > +{ > + X86CPU *cpu = X86_CPU(current_cpu); > + > + if (cpu->env.tsc_khz && cpu->env.apic_bus_freq) { > + uint64_t tsc_freq = (uint64_t)cpu->env.tsc_khz * 1000; > + > + cpu->env.regs[R_ECX] = cpu->env.apic_bus_freq; > + cpu->env.regs[R_EBX] = (uint32_t)(tsc_freq >> 32); > + cpu->env.regs[R_EAX] = (uint32_t)tsc_freq; > + } else { > + /* Signal cmd as not supported */ > + cpu->env.regs[R_EBX] = UINT32_MAX; > + } > + > + return cpu->env.regs[R_EAX]; > +} > + > static uint32_t vmport_cmd_time(void *opaque, uint32_t addr) > { > X86CPU *cpu = X86_CPU(current_cpu); > @@ -265,6 +283,7 @@ static void vmport_realizefn(DeviceState *dev, Error **errp) > if (s->compat_flags & VMPORT_COMPAT_CMDS_V2) { > vmport_register(VMPORT_CMD_GETBIOSUUID, vmport_cmd_get_bios_uuid, NULL); > vmport_register(VMPORT_CMD_GETTIME, vmport_cmd_time, NULL); > + vmport_register(VMPORT_CMD_GETHZ, vmport_cmd_get_hz, NULL); > vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL); > vmport_register(VMPORT_CMD_GET_VCPU_INFO, vmport_cmd_get_vcpu_info, > NULL); > diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h > index 34cc050b1ffa..aee809521aa0 100644 > --- a/include/hw/i386/vmport.h > +++ b/include/hw/i386/vmport.h > @@ -12,6 +12,7 @@ typedef enum { > VMPORT_CMD_VMMOUSE_DATA = 39, > VMPORT_CMD_VMMOUSE_STATUS = 40, > VMPORT_CMD_VMMOUSE_COMMAND = 41, > + VMPORT_CMD_GETHZ = 45, Can you rename to something easier to read, such _GET_FREQS_HZ or nicer? > VMPORT_CMD_GETTIMEFULL = 46, > VMPORT_CMD_GET_VCPU_INFO = 68, > VMPORT_ENTRIES >
On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote: > On 3/12/20 5:54 PM, Liran Alon wrote: >> >> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h >> index 34cc050b1ffa..aee809521aa0 100644 >> --- a/include/hw/i386/vmport.h >> +++ b/include/hw/i386/vmport.h >> @@ -12,6 +12,7 @@ typedef enum { >> VMPORT_CMD_VMMOUSE_DATA = 39, >> VMPORT_CMD_VMMOUSE_STATUS = 40, >> VMPORT_CMD_VMMOUSE_COMMAND = 41, >> + VMPORT_CMD_GETHZ = 45, > > Can you rename to something easier to read, such _GET_FREQS_HZ or nicer? > I actually prefer to stick with names similar to open-vm-tools. i.e. Similar to the definitions in lib/include/backdoor_def.h. This helps correlates a command in QEMU code to guest code (in open-vm-tools) that interacts with it. I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested for previous commands). But I don't have a strong opinion on this. If you still think _GET_FREQ_HZ is preferred, I will rename to that. -Liran
On 3/13/20 11:44 PM, Liran Alon wrote: > On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote: >> On 3/12/20 5:54 PM, Liran Alon wrote: >>> >>> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h >>> index 34cc050b1ffa..aee809521aa0 100644 >>> --- a/include/hw/i386/vmport.h >>> +++ b/include/hw/i386/vmport.h >>> @@ -12,6 +12,7 @@ typedef enum { >>> VMPORT_CMD_VMMOUSE_DATA = 39, >>> VMPORT_CMD_VMMOUSE_STATUS = 40, >>> VMPORT_CMD_VMMOUSE_COMMAND = 41, >>> + VMPORT_CMD_GETHZ = 45, >> >> Can you rename to something easier to read, such _GET_FREQS_HZ or nicer? >> > I actually prefer to stick with names similar to open-vm-tools. i.e. > Similar to the definitions in lib/include/backdoor_def.h. > This helps correlates a command in QEMU code to guest code (in > open-vm-tools) that interacts with it. Well, we don't have to follow bad examples ;) > I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested > for previous commands). Thanks, this makes code review more easier. > But I don't have a strong opinion on this. If you still think > _GET_FREQ_HZ is preferred, I will rename to that. > > -Liran >
On Sat, Mar 14, 2020 at 12:44:55AM +0200, Liran Alon wrote: > > On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote: > > On 3/12/20 5:54 PM, Liran Alon wrote: > > > > > > diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h > > > index 34cc050b1ffa..aee809521aa0 100644 > > > --- a/include/hw/i386/vmport.h > > > +++ b/include/hw/i386/vmport.h > > > @@ -12,6 +12,7 @@ typedef enum { > > > VMPORT_CMD_VMMOUSE_DATA = 39, > > > VMPORT_CMD_VMMOUSE_STATUS = 40, > > > VMPORT_CMD_VMMOUSE_COMMAND = 41, > > > + VMPORT_CMD_GETHZ = 45, > > > > Can you rename to something easier to read, such _GET_FREQS_HZ or nicer? > > > I actually prefer to stick with names similar to open-vm-tools. i.e. Similar > to the definitions in lib/include/backdoor_def.h. Please, do not copy without attribution. It really applies everywhere, I commented on another enum and you fixed it there, but please go over your code and try to generally apply the same rules. > This helps correlates a command in QEMU code to guest code (in > open-vm-tools) that interacts with it. > I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested for > previous commands). > But I don't have a strong opinion on this. If you still think _GET_FREQ_HZ > is preferred, I will rename to that. > > -Liran Generally I don't think a hard to read code somewhere is a good reason to have hard to read code in QEMU, especially since it tends to proliferate. It seems unlikely that VMPORT_CMD_GETHZ appears verbatim anywhere, and applying transformation rules is just too tricky. The best way to map host code to guest code in light of coding style differences etc is using comments. You did it in case of the type values, it applies equally here.
On 14/03/2020 23:52, Michael S. Tsirkin wrote: > On Sat, Mar 14, 2020 at 12:44:55AM +0200, Liran Alon wrote: >> On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote: >>> On 3/12/20 5:54 PM, Liran Alon wrote: >>>> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h >>>> index 34cc050b1ffa..aee809521aa0 100644 >>>> --- a/include/hw/i386/vmport.h >>>> +++ b/include/hw/i386/vmport.h >>>> @@ -12,6 +12,7 @@ typedef enum { >>>> VMPORT_CMD_VMMOUSE_DATA = 39, >>>> VMPORT_CMD_VMMOUSE_STATUS = 40, >>>> VMPORT_CMD_VMMOUSE_COMMAND = 41, >>>> + VMPORT_CMD_GETHZ = 45, >>> Can you rename to something easier to read, such _GET_FREQS_HZ or nicer? >>> >> I actually prefer to stick with names similar to open-vm-tools. i.e. Similar >> to the definitions in lib/include/backdoor_def.h. > Please, do not copy without attribution. It really applies everywhere, > I commented on another enum and you fixed it there, but please > go over your code and try to generally apply the same rules. This is not a copy of the enum as the other case you replied on. It's just names "inspired" or "similar" to original names. They are not even the same. > >> This helps correlates a command in QEMU code to guest code (in >> open-vm-tools) that interacts with it. >> I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested for >> previous commands). >> But I don't have a strong opinion on this. If you still think _GET_FREQ_HZ >> is preferred, I will rename to that. >> >> -Liran > > Generally I don't think a hard to read code somewhere is a good reason > to have hard to read code in QEMU, especially since it tends to > proliferate. It seems unlikely that VMPORT_CMD_GETHZ appears verbatim > anywhere, and applying transformation rules is just too tricky. The best > way to map host code to guest code in light of coding style differences > etc is using comments. You did it in case of the type values, it > applies equally here. > Honestly, even though I used slightly different names than original open-vm-tools code, I think it's quite trivial to coorelate. Both by similar name (not same), by value and by function. That's why I don't have a strong opinion about the name. I think VMPORT_CMD_GET_HZ is sufficient, but honestly I would name it however you want. I really don't care. I don't think any special comment is necessary here for correlation. But I don't mind putting above enum a general comment such as: /* See open-vm-tools lib/include/backdoor_def.h to match these to guest commands */ -Liran
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c index 1664a6b97332..9d3921cf418d 100644 --- a/hw/i386/vmport.c +++ b/hw/i386/vmport.c @@ -176,6 +176,24 @@ static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) return ram_size; } +static uint32_t vmport_cmd_get_hz(void *opaque, uint32_t addr) +{ + X86CPU *cpu = X86_CPU(current_cpu); + + if (cpu->env.tsc_khz && cpu->env.apic_bus_freq) { + uint64_t tsc_freq = (uint64_t)cpu->env.tsc_khz * 1000; + + cpu->env.regs[R_ECX] = cpu->env.apic_bus_freq; + cpu->env.regs[R_EBX] = (uint32_t)(tsc_freq >> 32); + cpu->env.regs[R_EAX] = (uint32_t)tsc_freq; + } else { + /* Signal cmd as not supported */ + cpu->env.regs[R_EBX] = UINT32_MAX; + } + + return cpu->env.regs[R_EAX]; +} + static uint32_t vmport_cmd_time(void *opaque, uint32_t addr) { X86CPU *cpu = X86_CPU(current_cpu); @@ -265,6 +283,7 @@ static void vmport_realizefn(DeviceState *dev, Error **errp) if (s->compat_flags & VMPORT_COMPAT_CMDS_V2) { vmport_register(VMPORT_CMD_GETBIOSUUID, vmport_cmd_get_bios_uuid, NULL); vmport_register(VMPORT_CMD_GETTIME, vmport_cmd_time, NULL); + vmport_register(VMPORT_CMD_GETHZ, vmport_cmd_get_hz, NULL); vmport_register(VMPORT_CMD_GETTIMEFULL, vmport_cmd_time_full, NULL); vmport_register(VMPORT_CMD_GET_VCPU_INFO, vmport_cmd_get_vcpu_info, NULL); diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h index 34cc050b1ffa..aee809521aa0 100644 --- a/include/hw/i386/vmport.h +++ b/include/hw/i386/vmport.h @@ -12,6 +12,7 @@ typedef enum { VMPORT_CMD_VMMOUSE_DATA = 39, VMPORT_CMD_VMMOUSE_STATUS = 40, VMPORT_CMD_VMMOUSE_COMMAND = 41, + VMPORT_CMD_GETHZ = 45, VMPORT_CMD_GETTIMEFULL = 46, VMPORT_CMD_GET_VCPU_INFO = 68, VMPORT_ENTRIES
This command returns to guest information on LAPIC bus frequency and TSC frequency. One can see how this interface is used by Linux vmware_platform_setup() introduced in Linux commit 88b094fb8d4f ("x86: Hypervisor detection and get tsc_freq from hypervisor"). Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- hw/i386/vmport.c | 19 +++++++++++++++++++ include/hw/i386/vmport.h | 1 + 2 files changed, 20 insertions(+)