Re: [PATCH 02/27] x86/cpuid: Introduce guest_cpuid() and struct cpuid_leaf

["Jan Beulich" <JBeulich@suse.com>]

>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote:
> Jan: I note that this patch texturally conflicts with your register renaming
> series.

And, not just texturally, the SSE/AVX moves series still awaiting your
review.

> @@ -17,6 +18,8 @@ uint32_t __read_mostly raw_featureset[FSCAPINTS];
>  uint32_t __read_mostly pv_featureset[FSCAPINTS];
>  uint32_t __read_mostly hvm_featureset[FSCAPINTS];
>  
> +#define EMPTY_LEAF (struct cpuid_leaf){}

Perhaps another pair of parentheses around the entire thing?

> @@ -215,6 +218,36 @@ const uint32_t * __init lookup_deep_deps(uint32_t feature)
>      return NULL;
>  }
>  
> +void guest_cpuid(const struct vcpu *v, unsigned int leaf,
> +                 unsigned int subleaf, struct cpuid_leaf *res)
> +{
> +    *res = EMPTY_LEAF;

Why? There's no valid path leaving the structure uninitialized.

> +    /* {pv,hvm}_cpuid() have this expectation. */
> +    ASSERT(v == current);
> +
> +    if ( is_pv_vcpu(v) || is_pvh_vcpu(v) )
> +    {
> +        struct cpu_user_regs regs = *guest_cpu_user_regs();

I assume this is only a transient thing, in which case I'm fine with
this relatively big item getting placed on the stack.

> +        regs.rax = leaf;
> +        regs.rcx = subleaf;

DYM _eax/_ecx respectively? The upper halves are of no interest.

> @@ -3246,10 +3252,10 @@ static int priv_op_wbinvd(struct x86_emulate_ctxt *ctxt)
>      return X86EMUL_OKAY;
>  }
>  
> -int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
> -                  unsigned int *edx, struct x86_emulate_ctxt *ctxt)
> +int pv_emul_cpuid(unsigned int leaf, unsigned int subleaf,
> +                  struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt)
>  {
> -    struct cpu_user_regs regs = *ctxt->regs;
> +    struct vcpu *curr = current;
>  
>      /*
>       * x86_emulate uses this function to query CPU features for its own
> @@ -3258,7 +3264,6 @@ int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>       */
>      if ( ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2) )
>      {
> -        const struct vcpu *curr = current;

There was a "const" here - did you really mean to get rid of it?

> @@ -3266,16 +3271,7 @@ int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>              return X86EMUL_EXCEPTION;
>      }
>  
> -    regs._eax = *eax;
> -    regs._ecx = *ecx;
> -
> -    pv_cpuid(&regs);
> -
> -    *eax = regs._eax;
> -    *ebx = regs._ebx;
> -    *ecx = regs._ecx;
> -    *edx = regs._edx;
> -
> +    guest_cpuid(curr, leaf, subleaf, res);
>      return X86EMUL_OKAY;
>  }

Please retain the blank line before the return statement.

> @@ -4502,15 +4502,15 @@ x86_emulate(
>  
>          case 0xfc: /* clzero */
>          {
> -            unsigned int eax = 1, ebx = 0, dummy = 0;
> +            struct cpuid_leaf res;

Please put a single instance of this at the top of the body of the giant
switch() statement (likely calling for it to be named other than "res").

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -164,6 +164,11 @@ enum x86_emulate_fpu_type {
>      X86EMUL_FPU_ymm  /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
>  };
>  
> +struct cpuid_leaf
> +{
> +    uint32_t a, b, c, d;

Could you please consistently use uint32_t or unsigned int between
here and ...

> @@ -415,10 +420,9 @@ struct x86_emulate_ops
>       * #GP[0].  Used to implement CPUID faulting.
>       */
>      int (*cpuid)(
> -        unsigned int *eax,
> -        unsigned int *ebx,
> -        unsigned int *ecx,
> -        unsigned int *edx,
> +        unsigned int leaf,
> +        unsigned int subleaf,
> +        struct cpuid_leaf *res,

... here? I have no particular preference which of the two to use.

> @@ -64,6 +65,9 @@ extern struct cpuidmasks cpuidmask_defaults;
>  /* Whether or not cpuid faulting is available for the current domain. */
>  DECLARE_PER_CPU(bool, cpuid_faulting_enabled);
>  
> +void guest_cpuid(const struct vcpu *v, unsigned int leaf,
> +                 unsigned int subleaf, struct cpuid_leaf *res);

Same for this one then, obviously (and a few others).

Jan

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