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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH 04/10] x86/cpuid: Handle leaf 0x4 in guest_cpuid()
Date: Wed, 22 Feb 2017 07:55:33 +0000	[thread overview]
Message-ID: <81a76bb8-0c47-c58d-d770-3ebea32024cd@citrix.com> (raw)
In-Reply-To: <58AD4A83020000780013C91E@prv-mh.provo.novell.com>

On 22/02/17 07:23, Jan Beulich wrote:
>>>> On 21.02.17 at 18:35, <andrew.cooper3@citrix.com> wrote:
>> On 21/02/17 17:16, Jan Beulich wrote:
>>>>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> 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).

I will just drop the clobbering.  Even this bit of logic isn't going to
survive to the end of eventual toolstack changes.

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

Ok.  I will leave it as-is.

~Andrew

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