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

From: Jan Beulich <jbeulich@suse.com>
To: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Cc: 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: Mon, 22 May 2023 09:18:47 +0200	[thread overview]
Message-ID: <3248d4a9-13a6-d85c-65e6-151fcca79cbd@suse.com> (raw)
In-Reply-To: <ea8bb0da-6326-55d6-18b7-0ce681046d53@citrix.com>

On 19.05.2023 17:36, Andrew Cooper wrote:
> 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.

I disagree, simply because of the name of the tool we're talking about
here. It's all about CPUID, so calling that out isn't relevant. Calling
out parts which aren't CPUID is, imo.

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

I'm one of those who don't, yet I still view the number as odd here.
Imo it has no relevance in this context. But anyway, I'm going to
accept this part whichever way you want it, while I continue to be
unconvinced of the xen-cpuid side of things.

Roger, do you have any opinion here? If you and Andrew agreed, I'd
certainly accept that.

Jan