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

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 01/16/15 02:57, Jan Beulich wrote:
>>>> On 15.01.15 at 22:00, <dslutz@xxxxxxxxxxx> wrote:
>> On 01/15/15 11:42, Jan Beulich wrote:
>>>>>> On 02.10.14 at 23:30, <dslutz@xxxxxxxxxxx> wrote:
>>>> --- /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.
> 
> Much will depend on the discussion of the subsequent much bigger
> patch, so nothing specific to say here for the moment.
> 
>>>> +/*
>>>> + * 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).
> 
> But afaict zeros get returned even when you bail from the function
> right at the top, thanks to the subsequent domain_cpuid() invocation
> in the caller (unless overridden in the guest config, which surely
> would be dubious).
> 

I will check this out.

>>>> --- 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 */
> 
> ... emulated ...
> 

Sure.

>> Did you what me to rename HVM_PARAM_VMWARE_HW also to new name (maybe
>> HVM_PARAM_VMWARE_HARDWARE_VERSION)?
> 
> Perhaps a good idea; if it turns out too long,
> HVM_PARAM_VMWARE_HWVER would also seem fine.
> 

Where it is used the lines are already long, so I will go with this.

   -Don Slutz

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel