Re: [PATCH for-4.5 v7 1/7] xen: Add support for VMware cpuid leaves

[Don Slutz <dslutz@verizon.com>]

On 01/15/15 11:42, Jan Beulich wrote:
>>>> On 02.10.14 at 23:30, <dslutz@verizon.com> wrote:
>> @@ -5536,6 +5540,11 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>                  if ( curr_d == d )
>>                      break;
>>  
>> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] )
>> +                {
>> +                    rc = -EXDEV;
> 
> That's a pretty strange error code here. -EOPNOTSUPP perhaps?
> 

Sure.

>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmware/cpuid.c
> 
> Whether adding another subdirectory here is really the way to go
> heavily depends on how much of this new code we really want to
> take into the tree. There sheer size of the series makes me
> hesitant to consider taking it all.
> 

Not sure what I can say here to help.

>> +/*
>> + * VMware hardware version 7 defines some of these cpuid levels,
>> + * below is a brief description about those.
>> + *
>> + *     Leaf 0x40000000, Hypervisor CPUID information
>> + * # EAX: The maximum input value for hypervisor CPUID info (0x40000010).
>> + * # EBX, ECX, EDX: Hypervisor vendor ID signature. E.g. "VMwareVMware"
>> + *
>> + *     Leaf 0x40000010, Timing information.
>> + * # EAX: (Virtual) TSC frequency in kHz.
>> + * # EBX: (Virtual) Bus (local apic timer) frequency in kHz.
>> + * # ECX, EDX: RESERVED
>> + */
>> +
>> +int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
>> +                        uint32_t *ecx, uint32_t *edx)
>> +{
>> +    struct domain *d = current->domain;
>> +
>> +    if ( !is_vmware_domain(d) )
>> +        return 0;
>> +
>> +    switch ( idx - 0x40000000 )
>> +    {
>> +    case 0x0:
>> +        if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] >= 7 )
>> +        {
>> +            *eax = 0x40000010;  /* Largest leaf */
>> +            *ebx = 0x61774d56;  /* "VMwa" */
>> +            *ecx = 0x4d566572;  /* "reVM" */
>> +            *edx = 0x65726177;  /* "ware" */
>> +            break;
>> +        }
>> +        /* fallthrough */
>> +    case 0x10:
>> +        if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] >= 7 )
>> +        {
>> +            /* (Virtual) TSC frequency in kHz. */
>> +            *eax =  d->arch.tsc_khz;
>> +            /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> +            *ebx = 1000000ull / APIC_BUS_CYCLE_NS;
>> +            *ecx = 0;          /* Reserved */
>> +            *edx = 0;          /* Reserved */
>> +            break;
>> +        }
>> +        /* fallthrough */
> 
> So for versions < 7 there's effectively no CPUID support at all?
> Wouldn't it then make more sense to check for the version together
> with the is_vmware_domain() check at the top?
> 

Nope, when version is > 0 & < 7, all zeros are returned for these.
This is why there is a fallthrough comment.

Doing the zero returns is part of making the environment look as much
like VMware as possible.  I feel this is better, but I can be pushed
into including this check at the top (since the VMware KB article
referenced just has an equal statement).

This range of zeros is what I see during testing on a VMware ESX server.

>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -346,6 +346,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>>  #define is_viridian_domain(d) \
>>      (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq))
>>  
>> +#define is_vmware_domain(_d)                                             \
>> + (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))
> 
> Indentation. Also please use d, not _d. I.e. take the macro above
> as reference.
> 

I did, it has changed during the time I started this patch set.  Will
fix.

>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -189,6 +189,9 @@
>>  /* Location of the VM Generation ID in guest physical address space. */
>>  #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
>>  
>> -#define HVM_NR_PARAMS          35
>> +/* Params for VMware */
>> +#define HVM_PARAM_VMWARE_HW                 35
> 
> The comment seems wrong - after all it's the version, not some
> arbitrary parameters/flags.
> 

It use to be a set of Params.  Will fix, thinking of:

/* VMware Hardware Version */

Did you what me to rename HVM_PARAM_VMWARE_HW also to new name (maybe
HVM_PARAM_VMWARE_HARDWARE_VERSION)?

   -Don Slutz

> Jan
>