Re: [PATCH v9 02/13] xen: Add support for VMware cpuid leaves

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/02/15 23:05, 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 or >= 7.
>
> If non-zero then
>   Return VMware's cpuid leaves.  If >= 7 return data, else
>   return 0.
>
> 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@verizon.com>
> ---
> v9:
>     s/vmware_hw/vmware_hwver/i
>     Change -EXDEV to EOPNOTSUPP.
>       Done.
>     adding another subdirectory: xen/arch/x86/hvm/vmware
>     Much will depend on the discussion of the subsequent patches.
>       TBD.
>     So for versions < 7 there's effectively no CPUID support at all?
>       Changed to check at entry.
>     The comment /* Params for VMware */ seems wrong...
>       Changed to /* emulated VMware Hardware Version */
>     Also please use d, not _d in #define is_vmware_domain()
>       Changed.  Line is now > 80 characters, so split into 2.
>
> v7:
>       Prevent setting of HVM_PARAM_VIRIDIAN if HVM_PARAM_VMWARE_HW set.
> 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  | 75 ++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/traps.c             |  8 +++--
>  xen/include/asm-x86/hvm/hvm.h    |  7 ++++
>  xen/include/asm-x86/hvm/vmware.h | 33 ++++++++++++++++++
>  xen/include/public/hvm/params.h  |  5 ++-
>  8 files changed, 160 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 a52c6e0..fefd023 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -60,6 +60,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>
> @@ -4277,6 +4278,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;
>  
> @@ -5752,6 +5756,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_HWVER] )
> +                {
> +                    rc = -EOPNOTSUPP;
> +                    break;
> +                }
>                  if ( a.value != d->arch.hvm_domain.params[a.index] )
>                  {
>                      rc = -EEXIST;
> @@ -5900,6 +5909,29 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>                  break;
>              }
> +            case HVM_PARAM_VMWARE_HWVER:
> +                /*
> +                 * This should only ever be set non-zero one time by
> +                 * the tools and is read only by the guest.
> +                 */
> +                if ( curr_d == d )
> +                {
> +                    rc = -EPERM;
> +                    break;
> +                }
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN] )
> +                {
> +                    rc = -EXDEV;
> +                    break;
> +                }
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HWVER] &&
> +                     d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HWVER] !=
> +                     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..70cdabd
> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/cpuid.c
> @@ -0,0 +1,75 @@
> +/*
> + * 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) ||
> +         d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HWVER] < 7 )
> +        return 0;
> +
> +    switch ( idx - 0x40000000 )
> +    {
> +    case 0x0:
> +        *eax = 0x40000010;  /* Largest leaf */
> +        *ebx = 0x61774d56;  /* "VMwa" */
> +        *ecx = 0x4d566572;  /* "reVM" */
> +        *edx = 0x65726177;  /* "ware" */
> +        break;
> +    case 0x10:
> +        /* (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;
> +    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 f5516dc..589e458 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -750,8 +750,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 0dc909b..8ecde89 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -356,6 +356,13 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>  #define has_viridian_time_ref_count(d) \
>      (is_viridian_domain(d) && (viridian_feature_mask(d) & HVMPV_time_ref_count))
>  
> +#define vmware_feature_mask(d) \
> +    (has_hvm_params(d) ? \
> +     (d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HWVER] : 0)

I can't spot any use of this vmware_feature_mask().  Is it stale?

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +
> +#define is_vmware_domain(d) \
> +    (is_hvm_domain(d) && vmware_feature_mask(d))
> +
>  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 a2d43bc..974d3a4 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -194,6 +194,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
> +/* emulated VMware Hardware Version */
> +#define HVM_PARAM_VMWARE_HWVER 35
> +
> +#define HVM_NR_PARAMS          36
>  
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */