Re: [Xen-devel] [PATCH 04/10] x86/cpuid: Handle leaf 0x4 in guest_cpuid()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.02.17 at 18:35, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 21/02/17 17:16, Jan Beulich wrote:
>>>>> On 20.02.17 at 12:00, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -163,6 +163,9 @@ static void recalculate_xstate(struct cpuid_policy *p)
>>>   */
>>>  static void recalculate_misc(struct cpuid_policy *p)
>>>  {
>>> +    /* Leaves with subleaf unions. */
>>> +    p->basic.raw[0x4] = p->basic.raw[0x7] = p->basic.raw[0xd] = EMPTY_LEAF;
>> How come you play with leaves 7 and 0xd here?
> 
> This particular piece of clobbering was something which has only just
> occurred to me now when implementing the leaf 4 union.
> 
> Then again, there is no supported way of getting any values into those
> particular rows, or reading out of them, so I could just rely on no-one
> caring?

Well, if they start out as EMPTY_LEAF and there's no way to get
other values into them, why bother filling them here? The more
with a line that doesn't allow neatly extending should one more
such leaf need adding here. I'd say if you want to clobber the
values here just in case, merge the assignments above (in
numeric order) with the ones that are already there just below
(visible in the original patch context).

>>> @@ -244,6 +248,25 @@ static void __init calculate_raw_policy(void)
>>>          cpuid_leaf(i, &p->basic.raw[i]);
>>>      }
>>>  
>>> +    if ( p->basic.max_leaf >= 4 )
>>> +    {
>>> +        for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i )
>>> +        {
>>> +            cpuid_count_leaf(4, i, &p->cache.raw[i]);
>>> +
>>> +            if ( p->cache.subleaf[i].type == 0 )
>>> +                break;
>>> +        }
>>> +
>>> +        /*
>>> +         * The choice of CPUID_GUEST_NR_CACHE is arbitrary.  It is expected
>>> +         * that it will eventually need increasing for future hardware.
>>> +         */
>>> +        if ( i == ARRAY_SIZE(p->cache.raw) )
>>> +            printk(XENLOG_WARNING
>>> +                   "CPUID: Insufficient Leaf 4 space for this hardware\n");
>>> +    }
>> It probably doesn't hurt, but it's one off: There's no enough space
>> only when the next (i-th) doesn't report type 0.
> 
> This bit of logic is slightly awkward.  We read into p->cache.raw[i]
> before looking to see whether p->cache.subleaf[i].type is the end of the
> list.  As such we always read one-past-the-end.

Sure. Issuing the message prematurely could of course be avoided
nevertheless, by reading sub-leaf i (regardless of whether i ==
CPUID_GUEST_NR_CACHE) into a local variable and checking type
there. But as said, it's not something I strictly ask for to be done,
as I can also see upsides of seeing this warning earlier than
absolutely needed.

Jan


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