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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

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

>
>> @@ -125,6 +126,15 @@ struct cpuid_policy
>>          };
>>      } basic;
>>  
>> +    /* Structured cache leaf: 0x00000004[xx] */
>> +    union {
>> +        struct cpuid_leaf raw[CPUID_GUEST_NR_CACHE];
>> +        struct {
>> +            uint32_t type:4,
> According to the SDM version I'm looking at this is a 5 bit field.

Right you are.  I'd got confused by the "Bits 04 - 00".  Will fix.

~Andrew

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