Re: [Xen-devel] [PATCH for-4.5 v6 01/16] xen: Add support for VMware cpuid leaves

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/09/14 19:07, 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).
>
> Xen can be found at the first otherwise unused 0x100 aligned
> offset between 0x40000000 and 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.
>
> Since I need to change xen/arch/x86/hvm/Makefile; also add
> a newline at end of file.
>
> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> ---
> v5:
>       Given how is_viridian and is_vmware are defined I think '||' is more
>       appropriate.
>         Fixed.
>       The names of all three functions are bogus.
>         removed static support routines.
>       This hunk is unrelated, but is perhaps something better fixed.
>         Added to commit message.
>       include <xen/types.h> (IIRC) please.
>         Done.
>       At least 1 pair of brackets please, especially as the placement of
>       brackets affects the result of this particular calculation.
>         Switch to "1000000ull / APIC_BUS_CYCLE_NS"      
>
>  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  | 89 
> ++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/traps.c             |  8 +++-
>  xen/include/asm-x86/hvm/hvm.h    |  3 ++
>  xen/include/asm-x86/hvm/vmware.h | 33 +++++++++++++++
>  xen/include/public/hvm/params.h  |  5 ++-
>  8 files changed, 170 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
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index bb45593..f3cf566 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -57,6 +57,7 @@
>  #include <asm/hvm/cacheattr.h>
>  #include <asm/hvm/trace.h>
>  #include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/vmware.h>
>  #include <asm/mtrr.h>
>  #include <asm/apic.h>
>  #include <public/sched.h>
> @@ -4228,6 +4229,9 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
> unsigned int *ebx,
>      if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) )
>          return;
>  
> +    if ( cpuid_vmware_leaves(input, eax, ebx, ecx, edx) )
> +        return;
> +
>      if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) )
>          return;
>  
> @@ -5555,6 +5559,11 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>                      rc = -EINVAL;
>                  break;
>              case HVM_PARAM_VIRIDIAN:
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] )
> +                {
> +                    rc = -EXDEV;
> +                    break;
> +                }
>                  if ( a.value > 1 )
>                      rc = -EINVAL;
>                  break;
> @@ -5692,6 +5701,29 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>                  break;
>              }
> +            case HVM_PARAM_VMWARE_HW:
> +                /*
> +                 * This should only ever be set non-zero one time by
> +                 * the tools and is read only by the guest.
> +                 */
> +                if ( d == current->domain )

curr_d instead of current->domain

> +                {
> +                    rc = -EPERM;
> +                    break;
> +                }
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN] )
> +                {
> +                    rc = -EXDEV;
> +                    break;
> +                }
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] &&

This check is redundant.  Repeatedly setting 0 is still permitted
because of the break after this if().

> +                     d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] !=
> +                     a.value )
> +                {
> +                    rc = -EEXIST;
> +                    break;
> +                }
> +                break;
>              }
>  
>              if ( rc == 0 ) 
> diff --git a/xen/arch/x86/hvm/vmware/Makefile 
> b/xen/arch/x86/hvm/vmware/Makefile
> new file mode 100644
> index 0000000..3fb2e0b
> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/Makefile
> @@ -0,0 +1 @@
> +obj-y += cpuid.o
> diff --git a/xen/arch/x86/hvm/vmware/cpuid.c b/xen/arch/x86/hvm/vmware/cpuid.c
> new file mode 100644
> index 0000000..29f6213
> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/cpuid.c
> @@ -0,0 +1,89 @@
> +/*
> + * arch/x86/hvm/vmware/cpuid.c
> + *
> + * 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/>.
> + */
> +
> +#include <xen/sched.h>
> +
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/vmware.h>
> +
> +/*
> + * 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 )

You are going to need some reference about VMWare versions.  The comment
for this function is fine for describing what "version 7" constitutes,
but how did you come about it?  What is supposed to be visible in the
leaves of a version less than 7 is selected by the toolstack, because I
doubt its all zeroes including the root leaf?

~Andrew

> +        {
> +            *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 */
> +    case 0x1 ... 0xf:
> +        *eax = 0;          /* Reserved */
> +        *ebx = 0;          /* Reserved */
> +        *ecx = 0;          /* Reserved */
> +        *edx = 0;          /* Reserved */
> +        break;
> +
> +    default:
> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 10fc2ca..90542f9 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;
>      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 121d053..3916fec 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -349,6 +349,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..8390173
> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/vmware.h
> @@ -0,0 +1,33 @@
> +/*
> + * 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>
> +
> +int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
> +                        uint32_t *ecx, uint32_t *edx);
> +
> +#endif /* ASM_X86_HVM_VMWARE_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 614ff5f..dee6d68 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -151,6 +151,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
> +
> +#define HVM_NR_PARAMS          36
>  
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */



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