From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v5 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file Date: Wed, 19 Mar 2014 10:41:08 -0400 Message-ID: <5329AC84.5050102@oracle.com> References: <1395190714-3802-1-git-send-email-boris.ostrovsky@oracle.com> <1395190714-3802-2-git-send-email-boris.ostrovsky@oracle.com> <1395221220.10203.8.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1395221220.10203.8.camel@kazak.uk.xensource.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: Ian Campbell Cc: keir@xen.org, jbeulich@suse.com, stefano.stabellini@eu.citrix.com, eddie.dong@intel.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jun.nakajima@intel.com, andrew.cooper3@citrix.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 03/19/2014 05:27 AM, Ian Campbell wrote: > On Tue, 2014-03-18 at 20:58 -0400, Boris Ostrovsky wrote: >> Currently only "real" cpuid leaves can be overwritten by users via >> 'cpuid' option in the configuration file. This patch provides ability to >> do the same for hypervisor leaves (but for now only 0x40000000 is allowed). >> >> Signed-off-by: Boris Ostrovsky >> --- >> tools/libxc/xc_cpuid_x86.c | 71 ++++++++++++++++++++++++++++++++++++++++-- >> xen/arch/x86/domain.c | 19 +++++++++-- >> xen/arch/x86/traps.c | 3 ++ >> xen/include/asm-x86/domain.h | 7 +++++ >> 4 files changed, 95 insertions(+), 5 deletions(-) >> >> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c >> index bbbf9b8..5501d5b 100644 >> --- a/tools/libxc/xc_cpuid_x86.c >> +++ b/tools/libxc/xc_cpuid_x86.c >> @@ -33,6 +33,8 @@ >> #define DEF_MAX_INTELEXT 0x80000008u >> #define DEF_MAX_AMDEXT 0x8000001cu >> >> +#define IS_HYPERVISOR_LEAF(idx) (((idx) & 0xffff0000) == 0x40000000) > Not idx == 0x40000000? > > Also as I think Jan said before if viridian support is enabled then the > Xen leaves may be elsewhere (at 0x100 increments above that address > IIRC). > >> + >> static int hypervisor_is_64bit(xc_interface *xch) >> { >> xen_capabilities_info_t xen_caps = ""; >> @@ -43,22 +45,31 @@ static int hypervisor_is_64bit(xc_interface *xch) >> static void cpuid(const unsigned int *input, unsigned int *regs) >> { >> unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1]; >> + uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]); >> #ifdef __i386__ >> /* Use the stack to avoid reg constraint failures with some gcc flags */ >> asm ( >> "push %%ebx; push %%edx\n\t" >> + "testb $0xff,%5\n\t" >> + "jz .Lcpuid%=\n\t" >> + XEN_EMULATE_PREFIX >> + ".Lcpuid%=:\n\t" >> "cpuid\n\t" >> "mov %%ebx,4(%4)\n\t" >> "mov %%edx,12(%4)\n\t" >> "pop %%edx; pop %%ebx\n\t" >> : "=a" (regs[0]), "=c" (regs[2]) >> - : "0" (input[0]), "1" (count), "S" (regs) >> + : "0" (input[0]), "1" (count), "S" (regs), "q" (is_hyp) > I think this would be clearer refactored into make_real_cpuid() and > make_pv_cpuid() functions. Would a comment explaining why we do it this way be sufficient or do you really want to split this into two routines? (And I assume you meant make_hv_cpuid, not make_pv_cpuid.) > > It also seems strange to use emulated for a subset of leafs, although I > understand why. > > How does this play out in e.g. a PVH toolstack domain where the even > "real" cpuid might be faked? It shouldn't matter what the guest it, the hypervisor leaves are guest-independent. > > Perhaps we should have a hypercall to retrieve the complete set of real > h/w, levelled h/w, pv, emulated etc values for a given leaf? That's what I was thinking about (except for leveled values) when I implemented sysctl in the early version of this series but then Andrew pointed out that for what I need prefixed cpuid was sufficient, so I went that route. > >> @@ -726,6 +740,57 @@ int xc_cpuid_check( >> return rc; >> } >> >> +static void xc_cpuid_revert(unsigned int *reg, unsigned int polreg, >> + unsigned int start, unsigned int end, char *config) > [...] >> +static void xc_cpuid_constrain(const unsigned int *input, unsigned int *regs, >> + unsigned int *polregs, char **config_transformed) > These complicated functions most certainly need some sort of explanation > about what they are trying to do. They will be gone since the functionality will be implemented in the hypervisor. -boris > > Why do the hypervisor leaves need to be manipulated by new functions, do > the existing facilities used for all the others not work? > >> +{ >> + unsigned int i; >> + >> + if ( IS_HYPERVISOR_LEAF(input[0]) ) >> + { >> + switch ( input[0] & 0xff ) >> + { >> + case 0: >> + if ( (regs[0] & 0xffffff00) != (polregs[0] & 0xffffff00) ) >> + xc_cpuid_revert(®s[0], polregs[0], 8, 31, >> + config_transformed[0]); >> + if ( (regs[0] & 0xff) < 2 ) > Lots of magic numbers with varying number's of f's. Please #define them. > >> + xc_cpuid_revert(®s[0], polregs[0], 0, 7, >> + config_transformed[0]); >> + for (i = 1; i < 4; i++) >> + if ( regs[i] != polregs[i] ) >> + xc_cpuid_revert(®s[i], polregs[i], 0, 31, >> + config_transformed[i]); >> + break; >> + >> + default: >> + for (i = 0; i < 4; i++) >> + xc_cpuid_revert(®s[i], polregs[i], 0, 31, >> + config_transformed[i]); >> + break; >> + } >> + } >> +} >> + >> /* >> * Configure a single input with the informatiom from config. >> *