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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/03/17 12:03, Jan Beulich wrote:
>>>> On 10.03.17 at 17:27, <andrew.cooper3@xxxxxxxxxx> wrote:
>> Leaf 0x4 is reserved by AMD.  For Intel, it is a multi-invocation leaf with
>> ecx enumerating different cache details.
>>
>> Add a new union for it in struct cpuid_policy, collect it from hardware in
>> calculate_raw_policy(), audit it in recalculate_cpuid_policy() and update
>> guest_cpuid() and update_domain_cpuid_info() to properly insert/extract 
>> data.
>>
>> A lot of the data here will need further auditing/refinement when better
>> topology support is introduced, but for now, this matches the existing
>> toolstack behaviour.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> but with a couple of remarks:
>
>> @@ -242,6 +243,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");
>> +    }
> As expressed before (perhaps in the context of another patch),
> the warning may be logged prematurely, which I'd prefer to be
> avoided.

How would you like it then?  You previously indicated that it probably
want a problem leaving it like this, which is why I did.

>
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -101,6 +101,10 @@ static int update_domain_cpuid_info(struct domain *d,
>>      switch ( ctl->input[0] )
>>      {
>>      case 0x00000000 ... ARRAY_SIZE(p->basic.raw) - 1:
>> +        if ( ctl->input[0] == 4 &&
>> +             ctl->input[1] >= ARRAY_SIZE(p->cache.raw) )
>> +            return 0;
>> +
>>          if ( ctl->input[0] == 7 &&
>>               ctl->input[1] >= ARRAY_SIZE(p->feat.raw) )
>>              return 0;
>> @@ -129,7 +133,9 @@ static int update_domain_cpuid_info(struct domain *d,
>>      switch ( ctl->input[0] )
>>      {
>>      case 0x00000000 ... ARRAY_SIZE(p->basic.raw) - 1:
>> -        if ( ctl->input[0] == 7 )
>> +        if ( ctl->input[0] == 4 )
>> +            p->cache.raw[ctl->input[1]] = leaf;
>> +        else if ( ctl->input[0] == 7 )
>>              p->feat.raw[ctl->input[1]] = leaf;
>>          else if ( ctl->input[0] == XSTATE_CPUID )
>>              p->xstate.raw[ctl->input[1]] = leaf;
> The contexts of these two hunks make it pretty likely that inner
> switch() statements would help readability.

Will do.

~Andrew

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