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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/01/17 14:52, Jan Beulich wrote:
>>>> On 05.01.17 at 15:28, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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.
> Well, that's why the recommendation to add a BUILD_BUG_ON() -
> that's a reminder to that "whoever".

Ok.

>
>>>> +        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].
> Actually, perhaps a mixture: Inside the default case have
>
>     if ( leaf == 0x7 )
>     {
>         if ( subleaf > p->feat.max_subleaf )
>             return;
>     }
>     else if ( leaf == 0xd)
>     {
>         if ( subleaf > ARRAY_SIZE(p->xstate.raw) )
>             return;
>     }
>     if ( leaf > p->basic.max_leaf )
>         return;
>
> Which (by making the last one if rather than else-if) also fixes an
> issue I've spotted only now: So far you exclude leaves 7 and 0xd
> from the basic.max_leaf checking. (And this way that check could
> also go first.)

Very good point, although I still think I'd still prefer a logic block
in this form inside a case 0 ... 0x3fffffff to avoid potential leakage
if other logic changes.

~Andrew

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