| Message ID | 20200309235411.76587-5-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : hw/i386/vmport: Bug fixes and improvements | expand |
On Tue, Mar 10, 2020 at 01:54:01AM +0200, Liran Alon wrote: > Instead of hard-coding the VMX version, make it a VMPORT object property. > This would allow user to control it's value via "-global vmport.vmx-version=X". > > Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> More detail on why this is useful? > --- > hw/i386/vmport.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c > index 7c21e56081b0..a2c8ff4b59cf 100644 > --- a/hw/i386/vmport.c > +++ b/hw/i386/vmport.c > @@ -44,6 +44,8 @@ typedef struct VMPortState { > MemoryRegion io; > VMPortReadFunc *func[VMPORT_ENTRIES]; > void *opaque[VMPORT_ENTRIES]; > + > + uint32_t vmx_version; > } VMPortState; > > static VMPortState *port_state; > @@ -112,7 +114,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr) > X86CPU *cpu = X86_CPU(current_cpu); > > cpu->env.regs[R_EBX] = VMPORT_MAGIC; > - return 6; > + return port_state->vmx_version; > } > > static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) > @@ -169,6 +171,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp) > } > > static Property vmport_properties[] = { > + /* Default value taken from open-vm-tools code VERSION_MAGIC definition */ > + DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6), > DEFINE_PROP_END_OF_LIST(), > }; > > -- > 2.20.1
On 10/03/2020 11:32, Michael S. Tsirkin wrote: > On Tue, Mar 10, 2020 at 01:54:01AM +0200, Liran Alon wrote: >> Instead of hard-coding the VMX version, make it a VMPORT object property. >> This would allow user to control it's value via "-global vmport.vmx-version=X". >> >> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> > More detail on why this is useful? It's more useful than returning a hard-coded "6" as the vmx-version... We have used it to preserve compatibility for some VMware guests that we run as-is on top of QEMU/KVM which expects specific vmx-version or else they fail to run properly. -Liran > >> --- >> hw/i386/vmport.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c >> index 7c21e56081b0..a2c8ff4b59cf 100644 >> --- a/hw/i386/vmport.c >> +++ b/hw/i386/vmport.c >> @@ -44,6 +44,8 @@ typedef struct VMPortState { >> MemoryRegion io; >> VMPortReadFunc *func[VMPORT_ENTRIES]; >> void *opaque[VMPORT_ENTRIES]; >> + >> + uint32_t vmx_version; >> } VMPortState; >> >> static VMPortState *port_state; >> @@ -112,7 +114,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr) >> X86CPU *cpu = X86_CPU(current_cpu); >> >> cpu->env.regs[R_EBX] = VMPORT_MAGIC; >> - return 6; >> + return port_state->vmx_version; >> } >> >> static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) >> @@ -169,6 +171,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp) >> } >> >> static Property vmport_properties[] = { >> + /* Default value taken from open-vm-tools code VERSION_MAGIC definition */ >> + DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> -- >> 2.20.1
On Tue, Mar 10, 2020 at 01:05:02PM +0200, Liran Alon wrote: > > On 10/03/2020 11:32, Michael S. Tsirkin wrote: > > On Tue, Mar 10, 2020 at 01:54:01AM +0200, Liran Alon wrote: > > > Instead of hard-coding the VMX version, make it a VMPORT object property. > > > This would allow user to control it's value via "-global vmport.vmx-version=X". > > > > > > Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> > > > Signed-off-by: Liran Alon <liran.alon@oracle.com> > > More detail on why this is useful? > It's more useful than returning a hard-coded "6" as the vmx-version... Maybe default should be 6 (a bit of explanation why 6 could be nice). > We have used it to preserve compatibility for some VMware guests that we run > as-is on top of QEMU/KVM which expects specific vmx-version or else they > fail to run properly. > > -Liran Any detail on which guest it is? Pretending to be a very advanced version has its pitfalls if we then don't behave the way vmware does, right? Figuring out the version number is I suspect a bit much to ask of users. > > > > > --- > > > hw/i386/vmport.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c > > > index 7c21e56081b0..a2c8ff4b59cf 100644 > > > --- a/hw/i386/vmport.c > > > +++ b/hw/i386/vmport.c > > > @@ -44,6 +44,8 @@ typedef struct VMPortState { > > > MemoryRegion io; > > > VMPortReadFunc *func[VMPORT_ENTRIES]; > > > void *opaque[VMPORT_ENTRIES]; > > > + > > > + uint32_t vmx_version; > > > } VMPortState; > > > static VMPortState *port_state; > > > @@ -112,7 +114,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr) > > > X86CPU *cpu = X86_CPU(current_cpu); > > > cpu->env.regs[R_EBX] = VMPORT_MAGIC; > > > - return 6; > > > + return port_state->vmx_version; > > > } > > > static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) > > > @@ -169,6 +171,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp) > > > } > > > static Property vmport_properties[] = { > > > + /* Default value taken from open-vm-tools code VERSION_MAGIC definition */ > > > + DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6), > > > DEFINE_PROP_END_OF_LIST(), > > > }; > > > -- > > > 2.20.1
On 10/03/2020 13:18, Michael S. Tsirkin wrote: > On Tue, Mar 10, 2020 at 01:05:02PM +0200, Liran Alon wrote: >> On 10/03/2020 11:32, Michael S. Tsirkin wrote: >>> On Tue, Mar 10, 2020 at 01:54:01AM +0200, Liran Alon wrote: >>>> Instead of hard-coding the VMX version, make it a VMPORT object property. >>>> This would allow user to control it's value via "-global vmport.vmx-version=X". >>>> >>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> >>>> Signed-off-by: Liran Alon <liran.alon@oracle.com> >>> More detail on why this is useful? >> It's more useful than returning a hard-coded "6" as the vmx-version... > > Maybe default should be 6 (a bit of explanation why 6 could be nice). The default is indeed defined as 6. As it was before this patch. There is not much to explain besides the fact that recent VMware products returns 6 here. I don't recall any mapping between the returned version here and the supported set of VMPort commands. There is a separate mechanism (which we implement in another patch) to signal that a command is unsupported / failed. The term "vmx-version" refers to the version of the Userspace-VMM of VMware which is called (confusingly) "vmx". >> We have used it to preserve compatibility for some VMware guests that we run >> as-is on top of QEMU/KVM which expects specific vmx-version or else they >> fail to run properly. >> >> -Liran > Any detail on which guest it is? I will need to dig in production history to find it... They are usually proprietary appliances specially made to run as VMware VMs. > Pretending to be a very advanced version has its pitfalls if we > then don't behave the way vmware does, right? In all those cases, we have taken the version number backwards, not forward. > Figuring out the version number is I suspect a bit much to ask of users. Most users will indeed not need to touch this. This is for advanced users, such as Ravello. We usually figured this out by reverse-engineering the failed guest and/or examining the original VMware environment it used to run on. -Liran
On Tue, Mar 10, 2020 at 01:28:32PM +0200, Liran Alon wrote: > > On 10/03/2020 13:18, Michael S. Tsirkin wrote: > > On Tue, Mar 10, 2020 at 01:05:02PM +0200, Liran Alon wrote: > > > On 10/03/2020 11:32, Michael S. Tsirkin wrote: > > > > On Tue, Mar 10, 2020 at 01:54:01AM +0200, Liran Alon wrote: > > > > > Instead of hard-coding the VMX version, make it a VMPORT object property. > > > > > This would allow user to control it's value via "-global vmport.vmx-version=X". > > > > > > > > > > Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> > > > > > Signed-off-by: Liran Alon <liran.alon@oracle.com> > > > > More detail on why this is useful? > > > It's more useful than returning a hard-coded "6" as the vmx-version... > > > > Maybe default should be 6 (a bit of explanation why 6 could be nice). > The default is indeed defined as 6. As it was before this patch. > There is not much to explain besides the fact that recent VMware products > returns 6 here. > > I don't recall any mapping between the returned version here and the > supported set of VMPort commands. There is a separate mechanism (which we > implement in another patch) to signal that a command is unsupported / > failed. > > The term "vmx-version" refers to the version of the Userspace-VMM of VMware > which is called (confusingly) "vmx". Short for Virtual Machine eXecutable. Sigh. People do come up with names that aren't great. I don't even know whether vmware was there first and intel shouldn't have shortened virtual machine extensions to vmx, but in KVM and QEMU it's quite entrenched by now. So let's try to avoid this in code. If you like how about VMPortExec and vm-exec-version? Also lets you use CamelCase consistently and not a mix of underscores and CamelCase. > > > We have used it to preserve compatibility for some VMware guests that we run > > > as-is on top of QEMU/KVM which expects specific vmx-version or else they > > > fail to run properly. > > > > > > -Liran > > Any detail on which guest it is? > I will need to dig in production history to find it... They are usually > proprietary appliances specially made to run as VMware VMs. > > Pretending to be a very advanced version has its pitfalls if we > > then don't behave the way vmware does, right? > In all those cases, we have taken the version number backwards, not forward. > > Figuring out the version number is I suspect a bit much to ask of users. > Most users will indeed not need to touch this. This is for advanced users, > such as Ravello. > We usually figured this out by reverse-engineering the failed guest and/or > examining the original VMware environment it used to run on. > > -Liran Right if you want this for debugging, prefix property with "x-" so it does not need to be maintained. Point being, maintaining low level interfaces has real cost ...
On 10/03/2020 13:44, Michael S. Tsirkin wrote: > On Tue, Mar 10, 2020 at 01:28:32PM +0200, Liran Alon wrote: >> On 10/03/2020 13:18, Michael S. Tsirkin wrote: >>> On Tue, Mar 10, 2020 at 01:05:02PM +0200, Liran Alon wrote: >>>> On 10/03/2020 11:32, Michael S. Tsirkin wrote: >>>>> On Tue, Mar 10, 2020 at 01:54:01AM +0200, Liran Alon wrote: >>>>>> Instead of hard-coding the VMX version, make it a VMPORT object property. >>>>>> This would allow user to control it's value via "-global vmport.vmx-version=X". >>>>>> >>>>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> >>>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com> >>>>> More detail on why this is useful? >>>> It's more useful than returning a hard-coded "6" as the vmx-version... >>> Maybe default should be 6 (a bit of explanation why 6 could be nice). >> The default is indeed defined as 6. As it was before this patch. >> There is not much to explain besides the fact that recent VMware products >> returns 6 here. >> >> I don't recall any mapping between the returned version here and the >> supported set of VMPort commands. There is a separate mechanism (which we >> implement in another patch) to signal that a command is unsupported / >> failed. >> >> The term "vmx-version" refers to the version of the Userspace-VMM of VMware >> which is called (confusingly) "vmx". > Short for Virtual Machine eXecutable. Sigh. People do come up with > names that aren't great. I don't even know whether vmware was there > first and intel shouldn't have shortened virtual machine extensions to > vmx, but in KVM and QEMU it's quite entrenched by now. So let's try to > avoid this in code. If you like how about VMPortExec and > vm-exec-version? In my opinion, this is more confusing. Terms should be evaluated based on the context you read them. It seems more confusing that this field is named "vmx-version" on open-vm-tools and we name it "vm-exec-version". So if you don't strongly disagree, I prefer to remain with names similar to VMware. > Also lets you use CamelCase consistently and not a mix > of underscores and CamelCase. Where do you refer to? Looking at QEMU code, properties are always named with "bla-bla2-bla3" convention. Also, variables of structs are also named with "bla_bla2" convention. CamelCase are only used for type definitions. > >>>> We have used it to preserve compatibility for some VMware guests that we run >>>> as-is on top of QEMU/KVM which expects specific vmx-version or else they >>>> fail to run properly. >>>> >>>> -Liran >>> Any detail on which guest it is? >> I will need to dig in production history to find it... They are usually >> proprietary appliances specially made to run as VMware VMs. >>> Pretending to be a very advanced version has its pitfalls if we >>> then don't behave the way vmware does, right? >> In all those cases, we have taken the version number backwards, not forward. >>> Figuring out the version number is I suspect a bit much to ask of users. >> Most users will indeed not need to touch this. This is for advanced users, >> such as Ravello. >> We usually figured this out by reverse-engineering the failed guest and/or >> examining the original VMware environment it used to run on. >> >> -Liran > > Right if you want this for debugging, prefix property with "x-" so it does > not need to be maintained. Point being, maintaining low level interfaces > has real cost ... It's not for debugging. It's for being able to run real VMware VMs as-is on top of QEMU/KVM. I don't see it much different than the existing ability to manipulate SMBIOS values from QEMU command-line or add ACPI tables to make a guest run properly. (Which we also manipulated sometimes based on reverse-engineering the failed guest and/or examining the original VMware environment). I can prefix it with "x-" if you wish to signal it's experimental, but I don't really think this is the case here. -Liran
diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c index 7c21e56081b0..a2c8ff4b59cf 100644 --- a/hw/i386/vmport.c +++ b/hw/i386/vmport.c @@ -44,6 +44,8 @@ typedef struct VMPortState { MemoryRegion io; VMPortReadFunc *func[VMPORT_ENTRIES]; void *opaque[VMPORT_ENTRIES]; + + uint32_t vmx_version; } VMPortState; static VMPortState *port_state; @@ -112,7 +114,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr) X86CPU *cpu = X86_CPU(current_cpu); cpu->env.regs[R_EBX] = VMPORT_MAGIC; - return 6; + return port_state->vmx_version; } static uint32_t vmport_cmd_ram_size(void *opaque, uint32_t addr) @@ -169,6 +171,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp) } static Property vmport_properties[] = { + /* Default value taken from open-vm-tools code VERSION_MAGIC definition */ + DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6), DEFINE_PROP_END_OF_LIST(), };