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

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

>>> On 05.01.17 at 15:28, <andrew.cooper3@citrix.com> 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".

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

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

Right there, yes, as an extension to the line you're already adding.

Jan


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