Re: [PATCH 05/14] hw/i386/vmport: Report VMX type in CMD_GETVERSION

[Liran Alon]

On 10/03/2020 14:14, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 01:54:02AM +0200, Liran Alon wrote:
> > As can be seen from VmCheck_GetVersion() in open-vm-tools code,
> > CMD_GETVERSION should return VMX type in ECX register.
> >
> > Default is to fake host as VMware ESX server. But user can control
> > this value by "-global vmport.vmx-type=X".
> >
> > Reviewed-by: Nikita Leshenko <address@hidden>
> > Signed-off-by: Liran Alon <address@hidden>
> > ---
> >   hw/i386/vmport.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> > index a2c8ff4b59cf..c03f57f2f636 100644
> > --- a/hw/i386/vmport.c
> > +++ b/hw/i386/vmport.c
> > @@ -36,6 +36,15 @@
> >   #define VMPORT_ENTRIES 0x2c
> >   #define VMPORT_MAGIC   0x564D5868
> >   
> > +typedef enum {
> > +   VMX_TYPE_UNSET = 0,
> > +   VMX_TYPE_EXPRESS,    /* Deprecated type used for VMware Express */
> > +   VMX_TYPE_SCALABLE_SERVER,    /* VMware ESX server */
> > +   VMX_TYPE_WGS,        /* Deprecated type used for VMware Server */
> > +   VMX_TYPE_WORKSTATION,
> > +   VMX_TYPE_WORKSTATION_ENTERPRISE /* Deprecated type used for ACE 1.x */
> > +} VMX_Type;
> > +
> Is this really VMX type? And do users care what it is?
This enum is copied from open-vm-tools source code 
(lib/include/vm_version.h). This is how it's called in VMware Tools 
terminology... Don't blame me :)
> Also, how about friendlier string values so people don't need to
> figure out code numbers?

I could have defined a new PropertyInfo struct in 
hw/core/qdev-properties.c for this enum and then define a proper macro 
in qdev-properties.h.
But it seems like an overkill for a value that is suppose to rarely be 
changed. So I thought this should suffice for now for user-experience 
perspective.
If you think otherwise, I can do what I just suggested above.

-Liran

> >   #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
> >   
> >   typedef struct VMPortState {
> > @@ -46,6 +55,7 @@ typedef struct VMPortState {
> >       void *opaque[VMPORT_ENTRIES];
> >   
> >       uint32_t vmx_version;
> > +    uint8_t vmx_type;
> >   } VMPortState;
> >   
> >   static VMPortState *port_state;
> > @@ -114,6 +124,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;
> > +    cpu->env.regs[R_ECX] = port_state->vmx_type;
> >       return port_state->vmx_version;
> >   }
> >   
> > @@ -173,6 +184,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_UINT8("vmx-type", VMPortState, vmx_type,
> > +                      VMX_TYPE_SCALABLE_SERVER),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> > --
> > 2.20.1