Re: [PATCH 3/6] x86/cpu-policy: Infrastructure for MSR_ARCH_CAPS

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 3/6] x86/cpu-policy: Infrastructure for MSR_ARCH_CAPS
Date: Fri, 19 May 2023 16:36:39 +0100	[thread overview]
Message-ID: <ea8bb0da-6326-55d6-18b7-0ce681046d53@citrix.com> (raw)
In-Reply-To: <347219e4-6c3a-a0ad-b010-4dbd7282c7ad@suse.com>

On 16/05/2023 1:02 pm, Jan Beulich wrote:
> On 15.05.2023 16:42, 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.
>>
>> The bool bitfield names in the arch_caps union are unused, and somewhat out of
>> date.  They'll shortly be automatically generated.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I'm largely okay, but I'd like to raise a couple of naming / presentation
> questions:
>
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -226,6 +226,14 @@ 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;
>> @@ -248,6 +256,8 @@ static const struct {
>>      { "0x00000007:2.edx", "7d2", str_7d2 },
>>      { "0x00000007:1.ecx", "7c1", str_7c1 },
>>      { "0x00000007:1.edx", "7d1", str_7d1 },
>> +    { "0x0000010a.lo",   "10Al", str_10Al },
>> +    { "0x0000010a.hi",   "10Ah", str_10Ah },
> The MSR-ness can certainly be inferred from the .lo / .hi and l/h
> suffixes of the strings, but I wonder whether having it e.g. like
>
>     { "MSR0000010a.lo",   "m10Al", str_10Al },
>     { "MSR0000010a.hi",   "m10Ah", str_10Ah },
>
> or
>
>     { "MSR[010a].lo",   "m10Al", str_10Al },
>     { "MSR[010a].hi",   "m10Ah", str_10Ah },
>
> or even
>
>     { "ARCH_CAPS.lo",   "m10Al", str_10Al },
>     { "ARCH_CAPS.hi",   "m10Ah", str_10Ah },
>
> wouldn't make it more obvious.

Well, it's takes something which is consistent, and introduces
inconsistencies.

The e is logically part of the leaf number, so using m for MSRs is not
equivalent.  If you do want to prefix MSRs, you need to prefix the
non-extended leaves, and c isn't obviously CPUID when there's the
Centaur range at 0xC000xxxx

Nor can you reasonably have MSR[...] in the long names without
CPUID[...] too, and that's taking some pretty long lines and making them
even longer.

>  For the two str_*, see below.
>
>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>> @@ -307,6 +307,10 @@ XEN_CPUFEATURE(AVX_VNNI_INT8,      15*32+ 4) /*A  AVX-VNNI-INT8 Instructions */
>>  XEN_CPUFEATURE(AVX_NE_CONVERT,     15*32+ 5) /*A  AVX-NE-CONVERT Instructions */
>>  XEN_CPUFEATURE(CET_SSS,            15*32+18) /*   CET Supervisor Shadow Stacks safe to use */
>>  
>> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.eax, word 16 */
>> +
>> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */
> Right here I'd be inclined to omit the MSR index; the name ought to
> be sufficient.

It doesn't hurt to have it, and it it might be helpful for people who
don't know the indices off by heart.

~Andrew