| Message ID | 20230104111146.2094-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Work around Shstk fracturing | expand |
On 04.01.2023 12:11, Andrew Cooper wrote: > We don't actually need ecx yet, but adding it in now will reduce the amount to > which leaf 7 is out of order in a featureset. > > cpufeatureset.h remains in leaf architectrual order for the sanity of anyone > trying to locate where to insert new rows. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit ... > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -288,6 +288,9 @@ XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base (and > /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */ > XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory Number */ > > +/* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */ > +/* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */ > + ... I'm not convinced getting these ordered by other than their word indexes is quite reasonable. We can't really predict in what order elements / leaves get populated, as can best be seen from ... > /* Intel-defined CPU features, CPUID level 0x00000007:2.edx, word 13 */ > XEN_CPUFEATURE(INTEL_PSFD, 13*32+ 0) /*A MSR_SPEC_CTRL.PSFD */ > XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /* MSR_SPEC_CTRL.IPRED_DIS_* */ ... subleaf 2 already having one entry, and that one not being eax, ebx, nor ecx, but edx. AMD (extended) leaves would also always be sprinkled into the middle of any such sequence. To me it ends up more confusing (perhaps not right away, but in a couple of years time) if one needs to go hunt for what the next free index value would be. Similarly the need to re-base stuff using non-upstream index values (like is the case for the KeyLocker leaves that I have pending locally) is easier to notice when new sets are put at the end of the existing list. In any event, what I'd like to ask for as a minimum is that you insert a blank line between the two new comments. Jan
On 04.01.2023 12:11, Andrew Cooper wrote: > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -288,6 +288,9 @@ XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base (and > /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */ > XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory Number */ > > +/* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */ > +/* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */ While committing the backports of this (where I normally test-build every commit individually) I came to notice that this introduces a transient (until the next commit) build breakage: FEATURESET_NR_ENTRIES is calculated from the highest entry found; the comments here don't matter at all. Therefore ... > @@ -343,6 +352,8 @@ static inline void cpuid_policy_to_featureset( > fs[FEATURESET_e21a] = p->extd.e21a; > fs[FEATURESET_7b1] = p->feat._7b1; > fs[FEATURESET_7d2] = p->feat._7d2; > + fs[FEATURESET_7c1] = p->feat._7c1; > + fs[FEATURESET_7d1] = p->feat._7d1; > } > > /* Fill in a CPUID policy from a featureset bitmap. */ > @@ -363,6 +374,8 @@ static inline void cpuid_featureset_to_policy( > p->extd.e21a = fs[FEATURESET_e21a]; > p->feat._7b1 = fs[FEATURESET_7b1]; > p->feat._7d2 = fs[FEATURESET_7d2]; > + p->feat._7c1 = fs[FEATURESET_7c1]; > + p->feat._7d1 = fs[FEATURESET_7d1]; > } ... the compiler legitimately complains about out-of-bounds array accesses here. This is just fyi for the future (to arrange patch splitting differently); I've left the backports as they were. Jan
On 03/03/2023 7:23 am, Jan Beulich wrote: > On 04.01.2023 12:11, Andrew Cooper wrote: >> --- a/xen/include/public/arch-x86/cpufeatureset.h >> +++ b/xen/include/public/arch-x86/cpufeatureset.h >> @@ -288,6 +288,9 @@ XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base (and >> /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */ >> XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory Number */ >> >> +/* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */ >> +/* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */ > While committing the backports of this (where I normally test-build > every commit individually) I came to notice that this introduces a > transient (until the next commit) build breakage: FEATURESET_NR_ENTRIES > is calculated from the highest entry found; the comments here don't > matter at all. Therefore ... > >> @@ -343,6 +352,8 @@ static inline void cpuid_policy_to_featureset( >> fs[FEATURESET_e21a] = p->extd.e21a; >> fs[FEATURESET_7b1] = p->feat._7b1; >> fs[FEATURESET_7d2] = p->feat._7d2; >> + fs[FEATURESET_7c1] = p->feat._7c1; >> + fs[FEATURESET_7d1] = p->feat._7d1; >> } >> >> /* Fill in a CPUID policy from a featureset bitmap. */ >> @@ -363,6 +374,8 @@ static inline void cpuid_featureset_to_policy( >> p->extd.e21a = fs[FEATURESET_e21a]; >> p->feat._7b1 = fs[FEATURESET_7b1]; >> p->feat._7d2 = fs[FEATURESET_7d2]; >> + p->feat._7c1 = fs[FEATURESET_7c1]; >> + p->feat._7d1 = fs[FEATURESET_7d1]; >> } > ... the compiler legitimately complains about out-of-bounds array > accesses here. This is just fyi for the future (to arrange patch > splitting differently); I've left the backports as they were. Hmm. c/s e3662437eb43 was designed to specifically allow CPUID patches to be split like this. Which compiler? I think I agree with your analysis, but I've never seen a complaint, hence not noticing. I suspect we actually want FEATURESET_NR_ENTRIES defined in C, next to the FEATURESET_* defines, and we want to BUILD_BUG_ON() if the autogen length is larger than the C length. ~Andrew
On 03.03.2023 19:32, Andrew Cooper wrote: > On 03/03/2023 7:23 am, Jan Beulich wrote: >> On 04.01.2023 12:11, Andrew Cooper wrote: >>> --- a/xen/include/public/arch-x86/cpufeatureset.h >>> +++ b/xen/include/public/arch-x86/cpufeatureset.h >>> @@ -288,6 +288,9 @@ XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base (and >>> /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */ >>> XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory Number */ >>> >>> +/* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */ >>> +/* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */ >> While committing the backports of this (where I normally test-build >> every commit individually) I came to notice that this introduces a >> transient (until the next commit) build breakage: FEATURESET_NR_ENTRIES >> is calculated from the highest entry found; the comments here don't >> matter at all. Therefore ... >> >>> @@ -343,6 +352,8 @@ static inline void cpuid_policy_to_featureset( >>> fs[FEATURESET_e21a] = p->extd.e21a; >>> fs[FEATURESET_7b1] = p->feat._7b1; >>> fs[FEATURESET_7d2] = p->feat._7d2; >>> + fs[FEATURESET_7c1] = p->feat._7c1; >>> + fs[FEATURESET_7d1] = p->feat._7d1; >>> } >>> >>> /* Fill in a CPUID policy from a featureset bitmap. */ >>> @@ -363,6 +374,8 @@ static inline void cpuid_featureset_to_policy( >>> p->extd.e21a = fs[FEATURESET_e21a]; >>> p->feat._7b1 = fs[FEATURESET_7b1]; >>> p->feat._7d2 = fs[FEATURESET_7d2]; >>> + p->feat._7c1 = fs[FEATURESET_7c1]; >>> + p->feat._7d1 = fs[FEATURESET_7d1]; >>> } >> ... the compiler legitimately complains about out-of-bounds array >> accesses here. This is just fyi for the future (to arrange patch >> splitting differently); I've left the backports as they were. > > Hmm. c/s e3662437eb43 was designed to specifically allow CPUID patches > to be split like this. > > Which compiler? I think I agree with your analysis, but I've never seen > a complaint, hence not noticing. gcc 12 > I suspect we actually want FEATURESET_NR_ENTRIES defined in C, next to > the FEATURESET_* defines, and we want to BUILD_BUG_ON() if the autogen > length is larger than the C length. Hmm, yes, this may be the best we can do. Jan
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c index d5833e9ce879..addb3a39a11a 100644 --- a/tools/misc/xen-cpuid.c +++ b/tools/misc/xen-cpuid.c @@ -202,6 +202,14 @@ static const char *const str_7b1[32] = [ 0] = "ppin", }; +static const char *const str_7c1[32] = +{ +}; + +static const char *const str_7d1[32] = +{ +}; + static const char *const str_7d2[32] = { [ 0] = "intel-psfd", @@ -229,6 +237,8 @@ static const struct { { "0x80000021.eax", "e21a", str_e21a }, { "0x00000007:1.ebx", "7b1", str_7b1 }, { "0x00000007:2.edx", "7d2", str_7d2 }, + { "0x00000007:1.ecx", "7c1", str_7c1 }, + { "0x00000007:1.edx", "7d1", str_7d1 }, }; #define COL_ALIGN "18" diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 0412dbc915e5..b3fcf4680f3a 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -450,7 +450,8 @@ static void generic_identify(struct cpuinfo_x86 *c) cpuid_count(7, 1, &c->x86_capability[FEATURESET_7a1], &c->x86_capability[FEATURESET_7b1], - &tmp, &tmp); + &c->x86_capability[FEATURESET_7c1], + &c->x86_capability[FEATURESET_7d1]); if (max_subleaf >= 2) cpuid_count(7, 2, &tmp, &tmp, &tmp, diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h index 7915f5826f57..7a896f0e2d92 100644 --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -288,6 +288,9 @@ XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base (and /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */ XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /* Protected Processor Inventory Number */ +/* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */ +/* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */ + /* Intel-defined CPU features, CPUID level 0x00000007:2.edx, word 13 */ XEN_CPUFEATURE(INTEL_PSFD, 13*32+ 0) /*A MSR_SPEC_CTRL.PSFD */ XEN_CPUFEATURE(IPRED_CTRL, 13*32+ 1) /* MSR_SPEC_CTRL.IPRED_DIS_* */ diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h index 73a5c330365e..fa98b371eef4 100644 --- a/xen/include/xen/lib/x86/cpuid.h +++ b/xen/include/xen/lib/x86/cpuid.h @@ -18,6 +18,8 @@ #define FEATURESET_e21a 11 /* 0x80000021.eax */ #define FEATURESET_7b1 12 /* 0x00000007:1.ebx */ #define FEATURESET_7d2 13 /* 0x00000007:2.edx */ +#define FEATURESET_7c1 14 /* 0x00000007:1.ecx */ +#define FEATURESET_7d1 15 /* 0x00000007:1.edx */ struct cpuid_leaf { @@ -194,7 +196,14 @@ struct cpuid_policy uint32_t _7b1; struct { DECL_BITFIELD(7b1); }; }; - uint32_t /* c */:32, /* d */:32; + union { + uint32_t _7c1; + struct { DECL_BITFIELD(7c1); }; + }; + union { + uint32_t _7d1; + struct { DECL_BITFIELD(7d1); }; + }; /* Subleaf 2. */ uint32_t /* a */:32, /* b */:32, /* c */:32; @@ -343,6 +352,8 @@ static inline void cpuid_policy_to_featureset( fs[FEATURESET_e21a] = p->extd.e21a; fs[FEATURESET_7b1] = p->feat._7b1; fs[FEATURESET_7d2] = p->feat._7d2; + fs[FEATURESET_7c1] = p->feat._7c1; + fs[FEATURESET_7d1] = p->feat._7d1; } /* Fill in a CPUID policy from a featureset bitmap. */ @@ -363,6 +374,8 @@ static inline void cpuid_featureset_to_policy( p->extd.e21a = fs[FEATURESET_e21a]; p->feat._7b1 = fs[FEATURESET_7b1]; p->feat._7d2 = fs[FEATURESET_7d2]; + p->feat._7c1 = fs[FEATURESET_7c1]; + p->feat._7d1 = fs[FEATURESET_7d1]; } static inline uint64_t cpuid_policy_xcr0_max(const struct cpuid_policy *p)
We don't actually need ecx yet, but adding it in now will reduce the amount to which leaf 7 is out of order in a featureset. cpufeatureset.h remains in leaf architectrual order for the sanity of anyone trying to locate where to insert new rows. 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> v2: * Fix decodes[] short string --- tools/misc/xen-cpuid.c | 10 ++++++++++ xen/arch/x86/cpu/common.c | 3 ++- xen/include/public/arch-x86/cpufeatureset.h | 3 +++ xen/include/xen/lib/x86/cpuid.h | 15 ++++++++++++++- 4 files changed, 29 insertions(+), 2 deletions(-)