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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>

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.

Jan