Re: [Xen-devel] [PATCH v5 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-03-19 at 10:41 -0400, Boris Ostrovsky wrote:
> 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 <boris.ostrovsky@xxxxxxxxxx>
> >> ---
> >>   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?

I think splitting would be clearer, just by virtue of being able to give
the functions comprehensible names.

>  (And I assume you meant 
> make_hv_cpuid, not make_pv_cpuid.)

I meant pv -- XEN_EMUALTE_PREFIX+cpuid instr is a "pv cpuid", isn't it?
It's not clear what "hypervisor cpuid" would be -- is it the cpuid which
the hypervisor sees (i.e. real) or is it some fabrication, in which case
how does it depend on the context (i.e. with which guest is it with
respect too).

> > 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.

Except when you've change them for a guest using the functionality you
are adding here, surely?

> > 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.

Hrm, it may be sufficient, but is it a good interface?

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

OK good, although if they reappear in hypervisor context I suppose my
comments will still stand.

Ian.



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