Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Don Slutz <dslutz@verizon.com>]

On 09/11/14 15:49, Andrew Cooper wrote:
> On 11/09/2014 19:36, Don Slutz wrote:
>> This is done by adding HVM_PARAM_VMWARE_HW. It is set to the VMware
>> virtual hardware version.
>>
>> Currently 0, 3-4, 6-11 are good values.  However the
>> code only checks for == 0 or != 0.
>>
>> If non-zero then
>>    Return VMware's cpuid leaves.
>>
>> The support of hypervisor cpuid leaves has not been agreed to.
>>
>> MicroSoft Hyper-V (AKA viridian) currently must be at 0x40000000.
>>
>> VMware currently must be at 0x40000000.
>>
>> KVM currently must be at 0x40000000 (from Seabios).
>>
>> Seabios will find xen at 0x40000000, 0x40000100, 0x40000200 ..
>> 0x40010000.
>>
>> http://download.microsoft.com/download/F/B/0/FB0D01A3-8E3A-4F5F-AA59-08C8026D3B8A/requirements-for-implementing-microsoft-hypervisor-interface.docx
>>
>> http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
>>
>> http://lwn.net/Articles/301888/
>>    Attempted to get this cleaned up.
>>
>> So based on this, I picked the order:
>>
>> Xen at 0x40000000 or
>> Viridian or VMware at 0x40000000 and Xen at 0x40000100
>>
>> If both Viridian and VMware selected, report an error.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   xen/arch/x86/hvm/Makefile        |  3 +-
>>   xen/arch/x86/hvm/hvm.c           | 32 +++++++++++++++++++
>>   xen/arch/x86/hvm/vmware/Makefile |  1 +
>>   xen/arch/x86/hvm/vmware/cpuid.c  | 69 ++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/x86/traps.c             |  8 +++--
>>   xen/include/asm-x86/hvm/hvm.h    |  3 ++
>>   xen/include/asm-x86/hvm/vmware.h | 31 ++++++++++++++++++
>>   xen/include/public/hvm/params.h  |  5 ++-
>>   8 files changed, 148 insertions(+), 4 deletions(-)
>>   create mode 100644 xen/arch/x86/hvm/vmware/Makefile
>>   create mode 100644 xen/arch/x86/hvm/vmware/cpuid.c
>>   create mode 100644 xen/include/asm-x86/hvm/vmware.h
>>
>> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
>> index eea5555..77598a6 100644
>> --- a/xen/arch/x86/hvm/Makefile
>> +++ b/xen/arch/x86/hvm/Makefile
>> @@ -1,5 +1,6 @@
>>   subdir-y += svm
>>   subdir-y += vmx
>> +subdir-y += vmware
>>   
>>   obj-y += asid.o
>>   obj-y += emulate.o
>> @@ -22,4 +23,4 @@ obj-y += vlapic.o
>>   obj-y += vmsi.o
>>   obj-y += vpic.o
>>   obj-y += vpt.o
>> -obj-y += vpmu.o
>> \ No newline at end of file
>> +obj-y += vpmu.o
> This hunk is unrelated, but is perhaps something better fixed.  A
> passing note in the commit message perhaps?
>

Sure.

[snip -- different thread]


> +#include <xen/sched.h>
> +
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/vmware.h>
> +
> +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:
> Do these leaves have semantic names from a spec, which could live in an
> appropriate header file?

Not sure.

http://lwn.net/Articles/301888/

Which is from Oct 2008 -- Looks like VMware went with this and no one
else.

I can only find list postings that refer the the statement:

"
VMware hardware version 7 defines some of these cpuid levels, below is a 
brief
description about those. These levels can be implemented by other 
hypervisors
too so that Linux has a standard way of communicating to any hypervisor.

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
"

There are a few about 0x40000010 as HYPERVISOR_TIMING_LEAF
or  HYPERVISOR_VMWARE_TIMING_LEAF.

All from late Sep 2008 (https://lkml.org/lkml/2008/9/29/45)

But I also found that I should be checking for >= 7:

The hypervisor present bit and hypervisor information leaf are only 
defined for products based on VMware hardware version 7.

So I will be changing this.



>> +    case 0x10:
>> +        /* (Virtual) TSC frequency in kHz. */
>> +        *eax =  d->arch.tsc_khz;
>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
> At least 1 pair of brackets please, especially as the placement of
> brackets affects the result of this particular calculation.

See other thread.

>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 10fc2ca..f353f42 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -685,8 +685,12 @@ 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;
>> +    /*
>> +     * Optionally shift out of the way of Viridian or VMware
>> +     * architectural leaves.
>> +     */
>> +    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?
>> +        0x40000100 : 0x40000000;
> Again, brackets please for binary operators.  (We have had one recent
> slipup because of the precedence of | and ?:)

I do not think they are needed, but can add them.  A separate
thread also talks about this.


> Furthermore, I think you mean (is_viridian_domain(d) ||
> is_vmware_domain(d)) ? 0x40000100 : 0x40000000; which allows for short
> circuiting of is_vmware_domain().


Boris Ostrovsky siad the same.  I will switch to "||".


>>       uint32_t limit, dummy;
>>   
>>       idx -= base;
>> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
>> index 1123857..546210a 100644
>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -347,6 +347,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>>   #define is_viridian_domain(_d)                                             \
>>    (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
>>   
>> +#define is_vmware_domain(_d)                                             \
>> + (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))
>> +
>>   void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>>                                  uint32_t *eax, uint32_t *ebx,
>>                                  uint32_t *ecx, uint32_t *edx);
>> diff --git a/xen/include/asm-x86/hvm/vmware.h b/xen/include/asm-x86/hvm/vmware.h
>> new file mode 100644
>> index 0000000..f254106
>> --- /dev/null
>> +++ b/xen/include/asm-x86/hvm/vmware.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + * asm-x86/hvm/vmware.h
>> + *
>> + * Copyright (C) 2012 Verizon Corporation
>> + *
>> + * This file is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License Version 2 (GPLv2)
>> + * as published by the Free Software Foundation.
>> + *
>> + * This file is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details. <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef ASM_X86_HVM_VMWARE_H__
>> +#define ASM_X86_HVM_VMWARE_H__
> #include <xen/types.h> (IIRC) please, or uint32_t could cause a
> compilation failure given specific orderings of #include's in
> translation units.

I am fine with adding this.

     -Don Slutz

> ~Andrew
>
>