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

[Don Slutz <dslutz@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > ---
> >   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

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