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

[Ian Campbell <Ian.Campbell@citrix.com>]

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

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?

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?

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

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