Re: [PATCH 1/3] x86/cpuid: Disentangle logic for new feature leaves

[Andrew Cooper <amc96@xxxxxxxx>]

On 27/01/2022 16:39, Jan Beulich wrote:
> On 27.01.2022 17:09, Andrew Cooper wrote:
>> Adding a new feature leaf is a reasonable amount of boilerplate and for the
>> patch to build, at least one feature from the new leaf needs defining.  This
>> typically causes two non-trivial changes to be merged together.
>>
>> First, have gen-cpuid.py write out some extra placeholder defines:
>>
>>   #define CPUID_BITFIELD_11 bool :1, :1, lfence_dispatch:1, ...
>>   #define CPUID_BITFIELD_12 uint32_t :32 /* placeholder */
>>   #define CPUID_BITFIELD_13 uint32_t :32 /* placeholder */
>>   #define CPUID_BITFIELD_14 uint32_t :32 /* placeholder */
>>   #define CPUID_BITFIELD_15 uint32_t :32 /* placeholder */
>>
>> This allows DECL_BITFIELD() to be added to struct cpuid_policy without
>> requiring a XEN_CPUFEATURE() declared for the leaf.  The choice of 4 is
>> arbitrary, and allows us to add more than one leaf at a time if necessary.
>>
>> Second, rework generic_identify() to not use feature leaf names.
>>
>> The choice of deriving the index from a feature was to avoid mismatches, but
>> its correctness depends on bugs like c/s 249e0f1d8f20 ("x86/cpuid: Fix
>> TSXLDTRK definition") not happening.
>>
>> Switch to using FEATURESET_* just like the policy/featureset helpers.  This
>> breaks the cognitive complexity of needing to know which leaf a specifically
>> named feature should reside in, and is shorter to write.  It is also far
>> easier to identify as correct at a glance, given the correlation with the
>> CPUID leaf being read.
>>
>> In addition, tidy up some other bits of generic_identify()
>>  * Drop leading zeros from leaf numbers.
>>  * Don't use a locked update for X86_FEATURE_APERFMPERF.
>>  * Rework extended_cpuid_level calculation to avoid setting it twice.
>>  * Use "leaf >= $N" consistently so $N matches with the CPUID input.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

>
> I will admit that I'm not sure yet whether I will want to break up the
> KeyLocker patch just like you did with the PPIN one.
>
> The one thing that worries me some that there's still the unvalidated
> connection between FEATURESET_* and the numbers used in the public
> cpufeatureset.h. But I have no good idea (yet) for a build-time check.

I was also struggling with that.  One thing I considered was having some
semantic structure to

/* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */

and have FEATURESET_* written automatically too, but I can't think of a
nice way of organising this right now.

~Andrew