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