Re: [PATCH] x86/Intel: use CPUID bit to determine PPIN availability

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2022 13:18, Andrew Cooper wrote:
> On 17/01/2022 15:30, Jan Beulich wrote:
>> As of SDM revision 076 there is a CPUID bit for this functionality. Use
>> it to amend the existing model-based logic.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> In xen-cpuid.c I wasn't sure whether it's better to put the 7b1 table
>> next to the 7a1 one, or whether tables should continue to be placed by
>> feature set ABI identifier.
> 
> They're in FEATURESET_* order, which is also chronological order. 
> str_7b1 wants to be after str_e21a.

I can see the ordering being necessary for decodes[], but I have a
hard time seeing why there would be any strict need for a particular
ordering model for the str_...[] objects; there it would seem
slightly more natural to me to have adjacent registers / leaves
next to each other. But since I don't care that much, I'll switch.

>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -156,7 +156,7 @@ static const char *const str_e8b[32] =
>>      [18] = "ibrs-fast",        [19] = "ibrs-same-mode",
>>  
>>      [20] = "no-lmsl",
>> -    /* [22] */                 [23] = "ppin",
>> +    /* [22] */                 [23] = "amd-ppin",
> 
> We don't retrofit names like this.  If we did, loads of the speculation
> bits would need to change.
> 
> The Intel vs AMD split is clear from the leaf index, and the only reason
> we have the distinction is to fit the two bits into the same namespace.

In which case are you also asking to make the new leave simply say
"ppin"?

> Please leave this as was, to match its name in the source code.

By match you mean what? Hardly

XEN_CPUFEATURE(AMD_PPIN,      8*32+23) /*   Protected Processor Inventory 
Number */

?

>> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
>> @@ -865,6 +865,13 @@ static void intel_init_ppin(const struct
>>      {
>>          uint64_t val;
>>  
>> +    default:
> 
> Considering the comment above this switch statement, which you haven't
> edited at all, this wants a note saying that a CPUID bit was added in
> Sapphire Rapids, but older CPUs still require model-specific enumeration.

Oh, yes, I should have paid attention to that comment. Will edit.

Jan