Re: [Xen-devel] [PATCH 22/27] x86/cpuid: Perform max_leaf calculations in guest_cpuid()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/01/17 13:51, Jan Beulich wrote:
>
>> @@ -333,21 +340,50 @@ void guest_cpuid(const struct vcpu *v, unsigned int 
>> leaf,
>>                   unsigned int subleaf, struct cpuid_leaf *res)
>>  {
>>      const struct domain *d = v->domain;
>> +    const struct cpuid_policy *p = d->arch.cpuid;
>>  
>>      *res = EMPTY_LEAF;
>>  
>>      /*
>>       * First pass:
>>       * - Dispatch the virtualised leaves to their respective handlers.
>> +     * - Perform max_leaf/subleaf calculations, maybe returning early.
>>       */
>>      switch ( leaf )
>>      {
>> +    case 0x0 ... 0x6:
>> +    case 0x8 ... 0xc:
>> +#if 0 /* For when CPUID_GUEST_NR_BASIC isn't 0xd */
>> +    case 0xe ... CPUID_GUEST_NR_BASIC - 1:
>> +#endif
> Perhaps have a BUILD_BUG_ON() in an #else here?

The presence of this was to be a reminder to whomever tries upping
max_leaf beyond 0xd.  Then again, there is a reasonable chance it will
be me.

I am half tempted to leave it out.

>
>> +        if ( leaf > p->basic.max_leaf )
>> +            return;
>> +        break;
>> +
>> +    case 0x7:
>> +        if ( subleaf > p->feat.max_subleaf )
>> +            return;
>> +        break;
>> +
>> +    case 0xd:
> XSTATE_CPUID again,

I considered this, but having a mix of named an numbered leaves is worse
than having them uniformly numbered, especially when visually checking
the conditions around the #if 0 case above.

I had considered making a cpuid-index.h for leaf names, but most leaves
are more commonly referred to by number than name, so I am really not
sure if that would be helpful or hindering in the long run.

> which raises the question whether switch() really is the best way to deal 
> with things here.

What else would you suggest?  One way or another (better shown in the
context of the following patch), we need one block per union{} to apply
max_leaf calculations and read the base data from p->$FOO.raw[$IDX].

>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3305,27 +3305,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
>> unsigned int *ebx,
>>      if ( !edx )
>>          edx = &dummy;
>>  
>> -    if ( input & 0x7fffffff )
>> -    {
>> -        /*
>> -         * Requests outside the supported leaf ranges return zero on AMD
>> -         * and the highest basic leaf output on Intel. Uniformly follow
>> -         * the AMD model as the more sane one.
>> -         */
> I think this comment would better be moved instead of deleted.

Where would you like it?  It doesn't have an easy logical place to live
in guest_cpuid().  The best I can think of is probably as an extension
of the "First Pass" comment.

~Andrew

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