Re: [PATCH v10 02/10] xen: Add support for VMware cpuid leaves

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

On 15/05/15 00:34, Don Slutz wrote:
> This is done by adding xen_arch_domainconfig 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>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v10:
>     Did not add "Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>"
>     because of changes here to do things the new way.
>   Reword comment message to reflect new way.

In which case by above tag doesn't count.

>
> 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/domain.c             |  2 ++
>  xen/arch/x86/hvm/Makefile         |  1 +
>  xen/arch/x86/hvm/hvm.c            | 11 ++++++
>  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/domain.h  |  3 ++
>  xen/include/asm-x86/hvm/hvm.h     |  6 ++++
>  xen/include/asm-x86/hvm/vmware.h  | 33 +++++++++++++++++
>  xen/include/public/arch-x86/xen.h |  2 +-
>  10 files changed, 139 insertions(+), 3 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/domain.c b/xen/arch/x86/domain.c
> index 1f1550e..bc3d3a5 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -518,6 +518,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>          hvm_funcs.hap_supported &&
>          (domcr_flags & DOMCRF_hap);
>      d->arch.hvm_domain.mem_sharing_enabled = 0;
> +    if ( config )
> +        d->arch.hvm_domain.vmware_hwver = config->vmware_hwver;

Urgh - as a result of this I have found a differet bug in this
function.  Please rebase this change over my bugfix patch which I will
post shortly.

>  
>      d->arch.s3_integrity = !!(domcr_flags & DOMCRF_s3_integrity);
>  
> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
> index 69af47f..284ca75 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
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 689e402..05c80e9 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -59,6 +59,7 @@
>  #include <asm/hvm/trace.h>
>  #include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/event.h>
> +#include <asm/hvm/vmware.h>
>  #include <asm/mtrr.h>
>  #include <asm/apic.h>
>  #include <public/sched.h>
> @@ -4253,6 +4254,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;
>  
> @@ -5656,6 +5660,13 @@ static int hvm_allow_set_param(struct domain *d,
>      {
>      /* The following parameters should only be changed once. */
>      case HVM_PARAM_VIRIDIAN:
> +        /* Disallow if vmware_hwver */

"is in use" or "is enabled"

> +        if ( d->arch.hvm_domain.vmware_hwver )
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
> +        /* Fall through */
>      case HVM_PARAM_IOREQ_SERVER_PFN:
>      case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>          if ( value != 0 && a->value != value )
> 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..4839f11
> --- /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.vmware_hwver < 7 )
> +        return 0;
> +
> +    switch ( idx - 0x40000000 )
> +    {
> +    case 0x0:
> +        *eax = 0x40000010;  /* Largest leaf */
> +        *ebx = 0x61774d56;  /* "VMwa" */
> +        *ecx = 0x4d566572;  /* "reVM" */
> +        *edx = 0x65726177;  /* "ware" */
> +        break;

Newline here please.

> +    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;

And here please.

> +    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 4b42b2d..8e1c00a 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/domain.h b/xen/include/asm-x86/hvm/domain.h
> index 0f8b19a..1cb0311 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -109,6 +109,9 @@ struct hvm_domain {
>  
>      uint64_t              *params;
>  
> +    /* emulated VMware Hardware Version */
> +    uint64_t               vmware_hwver;
> +
>      /* Memory ranges with pinned cache attributes. */
>      struct list_head       pinned_cacheattr_ranges;
>  
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 77eeac5..2965fbb 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -356,6 +356,12 @@ 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) \
> +    ((d)->arch.hvm_domain.vmware_hwver)
> +
> +#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/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
> index cea3fe7..5a5bad6 100644
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -263,7 +263,7 @@ struct arch_shared_info {
>  typedef struct arch_shared_info arch_shared_info_t;
>  
>  struct xen_arch_domainconfig {
> -    char dummy;
> +    uint64_t vmware_hwver;

Julien: (as the author of the introducing patch) Given that this
structure is used as part of a domain_create hypercall only, I think a
comment is warrented stating that its API/ABI is covered by the
DOMCTL_INTERFACE_VERSION.

Strictly speaking, this is currently part of the public ABI which must
always maintain backwards compatibility.

Don:  Feel free to retain my Reviewed-by, subject to the adjustments listed.

~Andrew