[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/3] x86/cpuid: Disentangle logic for new feature leaves
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |