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