Re: [RFC PATCH 03/10] Add cpuid_vmware_leaves

[Don Slutz <dslutz@verizon.com>]

On 12/13/13 08:38, Andrew Cooper wrote:
> On 13/12/2013 10:55, Jan Beulich wrote:
>>>>> On 12.12.13 at 23:27, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 12/12/2013 19:15, Don Slutz wrote:
>>>>   int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>>>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>>>>   {
>>>>       struct domain *d = current->domain;
>>>>       /* Optionally shift out of the way of Viridian architectural leaves. */
>>>> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>>>> +    uint32_t base = 0x40000000;
>>>>       uint32_t limit;
>>>>   
>>>> +    if ( is_viridian_domain(d) )
>>>> +        base += 0x100;
>>>> +    if ( is_vmware_domain(d) )
>>>> +        base += 0x100;
>>>> +
As a related question: Should I reply to each e-mail, or is the reply at the thread place (like here) ok?
>>> These bases need a far more scalable solution, especially as the result
>>> of each of these clauses can be changed at runtime with a cunning
>>> hvm_param_set hypercall.
I see.  Did not think about HVM_PARAM_VIRIDIAN changing and the effect it has on all the related code.  I just saw what was there for viridian and tried to add vmware support the same way.  I will look into a patch that will make the bases more scalable, not run time changeable.  I see that right now you can get the code to do strange (and maybe bad) things by changing HVM_PARAM_VIRIDIAN.  So this patch (or patches) would make is_viridian_domain() an unchangable result.  Same for is_vmware_domain() when I add it.
>>> I think that both "is pretending to be HyperV" and "is pretending to be
>>> VMware" need to be domain creation flags which are strictly static for
>>> the lifetime of the domain.
I agree with this.
>> And it seems highly questionable to me whether having both at the
>> same time makes much sense.
>>
>> Plus the new function doesn't belong in xen/arch/x86/traps.c ...
I read this as "Change to some sort of data structure to get the answer instead of a function".  This is because I do not see how to return the correct data from a function if it is not in xen/arch/x86/traps.c (like cpuid_hypervisor_leaves is) or called from there, which I would see as more confusing.
>> Jan
>>
> I would certainly agree on the sentiment of it not making much sense.
>
> However, as Xen needs viridian to run windows, I would be astounded if
> vmware VMs didn't have viridian as well, in which case it is probably
> quite likely that a vmware windows vm with vmware tools would expect to
> find viridian and vmware extensions.
I also find it strange, but could not see a reason not to try and support it.  This could be related to VMware stating that using CPUID is not 100% the way to find out you are running on VMware.

    -Don Slutz
> ~Andrew