From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves Date: Fri, 12 Sep 2014 17:26:50 -0400 Message-ID: <5413651A.5010005@terremark.com> References: <1410460610-14759-1-git-send-email-dslutz@verizon.com> <1410460610-14759-2-git-send-email-dslutz@verizon.com> <5411FCCD.2000707@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5411FCCD.2000707@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Don Slutz , xen-devel@lists.xen.org Cc: Kevin Tian , Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , Eddie Dong , Ian Jackson , Tim Deegan , George Dunlap , Aravind Gopalakrishnan , Jan Beulich , Boris Ostrovsky , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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 > + > +#include > +#include > + > +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. . >> + */ >> + >> +#ifndef ASM_X86_HVM_VMWARE_H__ >> +#define ASM_X86_HVM_VMWARE_H__ > #include (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 > >