Re: [PATCH v2 03/10] x86/cpu-policy: Infrastructure for MSR_ARCH_CAPS

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/05/2023 3:53 pm, Jan Beulich wrote:
> On 24.05.2023 13:25, Andrew Cooper wrote:
>> Bits through 24 are already defined, meaning that we're not far off needing
>> the second word.  Put both in right away.
>>
>> As both halves are present now, the arch_caps field is full width.  Adjust 
>> the
>> unit test, which notices.
>>
>> The bool bitfield names in the arch_caps union are unused, and somewhat out 
>> of
>> date.  They'll shortly be automatically generated.
>>
>> Add CPUID and MSR prefixes to the ./xen-cpuid verbose output, now that there
>> are a mix of the two.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

> albeit ...
>
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -226,31 +226,41 @@ static const char *const str_7d2[32] =
>>      [ 4] = "bhi-ctrl",      [ 5] = "mcdt-no",
>>  };
>>  
>> +static const char *const str_10Al[32] =
>> +{
>> +};
>> +
>> +static const char *const str_10Ah[32] =
>> +{
>> +};
>> +
>>  static const struct {
>>      const char *name;
>>      const char *abbr;
>>      const char *const *strs;
>>  } decodes[] =
>>  {
>> -    { "0x00000001.edx",   "1d",  str_1d },
>> -    { "0x00000001.ecx",   "1c",  str_1c },
>> -    { "0x80000001.edx",   "e1d", str_e1d },
>> -    { "0x80000001.ecx",   "e1c", str_e1c },
>> -    { "0x0000000d:1.eax", "Da1", str_Da1 },
>> -    { "0x00000007:0.ebx", "7b0", str_7b0 },
>> -    { "0x00000007:0.ecx", "7c0", str_7c0 },
>> -    { "0x80000007.edx",   "e7d", str_e7d },
>> -    { "0x80000008.ebx",   "e8b", str_e8b },
>> -    { "0x00000007:0.edx", "7d0", str_7d0 },
>> -    { "0x00000007:1.eax", "7a1", str_7a1 },
>> -    { "0x80000021.eax",  "e21a", str_e21a },
>> -    { "0x00000007:1.ebx", "7b1", str_7b1 },
>> -    { "0x00000007:2.edx", "7d2", str_7d2 },
>> -    { "0x00000007:1.ecx", "7c1", str_7c1 },
>> -    { "0x00000007:1.edx", "7d1", str_7d1 },
>> +    { "CPUID 0x00000001.edx",        "1d", str_1d },
>> +    { "CPUID 0x00000001.ecx",        "1c", str_1c },
>> +    { "CPUID 0x80000001.edx",       "e1d", str_e1d },
>> +    { "CPUID 0x80000001.ecx",       "e1c", str_e1c },
>> +    { "CPUID 0x0000000d:1.eax",     "Da1", str_Da1 },
>> +    { "CPUID 0x00000007:0.ebx",     "7b0", str_7b0 },
>> +    { "CPUID 0x00000007:0.ecx",     "7c0", str_7c0 },
>> +    { "CPUID 0x80000007.edx",       "e7d", str_e7d },
>> +    { "CPUID 0x80000008.ebx",       "e8b", str_e8b },
>> +    { "CPUID 0x00000007:0.edx",     "7d0", str_7d0 },
>> +    { "CPUID 0x00000007:1.eax",     "7a1", str_7a1 },
>> +    { "CPUID 0x80000021.eax",      "e21a", str_e21a },
>> +    { "CPUID 0x00000007:1.ebx",     "7b1", str_7b1 },
>> +    { "CPUID 0x00000007:2.edx",     "7d2", str_7d2 },
>> +    { "CPUID 0x00000007:1.ecx",     "7c1", str_7c1 },
>> +    { "CPUID 0x00000007:1.edx",     "7d1", str_7d1 },
> ... I'm not really happy about this added verbosity. In a tool of this
> name, I think it's pretty clear that unadorned names are CPUID stuff.

You might make the connection, but it's unreasonable to expect the same
of everyone else.  This is used by end users.

If nothing else, the name of the binary is made stale by this change.

>> +    { "MSR   0x0000010a.lo",      "m10Al", str_10Al },
>> +    { "MSR   0x0000010a.hi",      "m10Ah", str_10Ah },
> Once we gain a few more MSRs, I'm afraid the raw numbers aren't going
> to be very useful. As vaguely suggested before, how about
>
>     { "MSR_ARCH_CAPS.lo",      "m10Al", str_10Al },
>     { "MSR_ARCH_CAPS.hi",      "m10Ah", str_10Ah },
>
> ?

I've done this.  I remain to be convinced, but it probably is nicer for
people who don't know the MSR indices like I do.

~Andrew