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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/01/17 14:01, Jan Beulich wrote:
>
>> @@ -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?

Can do.

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

Paths are introduced later in the series.

>
>> +    /* {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.

Yes.  Removed in patch 27.

>
>> +        regs.rax = leaf;
>> +        regs.rcx = subleaf;
> DYM _eax/_ecx respectively? The upper halves are of no interest.

Mainly for my peace of mind.  This, like the above, is only transient.

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

No.  Will fix.

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

Ok.

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

struct cpuid_leaf cpuid_leaf?

I can't think of anything clearer.

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

Will use uint32_t.

~Andrew

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