| Message ID | 2d13a663-f03e-b1e2-0c38-5dc3282dab10@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/Intel: PPIN recognition adjustments | expand |
On 20/01/2022 14:17, Jan Beulich wrote: > --- a/tools/misc/xen-cpuid.c > +++ b/tools/misc/xen-cpuid.c > @@ -195,6 +195,11 @@ static const char *const str_e21a[32] = > [ 6] = "nscb", > }; > > +static const char *const str_7b1[32] = > +{ > + [ 0] = "ppin", > +}; I hadn't realised what a mess we had with the prefixes. We have AMD_PPIN rendered as simply "ppin", while we also have AMD_{STIBP,SSBD} which are rendered with an amd- prefix. This patch is the first introduction of an INTEL_ prefixed feature. We should figure out a consistency rule and fix the logic, before adding more confusion. Given the AMD MSR_SPEC_CTRL series just posted, use of CPUID bits will often be symmetrical and it's awkward to have one or with a prefix and the other without. > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -299,6 +299,9 @@ XEN_CPUFEATURE(FSRCS, 10*32+12) / > XEN_CPUFEATURE(LFENCE_DISPATCH, 11*32+ 2) /*A LFENCE always serializing */ > XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base (and limit too) */ > > +/* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */ > +XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory Number */ > + > #endif /* XEN_CPUFEATURE */ > > /* Clean up from a default include. Close the enum (for C). */ > --- a/xen/include/xen/lib/x86/cpuid.h > +++ b/xen/include/xen/lib/x86/cpuid.h > @@ -16,6 +16,7 @@ > #define FEATURESET_7d0 9 /* 0x00000007:0.edx */ > #define FEATURESET_7a1 10 /* 0x00000007:1.eax */ > #define FEATURESET_e21a 11 /* 0x80000021.eax */ > +#define FEATURESET_7b1 12 /* 0x00000007:1.ebx */ > > struct cpuid_leaf > { > @@ -188,6 +189,10 @@ struct cpuid_policy > uint32_t _7a1; > struct { DECL_BITFIELD(7a1); }; > }; > + union { > + uint32_t _7b1; > + struct { DECL_BITFIELD(7b1); }; > + }; > }; > } feat; > > Looking at a related patch I've got, at a minimum, you also need: * collect the leaf in generic_identify() * extend cpuid_policy_to_featureset() and cpuid_featureset_to_policy() However I've got an idea to help us split "add new leaf" from "first bit in new leaf" which I'd like to experiment with first. It is rather awkward having the two mostly-unrelated changes forced together in a single patch. ~Andrew
On 27.01.2022 00:30, Andrew Cooper wrote: > On 20/01/2022 14:17, Jan Beulich wrote: >> --- a/tools/misc/xen-cpuid.c >> +++ b/tools/misc/xen-cpuid.c >> @@ -195,6 +195,11 @@ static const char *const str_e21a[32] = >> [ 6] = "nscb", >> }; >> >> +static const char *const str_7b1[32] = >> +{ >> + [ 0] = "ppin", >> +}; > > I hadn't realised what a mess we had with the prefixes. > > We have AMD_PPIN rendered as simply "ppin", while we also have > AMD_{STIBP,SSBD} which are rendered with an amd- prefix. This patch is > the first introduction of an INTEL_ prefixed feature. > > We should figure out a consistency rule and fix the logic, before adding > more confusion. > > Given the AMD MSR_SPEC_CTRL series just posted, use of CPUID bits will > often be symmetrical and it's awkward to have one or with a prefix and > the other without. IOW you suggest I add a kind-of-prereq patch to drop the prefixes? >> --- a/xen/include/xen/lib/x86/cpuid.h >> +++ b/xen/include/xen/lib/x86/cpuid.h >> @@ -16,6 +16,7 @@ >> #define FEATURESET_7d0 9 /* 0x00000007:0.edx */ >> #define FEATURESET_7a1 10 /* 0x00000007:1.eax */ >> #define FEATURESET_e21a 11 /* 0x80000021.eax */ >> +#define FEATURESET_7b1 12 /* 0x00000007:1.ebx */ >> >> struct cpuid_leaf >> { >> @@ -188,6 +189,10 @@ struct cpuid_policy >> uint32_t _7a1; >> struct { DECL_BITFIELD(7a1); }; >> }; >> + union { >> + uint32_t _7b1; >> + struct { DECL_BITFIELD(7b1); }; >> + }; >> }; >> } feat; >> >> > > Looking at a related patch I've got, at a minimum, you also need: > * collect the leaf in generic_identify() I'll need to make a patch first to collect 7a1, as it seems. It was actually 7a1 that I used as a reference, iirc. > * extend cpuid_policy_to_featureset() and cpuid_featureset_to_policy() Yeah, I missed those. Presumably by looking for instances only under arch/x86/. Especially with per-arch include/ now living under arch/<arch>/, having separate x86 bits elsewhere is a little unhelpful. > However I've got an idea to help us split "add new leaf" from "first bit > in new leaf" which I'd like to experiment with first. It is rather > awkward having the two mostly-unrelated changes forced together in a > single patch. I'll make the necessary adjustments here anyway. I can always re-base on top of what you may come up with. Jan
On 27.01.2022 08:56, Jan Beulich wrote: > On 27.01.2022 00:30, Andrew Cooper wrote: >> On 20/01/2022 14:17, Jan Beulich wrote: >>> @@ -188,6 +189,10 @@ struct cpuid_policy >>> uint32_t _7a1; >>> struct { DECL_BITFIELD(7a1); }; >>> }; >>> + union { >>> + uint32_t _7b1; >>> + struct { DECL_BITFIELD(7b1); }; >>> + }; >>> }; >>> } feat; >>> >>> >> >> Looking at a related patch I've got, at a minimum, you also need: >> * collect the leaf in generic_identify() > > I'll need to make a patch first to collect 7a1, as it seems. It was > actually 7a1 that I used as a reference, iirc. Actually that's there, just that I didn't spot it when looking for the "(7, " pattern in cpu/common.c. This form is used only in early_cpu_init(), while generic_identify() uses cpuid_count(0x00000007, ...). All quite inconsistent ... Jan
--- a/tools/misc/xen-cpuid.c +++ b/tools/misc/xen-cpuid.c @@ -195,6 +195,11 @@ static const char *const str_e21a[32] = [ 6] = "nscb", }; +static const char *const str_7b1[32] = +{ + [ 0] = "ppin", +}; + static const struct { const char *name; const char *abbr; @@ -213,6 +218,7 @@ static const struct { { "0x00000007:0.edx", "7d0", str_7d0 }, { "0x00000007:1.eax", "7a1", str_7a1 }, { "0x80000021.eax", "e21a", str_e21a }, + { "0x00000007:1.ebx", "7b1", str_7b1 }, }; #define COL_ALIGN "18" --- a/xen/arch/x86/cpu/mcheck/mce_intel.c +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c @@ -859,12 +859,20 @@ static void intel_init_ppin(const struct /* * Even if testing the presence of the MSR would be enough, we don't * want to risk the situation where other models reuse this MSR for - * other purposes. + * other purposes. Despite the late addition of a CPUID bit (rendering + * the MSR architectural), keep using the same detection logic there. */ switch ( c->x86_model ) { uint64_t val; + default: + if ( !cpu_has(c, X86_FEATURE_INTEL_PPIN) ) + { + ppin_msr = 0; + return; + } + fallthrough; case 0x3e: /* IvyBridge X */ case 0x3f: /* Haswell X */ case 0x4f: /* Broadwell X */ --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -299,6 +299,9 @@ XEN_CPUFEATURE(FSRCS, 10*32+12) / XEN_CPUFEATURE(LFENCE_DISPATCH, 11*32+ 2) /*A LFENCE always serializing */ XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base (and limit too) */ +/* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */ +XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory Number */ + #endif /* XEN_CPUFEATURE */ /* Clean up from a default include. Close the enum (for C). */ --- a/xen/include/xen/lib/x86/cpuid.h +++ b/xen/include/xen/lib/x86/cpuid.h @@ -16,6 +16,7 @@ #define FEATURESET_7d0 9 /* 0x00000007:0.edx */ #define FEATURESET_7a1 10 /* 0x00000007:1.eax */ #define FEATURESET_e21a 11 /* 0x80000021.eax */ +#define FEATURESET_7b1 12 /* 0x00000007:1.ebx */ struct cpuid_leaf { @@ -188,6 +189,10 @@ struct cpuid_policy uint32_t _7a1; struct { DECL_BITFIELD(7a1); }; }; + union { + uint32_t _7b1; + struct { DECL_BITFIELD(7b1); }; + }; }; } feat;
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@suse.com> --- It continues to be unclear for which CPU models, if any, the PPIN_CAP bit in PLATFORM_INFO could be used in favor of a model check. --- v2: Don't rename AMD's identifier in xen-cpuid.c. Name Intel's just "ppin" as well. Move str_7b1[]. Update a comment.