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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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? (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(&regs[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(&regs[0], polregs[0], 0, 7,
> > +                                        config_transformed[0]);
> > +                for (i = 1; i < 4; i++)
> > +                    if ( regs[i] != polregs[i] )
> > +                        xc_cpuid_revert(&regs[i], polregs[i], 0, 31,
> > +                                        config_transformed[i]);
> > +                break;
> > +
> > +            default:
> > +                for (i = 0; i < 4; i++)
> > +                    xc_cpuid_revert(&regs[i], polregs[i], 0, 31,
> > +                                    config_transformed[i]);
> > +                break;
> > +        }
> > +    }
> > +}
> > +
> >   /*
> >    * Configure a single input with the informatiom from config.
> >    *

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