| Message ID | 20220127160940.19469-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpuid: Disntangle, and PPIN | expand |
On 27/01/2022 16: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. This should say "not use specific feature names." Fixed locally. ~Andrew > 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@citrix.com>
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@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> 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
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@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> 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
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 4a163afbfc7e..c6773c85fd3e 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -379,7 +379,7 @@ static void generic_identify(struct cpuinfo_x86 *c) u32 eax, ebx, ecx, edx, tmp; /* Get vendor name */ - cpuid(0x00000000, &c->cpuid_level, &ebx, &ecx, &edx); + cpuid(0, &c->cpuid_level, &ebx, &ecx, &edx); *(u32 *)&c->x86_vendor_id[0] = ebx; *(u32 *)&c->x86_vendor_id[8] = ecx; *(u32 *)&c->x86_vendor_id[4] = edx; @@ -394,7 +394,7 @@ static void generic_identify(struct cpuinfo_x86 *c) /* Note that the vendor-specific code below might override */ /* Model and family information. */ - cpuid(0x00000001, &eax, &ebx, &ecx, &edx); + cpuid(1, &eax, &ebx, &ecx, &edx); c->x86 = get_cpu_family(eax, &c->x86_model, &c->x86_mask); c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0); c->phys_proc_id = c->apicid; @@ -404,53 +404,53 @@ static void generic_identify(struct cpuinfo_x86 *c) /* c_early_init() may have adjusted cpuid levels/features. Reread. */ c->cpuid_level = cpuid_eax(0); - cpuid(0x00000001, &eax, &ebx, &ecx, &edx); - c->x86_capability[cpufeat_word(X86_FEATURE_FPU)] = edx; - c->x86_capability[cpufeat_word(X86_FEATURE_SSE3)] = ecx; + cpuid(1, &eax, &ebx, + &c->x86_capability[FEATURESET_1c], + &c->x86_capability[FEATURESET_1d]); if ( cpu_has(c, X86_FEATURE_CLFLUSH) ) c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8; if ( (c->cpuid_level >= CPUID_PM_LEAF) && (cpuid_ecx(CPUID_PM_LEAF) & CPUID6_ECX_APERFMPERF_CAPABILITY) ) - set_bit(X86_FEATURE_APERFMPERF, c->x86_capability); + __set_bit(X86_FEATURE_APERFMPERF, c->x86_capability); + + eax = cpuid_eax(0x80000000); + if ((eax >> 16) == 0x8000) + c->extended_cpuid_level = eax; /* AMD-defined flags: level 0x80000001 */ - c->extended_cpuid_level = cpuid_eax(0x80000000); - if ((c->extended_cpuid_level >> 16) != 0x8000) - c->extended_cpuid_level = 0; - if (c->extended_cpuid_level > 0x80000000) + if (c->extended_cpuid_level >= 0x80000001) cpuid(0x80000001, &tmp, &tmp, - &c->x86_capability[cpufeat_word(X86_FEATURE_LAHF_LM)], - &c->x86_capability[cpufeat_word(X86_FEATURE_SYSCALL)]); + &c->x86_capability[FEATURESET_e1c], + &c->x86_capability[FEATURESET_e1d]); if (c->extended_cpuid_level >= 0x80000004) get_model_name(c); /* Default name */ if (c->extended_cpuid_level >= 0x80000007) - c->x86_capability[cpufeat_word(X86_FEATURE_ITSC)] - = cpuid_edx(0x80000007); + c->x86_capability[FEATURESET_e7d] = cpuid_edx(0x80000007); if (c->extended_cpuid_level >= 0x80000008) - c->x86_capability[cpufeat_word(X86_FEATURE_CLZERO)] - = cpuid_ebx(0x80000008); + c->x86_capability[FEATURESET_e8b] = cpuid_ebx(0x80000008); if (c->extended_cpuid_level >= 0x80000021) - c->x86_capability[cpufeat_word(X86_FEATURE_LFENCE_DISPATCH)] - = cpuid_eax(0x80000021); + c->x86_capability[FEATURESET_e21a] = cpuid_eax(0x80000021); /* Intel-defined flags: level 0x00000007 */ - if ( c->cpuid_level >= 0x00000007 ) { - cpuid_count(0x00000007, 0, &eax, - &c->x86_capability[cpufeat_word(X86_FEATURE_FSGSBASE)], - &c->x86_capability[cpufeat_word(X86_FEATURE_PKU)], - &c->x86_capability[cpufeat_word(X86_FEATURE_AVX512_4VNNIW)]); - if (eax > 0) - cpuid_count(0x00000007, 1, - &c->x86_capability[cpufeat_word(X86_FEATURE_AVX512_BF16)], + if (c->cpuid_level >= 7) { + uint32_t max_subleaf; + + cpuid_count(7, 0, &max_subleaf, + &c->x86_capability[FEATURESET_7b0], + &c->x86_capability[FEATURESET_7c0], + &c->x86_capability[FEATURESET_7d0]); + if (max_subleaf >= 1) + cpuid_count(7, 1, + &c->x86_capability[FEATURESET_7a1], &tmp, &tmp, &tmp); } if (c->cpuid_level >= 0xd) cpuid_count(0xd, 1, - &c->x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)], + &c->x86_capability[FEATURESET_Da1], &tmp, &tmp, &tmp); } diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py index b953648b6572..470cd76d1c52 100755 --- a/xen/tools/gen-cpuid.py +++ b/xen/tools/gen-cpuid.py @@ -423,6 +423,8 @@ def write_results(state): """) + state.bitfields += ["uint32_t :32 /* placeholder */"] * 4 + for idx, text in enumerate(state.bitfields): state.output.write( "#define CPUID_BITFIELD_%d \\\n %s\n\n"
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@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/cpu/common.c | 54 +++++++++++++++++++++++------------------------ xen/tools/gen-cpuid.py | 2 ++ 2 files changed, 29 insertions(+), 27 deletions(-)