| Message ID | 20230515144259.1009245-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Introduce MSR_ARCH_CAPS into featuresets | expand |
On 15.05.2023 16:42, Andrew Cooper wrote: > Bits through 24 are already defined, meaning that we're not far off needing > the second word. Put both in right away. > > The bool bitfield names in the arch_caps union are unused, and somewhat out of > date. They'll shortly be automatically generated. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> I'm largely okay, but I'd like to raise a couple of naming / presentation questions: > --- a/tools/misc/xen-cpuid.c > +++ b/tools/misc/xen-cpuid.c > @@ -226,6 +226,14 @@ static const char *const str_7d2[32] = > [ 4] = "bhi-ctrl", [ 5] = "mcdt-no", > }; > > +static const char *const str_10Al[32] = > +{ > +}; > + > +static const char *const str_10Ah[32] = > +{ > +}; > + > static const struct { > const char *name; > const char *abbr; > @@ -248,6 +256,8 @@ static const struct { > { "0x00000007:2.edx", "7d2", str_7d2 }, > { "0x00000007:1.ecx", "7c1", str_7c1 }, > { "0x00000007:1.edx", "7d1", str_7d1 }, > + { "0x0000010a.lo", "10Al", str_10Al }, > + { "0x0000010a.hi", "10Ah", str_10Ah }, The MSR-ness can certainly be inferred from the .lo / .hi and l/h suffixes of the strings, but I wonder whether having it e.g. like { "MSR0000010a.lo", "m10Al", str_10Al }, { "MSR0000010a.hi", "m10Ah", str_10Ah }, or { "MSR[010a].lo", "m10Al", str_10Al }, { "MSR[010a].hi", "m10Ah", str_10Ah }, or even { "ARCH_CAPS.lo", "m10Al", str_10Al }, { "ARCH_CAPS.hi", "m10Ah", str_10Ah }, wouldn't make it more obvious. For the two str_*, see below. > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -307,6 +307,10 @@ XEN_CPUFEATURE(AVX_VNNI_INT8, 15*32+ 4) /*A AVX-VNNI-INT8 Instructions */ > XEN_CPUFEATURE(AVX_NE_CONVERT, 15*32+ 5) /*A AVX-NE-CONVERT Instructions */ > XEN_CPUFEATURE(CET_SSS, 15*32+18) /* CET Supervisor Shadow Stacks safe to use */ > > +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.eax, word 16 */ > + > +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */ Right here I'd be inclined to omit the MSR index; the name ought to be sufficient. > --- a/xen/include/xen/lib/x86/cpu-policy.h > +++ b/xen/include/xen/lib/x86/cpu-policy.h > @@ -20,6 +20,8 @@ > #define FEATURESET_7d2 13 /* 0x00000007:2.edx */ > #define FEATURESET_7c1 14 /* 0x00000007:1.ecx */ > #define FEATURESET_7d1 15 /* 0x00000007:1.edx */ > +#define FEATURESET_10Al 16 /* 0x0000010a.eax */ > +#define FEATURESET_10Ah 17 /* 0x0000010a.edx */ Just like we use an "e" prefix for extended CPUID leaves, perhaps use an "m" prefix for MSRs (then also affecting e.g. the str_* above)? Jan
On 16/05/2023 1:02 pm, Jan Beulich wrote: > On 15.05.2023 16:42, Andrew Cooper wrote: >> Bits through 24 are already defined, meaning that we're not far off needing >> the second word. Put both in right away. >> >> The bool bitfield names in the arch_caps union are unused, and somewhat out of >> date. They'll shortly be automatically generated. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > I'm largely okay, but I'd like to raise a couple of naming / presentation > questions: > >> --- a/tools/misc/xen-cpuid.c >> +++ b/tools/misc/xen-cpuid.c >> @@ -226,6 +226,14 @@ static const char *const str_7d2[32] = >> [ 4] = "bhi-ctrl", [ 5] = "mcdt-no", >> }; >> >> +static const char *const str_10Al[32] = >> +{ >> +}; >> + >> +static const char *const str_10Ah[32] = >> +{ >> +}; >> + >> static const struct { >> const char *name; >> const char *abbr; >> @@ -248,6 +256,8 @@ static const struct { >> { "0x00000007:2.edx", "7d2", str_7d2 }, >> { "0x00000007:1.ecx", "7c1", str_7c1 }, >> { "0x00000007:1.edx", "7d1", str_7d1 }, >> + { "0x0000010a.lo", "10Al", str_10Al }, >> + { "0x0000010a.hi", "10Ah", str_10Ah }, > The MSR-ness can certainly be inferred from the .lo / .hi and l/h > suffixes of the strings, but I wonder whether having it e.g. like > > { "MSR0000010a.lo", "m10Al", str_10Al }, > { "MSR0000010a.hi", "m10Ah", str_10Ah }, > > or > > { "MSR[010a].lo", "m10Al", str_10Al }, > { "MSR[010a].hi", "m10Ah", str_10Ah }, > > or even > > { "ARCH_CAPS.lo", "m10Al", str_10Al }, > { "ARCH_CAPS.hi", "m10Ah", str_10Ah }, > > wouldn't make it more obvious. Well, it's takes something which is consistent, and introduces inconsistencies. The e is logically part of the leaf number, so using m for MSRs is not equivalent. If you do want to prefix MSRs, you need to prefix the non-extended leaves, and c isn't obviously CPUID when there's the Centaur range at 0xC000xxxx Nor can you reasonably have MSR[...] in the long names without CPUID[...] too, and that's taking some pretty long lines and making them even longer. > For the two str_*, see below. > >> --- a/xen/include/public/arch-x86/cpufeatureset.h >> +++ b/xen/include/public/arch-x86/cpufeatureset.h >> @@ -307,6 +307,10 @@ XEN_CPUFEATURE(AVX_VNNI_INT8, 15*32+ 4) /*A AVX-VNNI-INT8 Instructions */ >> XEN_CPUFEATURE(AVX_NE_CONVERT, 15*32+ 5) /*A AVX-NE-CONVERT Instructions */ >> XEN_CPUFEATURE(CET_SSS, 15*32+18) /* CET Supervisor Shadow Stacks safe to use */ >> >> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.eax, word 16 */ >> + >> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */ > Right here I'd be inclined to omit the MSR index; the name ought to > be sufficient. It doesn't hurt to have it, and it it might be helpful for people who don't know the indices off by heart. ~Andrew
On 19.05.2023 17:36, Andrew Cooper wrote: > On 16/05/2023 1:02 pm, Jan Beulich wrote: >> On 15.05.2023 16:42, Andrew Cooper wrote: >>> Bits through 24 are already defined, meaning that we're not far off needing >>> the second word. Put both in right away. >>> >>> The bool bitfield names in the arch_caps union are unused, and somewhat out of >>> date. They'll shortly be automatically generated. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> I'm largely okay, but I'd like to raise a couple of naming / presentation >> questions: >> >>> --- a/tools/misc/xen-cpuid.c >>> +++ b/tools/misc/xen-cpuid.c >>> @@ -226,6 +226,14 @@ static const char *const str_7d2[32] = >>> [ 4] = "bhi-ctrl", [ 5] = "mcdt-no", >>> }; >>> >>> +static const char *const str_10Al[32] = >>> +{ >>> +}; >>> + >>> +static const char *const str_10Ah[32] = >>> +{ >>> +}; >>> + >>> static const struct { >>> const char *name; >>> const char *abbr; >>> @@ -248,6 +256,8 @@ static const struct { >>> { "0x00000007:2.edx", "7d2", str_7d2 }, >>> { "0x00000007:1.ecx", "7c1", str_7c1 }, >>> { "0x00000007:1.edx", "7d1", str_7d1 }, >>> + { "0x0000010a.lo", "10Al", str_10Al }, >>> + { "0x0000010a.hi", "10Ah", str_10Ah }, >> The MSR-ness can certainly be inferred from the .lo / .hi and l/h >> suffixes of the strings, but I wonder whether having it e.g. like >> >> { "MSR0000010a.lo", "m10Al", str_10Al }, >> { "MSR0000010a.hi", "m10Ah", str_10Ah }, >> >> or >> >> { "MSR[010a].lo", "m10Al", str_10Al }, >> { "MSR[010a].hi", "m10Ah", str_10Ah }, >> >> or even >> >> { "ARCH_CAPS.lo", "m10Al", str_10Al }, >> { "ARCH_CAPS.hi", "m10Ah", str_10Ah }, >> >> wouldn't make it more obvious. > > Well, it's takes something which is consistent, and introduces > inconsistencies. > > The e is logically part of the leaf number, so using m for MSRs is not > equivalent. If you do want to prefix MSRs, you need to prefix the > non-extended leaves, and c isn't obviously CPUID when there's the > Centaur range at 0xC000xxxx > > Nor can you reasonably have MSR[...] in the long names without > CPUID[...] too, and that's taking some pretty long lines and making them > even longer. I disagree, simply because of the name of the tool we're talking about here. It's all about CPUID, so calling that out isn't relevant. Calling out parts which aren't CPUID is, imo. >>> --- a/xen/include/public/arch-x86/cpufeatureset.h >>> +++ b/xen/include/public/arch-x86/cpufeatureset.h >>> @@ -307,6 +307,10 @@ XEN_CPUFEATURE(AVX_VNNI_INT8, 15*32+ 4) /*A AVX-VNNI-INT8 Instructions */ >>> XEN_CPUFEATURE(AVX_NE_CONVERT, 15*32+ 5) /*A AVX-NE-CONVERT Instructions */ >>> XEN_CPUFEATURE(CET_SSS, 15*32+18) /* CET Supervisor Shadow Stacks safe to use */ >>> >>> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.eax, word 16 */ >>> + >>> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */ >> Right here I'd be inclined to omit the MSR index; the name ought to >> be sufficient. > > It doesn't hurt to have it, and it it might be helpful for people who > don't know the indices off by heart. I'm one of those who don't, yet I still view the number as odd here. Imo it has no relevance in this context. But anyway, I'm going to accept this part whichever way you want it, while I continue to be unconvinced of the xen-cpuid side of things. Roger, do you have any opinion here? If you and Andrew agreed, I'd certainly accept that. Jan
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c index 8ec143ebc854..258584aafb9f 100644 --- a/tools/misc/xen-cpuid.c +++ b/tools/misc/xen-cpuid.c @@ -226,6 +226,14 @@ static const char *const str_7d2[32] = [ 4] = "bhi-ctrl", [ 5] = "mcdt-no", }; +static const char *const str_10Al[32] = +{ +}; + +static const char *const str_10Ah[32] = +{ +}; + static const struct { const char *name; const char *abbr; @@ -248,6 +256,8 @@ static const struct { { "0x00000007:2.edx", "7d2", str_7d2 }, { "0x00000007:1.ecx", "7c1", str_7c1 }, { "0x00000007:1.edx", "7d1", str_7d1 }, + { "0x0000010a.lo", "10Al", str_10Al }, + { "0x0000010a.hi", "10Ah", str_10Ah }, }; #define COL_ALIGN "18" diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h index 8de73aebc3e0..032cec3ccba2 100644 --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -307,6 +307,10 @@ XEN_CPUFEATURE(AVX_VNNI_INT8, 15*32+ 4) /*A AVX-VNNI-INT8 Instructions */ XEN_CPUFEATURE(AVX_NE_CONVERT, 15*32+ 5) /*A AVX-NE-CONVERT Instructions */ XEN_CPUFEATURE(CET_SSS, 15*32+18) /* CET Supervisor Shadow Stacks safe to use */ +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.eax, word 16 */ + +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */ + #endif /* XEN_CPUFEATURE */ /* Clean up from a default include. Close the enum (for C). */ diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h index bfa425060464..9b51f8330f92 100644 --- a/xen/include/xen/lib/x86/cpu-policy.h +++ b/xen/include/xen/lib/x86/cpu-policy.h @@ -20,6 +20,8 @@ #define FEATURESET_7d2 13 /* 0x00000007:2.edx */ #define FEATURESET_7c1 14 /* 0x00000007:1.ecx */ #define FEATURESET_7d1 15 /* 0x00000007:1.edx */ +#define FEATURESET_10Al 16 /* 0x0000010a.eax */ +#define FEATURESET_10Ah 17 /* 0x0000010a.edx */ struct cpuid_leaf { @@ -350,17 +352,13 @@ struct cpu_policy * fixed in hardware. */ union { - uint32_t raw; + uint64_t raw; + struct { + uint32_t lo, hi; + }; struct { - bool rdcl_no:1; - bool ibrs_all:1; - bool rsba:1; - bool skip_l1dfl:1; - bool ssb_no:1; - bool mds_no:1; - bool if_pschange_mc_no:1; - bool tsx_ctrl:1; - bool taa_no:1; + DECL_BITFIELD(10Al); + DECL_BITFIELD(10Ah); }; } arch_caps; diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c index 68aafb404927..a9f31858aeff 100644 --- a/xen/lib/x86/cpuid.c +++ b/xen/lib/x86/cpuid.c @@ -79,6 +79,8 @@ void x86_cpu_policy_to_featureset( fs[FEATURESET_7d2] = p->feat._7d2; fs[FEATURESET_7c1] = p->feat._7c1; fs[FEATURESET_7d1] = p->feat._7d1; + fs[FEATURESET_10Al] = p->arch_caps.lo; + fs[FEATURESET_10Ah] = p->arch_caps.hi; } void x86_cpu_featureset_to_policy( @@ -100,6 +102,8 @@ void x86_cpu_featureset_to_policy( p->feat._7d2 = fs[FEATURESET_7d2]; p->feat._7c1 = fs[FEATURESET_7c1]; p->feat._7d1 = fs[FEATURESET_7d1]; + p->arch_caps.lo = fs[FEATURESET_10Al]; + p->arch_caps.hi = fs[FEATURESET_10Ah]; } void x86_cpu_policy_recalc_synth(struct cpu_policy *p)
Bits through 24 are already defined, meaning that we're not far off needing the second word. Put both in right away. The bool bitfield names in the arch_caps union are unused, and somewhat out of date. They'll shortly be automatically generated. 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> --- tools/misc/xen-cpuid.c | 10 ++++++++++ xen/include/public/arch-x86/cpufeatureset.h | 4 ++++ xen/include/xen/lib/x86/cpu-policy.h | 18 ++++++++---------- xen/lib/x86/cpuid.c | 4 ++++ 4 files changed, 26 insertions(+), 10 deletions(-)