From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [RFC PATCH 03/10] Add cpuid_vmware_leaves Date: Tue, 17 Dec 2013 11:20:40 -0500 Message-ID: <52B079D8.7060708@CloudSwitch.Com> References: <1386875718-28166-1-git-send-email-dslutz@terremark.com> <1386875718-28166-4-git-send-email-dslutz@terremark.com> <52AA383B.8000105@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52AA383B.8000105@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 Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , Ian Jackson , Eddie Dong , Don Slutz , xen-devel@lists.xen.org, Jan Beulich , Boris Ostrovsky , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org On 12/12/13 17:27, Andrew Cooper wrote: > On 12/12/2013 19:15, Don Slutz wrote: >> From: Don Slutz >> >> Signed-off-by: Don Slutz >> --- >> xen/arch/x86/hvm/hvm.c | 3 +++ >> xen/arch/x86/traps.c | 58 ++++++++++++++++++++++++++++++++++++++++- >> xen/include/asm-x86/hvm/hvm.h | 3 +++ >> xen/include/asm-x86/processor.h | 2 ++ >> 4 files changed, 65 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index 69f7e74..6a7a781 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -2878,6 +2878,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, count, eax, ebx, ecx, edx) ) >> + return; >> + >> if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) ) >> return; >> >> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c >> index 940bc33..71a76df 100644 >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -671,14 +671,70 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val) >> return 0; >> } >> >> +int cpuid_vmware_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; >> + uint32_t limit; >> + const uint32_t apic_khz = 1000000L; > Please use the proper constant for this. Is this SYSTEM_TIME_HZ / 1000 ? Or is not a constant. All my old testing on amazon showed it to be a constant (but that is an much older xen; most likely did not use virtual apic hardware, but used qemu). I just checked and the hardware I am testing on has: (XEN) calibrating APIC timer ... (XEN) ..... CPU clock speed is 2400.0122 MHz. (XEN) ..... host bus clock speed is 100.0003 MHz. (XEN) ..... bus_scale = 0x6669 So it may be safer to pass on the detected host bus clock speed. >> + >> + if ( !is_vmware_domain(d) ) >> + return 0; >> + >> + idx -= base; >> + >> + limit = 0x10; >> + >> + if ( idx > limit ) >> + return 0; > This `limit` is pointless and the BUG() below is dead code (which > Coverity will complain about). > > Please see 839b966e3f587bbb1a0d954230fb3904330dccb6 and follow its > style, rather than propagating this bad style (which admittedly you have > gotten from following cpuid_hypervisor_leaves() ) Will do. >> + >> + switch ( idx ) >> + { >> + case 0: >> + *eax = base + limit; /* Largest leaf */ >> + *ebx = 0x61774d56; /* "VMwa" */ >> + *ecx = 0x4d566572; /* "reVM" */ >> + *edx = 0x65726177; /* "ware" */ >> + break; >> + >> + case 1 ... 0xf: >> + *eax = 0; /* Reserved */ >> + *ebx = 0; /* Reserved */ >> + *ecx = 0; /* Reserved */ >> + *edx = 0; /* Reserved */ >> + break; >> + >> + case 0x10: >> + /* (Virtual) TSC frequency in kHz. */ >> + *eax = d->arch.tsc_khz; >> + /* (Virtual) Bus (local apic timer) frequency in kHz. */ >> + *ebx = apic_khz; >> + *ecx = 0; /* Reserved */ >> + *edx = 0; /* Reserved */ >> + break; >> + >> + default: >> + BUG(); >> + } >> + >> + return 1; >> +} >> + >> 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; >> + uint32_t base = 0x40000000; >> uint32_t limit; >> >> + if ( is_viridian_domain(d) ) >> + base += 0x100; >> + if ( is_vmware_domain(d) ) >> + base += 0x100; >> + > These bases need a far more scalable solution, especially as the result > of each of these clauses can be changed at runtime with a cunning > hvm_param_set hypercall. Will work on a redesign for these bases. -Don Slutz > I think that both "is pretending to be HyperV" and "is pretending to be > VMware" need to be domain creation flags which are strictly static for > the lifetime of the domain. > > ~Andrew > >> idx -= base; >> >> /* >> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h >> index ccca5df..ae3768c 100644 >> --- a/xen/include/asm-x86/hvm/hvm.h >> +++ b/xen/include/asm-x86/hvm/hvm.h >> @@ -332,6 +332,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_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, >> unsigned int *ecx, unsigned int *edx); >> void hvm_migrate_timers(struct vcpu *v); >> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h >> index c120460..6c53e45 100644 >> --- a/xen/include/asm-x86/processor.h >> +++ b/xen/include/asm-x86/processor.h >> @@ -559,6 +559,8 @@ void int80_direct_trap(void); >> >> extern int hypercall(void); >> >> +int cpuid_vmware_leaves( uint32_t idx, uint32_t sub_idx, >> + uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); >> int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx, >> uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); >> int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val);