* [PATCH v1 0/2] x86/cpuid: Use AMD's NullSelectorClearsBase CPUID bit @ 2021-09-06 12:00 Jane Malalane 2021-09-06 12:00 ` [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests Jane Malalane 2021-09-06 12:00 ` [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs Jane Malalane 0 siblings, 2 replies; 13+ messages in thread From: Jane Malalane @ 2021-09-06 12:00 UTC (permalink / raw) To: Xen-devel; +Cc: Jane Malalane Jane Malalane (2): x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests x86/cpuid: Detect null segment behaviour on Zen2 CPUs tools/libs/light/libxl_cpuid.c | 1 + tools/misc/xen-cpuid.c | 1 + xen/arch/x86/cpu/amd.c | 18 ++++++++++++++++++ xen/arch/x86/cpu/cpu.h | 1 + xen/arch/x86/cpu/hygon.c | 5 +++++ xen/include/asm-x86/cpufeature.h | 1 + xen/include/public/arch-x86/cpufeatureset.h | 1 + 7 files changed, 28 insertions(+) -- 2.11.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests 2021-09-06 12:00 [PATCH v1 0/2] x86/cpuid: Use AMD's NullSelectorClearsBase CPUID bit Jane Malalane @ 2021-09-06 12:00 ` Jane Malalane 2021-09-06 15:04 ` Jan Beulich 2021-09-06 19:20 ` Andrew Cooper 2021-09-06 12:00 ` [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs Jane Malalane 1 sibling, 2 replies; 13+ messages in thread From: Jane Malalane @ 2021-09-06 12:00 UTC (permalink / raw) To: Xen-devel Cc: Jane Malalane, Wei Liu, Jan Beulich, Andrew Cooper, Roger Pau Monné, Pu Wen, Andy Lutomirski AMD Zen3 adds the NullSelectorClearsBase bit to indicate that loading a NULL segment selector zeroes the base and limit fields, as well as just attributes. Expose bit to all guests. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jane Malalane <jane.malalane@citrix.com> --- CC: Wei Liu <wl@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: "Roger Pau Monné" <roger.pau@citrix.com> CC: Pu Wen <puwen@hygon.cn> CC: Andy Lutomirski <luto@kernel.org> --- tools/libs/light/libxl_cpuid.c | 1 + tools/misc/xen-cpuid.c | 1 + xen/include/public/arch-x86/cpufeatureset.h | 1 + 3 files changed, 3 insertions(+) diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c index b2c673841a..d667c36f31 100644 --- a/tools/libs/light/libxl_cpuid.c +++ b/tools/libs/light/libxl_cpuid.c @@ -289,6 +289,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str) {"svm_pausefilt",0x8000000a, NA, CPUID_REG_EDX, 10, 1}, {"lfence+", 0x80000021, NA, CPUID_REG_EAX, 2, 1}, + {"nscb", 0x80000021, NA, CPUID_REG_EAX, 6, 1}, {"maxhvleaf", 0x40000000, NA, CPUID_REG_EAX, 0, 8}, diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c index 735bcf8f0e..d79e67ecfb 100644 --- a/tools/misc/xen-cpuid.c +++ b/tools/misc/xen-cpuid.c @@ -185,6 +185,7 @@ static const char *const str_7a1[32] = static const char *const str_e21a[32] = { [ 2] = "lfence+", + [ 6] = "nscb", }; static const struct { diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h index 380b51b1b3..e5a7c94c78 100644 --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -285,6 +285,7 @@ XEN_CPUFEATURE(FSRCS, 10*32+12) /*A Fast Short REP CMPSB/SCASB */ /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */ XEN_CPUFEATURE(LFENCE_DISPATCH, 11*32+ 2) /*A LFENCE always serializing */ +XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base */ #endif /* XEN_CPUFEATURE */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests 2021-09-06 12:00 ` [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests Jane Malalane @ 2021-09-06 15:04 ` Jan Beulich 2021-09-06 19:20 ` Andrew Cooper 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2021-09-06 15:04 UTC (permalink / raw) To: Jane Malalane Cc: Wei Liu, Andrew Cooper, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel On 06.09.2021 14:00, Jane Malalane wrote: > AMD Zen3 adds the NullSelectorClearsBase bit to indicate that loading > a NULL segment selector zeroes the base and limit fields, as well as > just attributes. > > Expose bit to all guests. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jane Malalane <jane.malalane@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests 2021-09-06 12:00 ` [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests Jane Malalane 2021-09-06 15:04 ` Jan Beulich @ 2021-09-06 19:20 ` Andrew Cooper 1 sibling, 0 replies; 13+ messages in thread From: Andrew Cooper @ 2021-09-06 19:20 UTC (permalink / raw) To: Jane Malalane, Xen-devel Cc: Wei Liu, Jan Beulich, Roger Pau Monné, Pu Wen, Andy Lutomirski On 06/09/2021 13:00, Jane Malalane wrote: > diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h > index 380b51b1b3..e5a7c94c78 100644 > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -285,6 +285,7 @@ XEN_CPUFEATURE(FSRCS, 10*32+12) /*A Fast Short REP CMPSB/SCASB */ > > /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */ > XEN_CPUFEATURE(LFENCE_DISPATCH, 11*32+ 2) /*A LFENCE always serializing */ > +XEN_CPUFEATURE(NSCB, 11*32+ 6) /*A Null Selector Clears Base */ On second thoughts, I'm tempted to add " (and limit too)" to the comment. Can be fixed on commit. ~Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-06 12:00 [PATCH v1 0/2] x86/cpuid: Use AMD's NullSelectorClearsBase CPUID bit Jane Malalane 2021-09-06 12:00 ` [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests Jane Malalane @ 2021-09-06 12:00 ` Jane Malalane 2021-09-06 15:17 ` Jan Beulich 2021-09-08 8:19 ` [PATCH v2 " Jane Malalane 1 sibling, 2 replies; 13+ messages in thread From: Jane Malalane @ 2021-09-06 12:00 UTC (permalink / raw) To: Xen-devel Cc: Jane Malalane, Wei Liu, Jan Beulich, Andrew Cooper, Roger Pau Monné, Pu Wen, Andy Lutomirski Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be introduced into Zen2 due to a lack of leaves. So, it was added in a new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID bit in software. So, on Zen2 hardware, Xen probes for NSCB (NullSelectorClearsBit) and synthesizes the bit. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jane Malalane <jane.malalane@citrix.com> --- CC: Wei Liu <wl@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: "Roger Pau Monné" <roger.pau@citrix.com> CC: Pu Wen <puwen@hygon.cn> CC: Andy Lutomirski <luto@kernel.org> --- xen/arch/x86/cpu/amd.c | 18 ++++++++++++++++++ xen/arch/x86/cpu/cpu.h | 1 + xen/arch/x86/cpu/hygon.c | 5 +++++ xen/include/asm-x86/cpufeature.h | 1 + 4 files changed, 25 insertions(+) diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index 2260eef3aa..654f82e2cb 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c) c->x86_capability); } +void detect_zen2_null_seg_behaviour(void) +{ + uint64_t base; + + wrmsrl(MSR_FS_BASE, 1); + asm volatile ( "mov %0, %%fs" :: "rm" (0) ); + rdmsrl(MSR_FS_BASE, base); + + if (base == 0) + setup_force_cpu_cap(X86_FEATURE_NSCB); + +} + static void init_amd(struct cpuinfo_x86 *c) { u32 l, h; @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c) else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ amd_init_lfence(c); + /* Probe for NSCB on Zen2 CPUs when not virtualised */ + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && + c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f) + detect_zen2_null_seg_behaviour(); + /* * If the user has explicitly chosen to disable Memory Disambiguation * to mitigiate Speculative Store Bypass, poke the appropriate MSR. diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h index 1ac3b2867a..0dd1b762ff 100644 --- a/xen/arch/x86/cpu/cpu.h +++ b/xen/arch/x86/cpu/cpu.h @@ -21,3 +21,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c); void early_init_amd(struct cpuinfo_x86 *c); void amd_log_freq(const struct cpuinfo_x86 *c); void amd_init_lfence(struct cpuinfo_x86 *c); +void detect_zen2_null_seg_behaviour(void); diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c index 67e23c5df9..232edb0c4d 100644 --- a/xen/arch/x86/cpu/hygon.c +++ b/xen/arch/x86/cpu/hygon.c @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c) amd_init_lfence(c); + /* Probe for NSCB on Zen2 CPUs when not virtualised */ + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && + c->x86 == 0x18 && c->x86_model >= 4) + detect_zen2_null_seg_behaviour(); + /* * If the user has explicitly chosen to disable Memory Disambiguation * to mitigiate Speculative Store Bypass, poke the appropriate MSR. diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h index 5f6b83f71c..4faf9bff29 100644 --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -146,6 +146,7 @@ #define cpu_has_cpuid_faulting boot_cpu_has(X86_FEATURE_CPUID_FAULTING) #define cpu_has_aperfmperf boot_cpu_has(X86_FEATURE_APERFMPERF) #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH) +#define cpu_has_nscb boot_cpu_has(X86_FEATURE_NSCB) #define cpu_has_xen_lbr boot_cpu_has(X86_FEATURE_XEN_LBR) #define cpu_has_xen_shstk boot_cpu_has(X86_FEATURE_XEN_SHSTK) -- 2.11.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-06 12:00 ` [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs Jane Malalane @ 2021-09-06 15:17 ` Jan Beulich 2021-09-06 18:07 ` Andrew Cooper 2021-09-08 8:19 ` [PATCH v2 " Jane Malalane 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2021-09-06 15:17 UTC (permalink / raw) To: Jane Malalane Cc: Wei Liu, Andrew Cooper, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel On 06.09.2021 14:00, Jane Malalane wrote: > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c) > c->x86_capability); > } > > +void detect_zen2_null_seg_behaviour(void) This can in principle be marked __init. > +{ > + uint64_t base; > + > + wrmsrl(MSR_FS_BASE, 1); > + asm volatile ( "mov %0, %%fs" :: "rm" (0) ); While I don't strictly mind the "m" part of the constraint to remain there (in the hope for compilers actually to support this), iirc it's not useful to have when the value is a constant: Last time I checked, the compiler would not instantiate an anonymous (stack) variable to fulfill this constraint (as can be seen when dropping the "r" part of the constraint). > @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c) > else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ > amd_init_lfence(c); > > + /* Probe for NSCB on Zen2 CPUs when not virtualised */ > + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && > + c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f) DYM 0x30 here? Or 0x1e? In any event 0x5f should be accompanied by another hex constant. And it would also help if in the description you said where these bounds as well as ... > --- a/xen/arch/x86/cpu/hygon.c > +++ b/xen/arch/x86/cpu/hygon.c > @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c) > > amd_init_lfence(c); > > + /* Probe for NSCB on Zen2 CPUs when not virtualised */ > + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && > + c->x86 == 0x18 && c->x86_model >= 4) ... this one come from. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-06 15:17 ` Jan Beulich @ 2021-09-06 18:07 ` Andrew Cooper 2021-09-07 6:09 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2021-09-06 18:07 UTC (permalink / raw) To: Jan Beulich, Jane Malalane Cc: Wei Liu, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel On 06/09/2021 16:17, Jan Beulich wrote: > On 06.09.2021 14:00, Jane Malalane wrote: >> --- a/xen/arch/x86/cpu/amd.c >> +++ b/xen/arch/x86/cpu/amd.c >> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c) >> c->x86_capability); >> } >> >> +void detect_zen2_null_seg_behaviour(void) > This can in principle be marked __init. > >> +{ >> + uint64_t base; >> + >> + wrmsrl(MSR_FS_BASE, 1); >> + asm volatile ( "mov %0, %%fs" :: "rm" (0) ); > While I don't strictly mind the "m" part of the constraint to remain > there (in the hope for compilers actually to support this), iirc it's > not useful to have when the value is a constant: Last time I checked, > the compiler would not instantiate an anonymous (stack) variable to > fulfill this constraint (as can be seen when dropping the "r" part of > the constraint). This is "rm" because it is what we use elsewhere in Xen for selectors, and because it is the correct constraints based on the legal instruction encodings. If you want to work around what you perceive to be bugs in compilers then submit a independent change yourself. >> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c) >> else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ >> amd_init_lfence(c); >> >> + /* Probe for NSCB on Zen2 CPUs when not virtualised */ >> + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && >> + c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f) > DYM 0x30 here? 0x30, although it turns out that some of the mobile Zen2 CPUs exceed 0x60 in terms of model number. As Zen3 changes the family number to 0x19, I'd just drop the upper bound. > Or 0x1e? In any event 0x5f should be accompanied by > another hex constant. And it would also help if in the description > you said where these bounds From talking to people at AMD. > as well as ... > >> --- a/xen/arch/x86/cpu/hygon.c >> +++ b/xen/arch/x86/cpu/hygon.c >> @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c) >> >> amd_init_lfence(c); >> >> + /* Probe for NSCB on Zen2 CPUs when not virtualised */ >> + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && >> + c->x86 == 0x18 && c->x86_model >= 4) > ... this one come from. From talking to people at Hygon. ~Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-06 18:07 ` Andrew Cooper @ 2021-09-07 6:09 ` Jan Beulich 2021-09-07 13:27 ` Andrew Cooper 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2021-09-07 6:09 UTC (permalink / raw) To: Andrew Cooper Cc: Wei Liu, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel, Jane Malalane On 06.09.2021 20:07, Andrew Cooper wrote: > On 06/09/2021 16:17, Jan Beulich wrote: >> On 06.09.2021 14:00, Jane Malalane wrote: >>> --- a/xen/arch/x86/cpu/amd.c >>> +++ b/xen/arch/x86/cpu/amd.c >>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c) >>> c->x86_capability); >>> } >>> >>> +void detect_zen2_null_seg_behaviour(void) >> This can in principle be marked __init. >> >>> +{ >>> + uint64_t base; >>> + >>> + wrmsrl(MSR_FS_BASE, 1); >>> + asm volatile ( "mov %0, %%fs" :: "rm" (0) ); >> While I don't strictly mind the "m" part of the constraint to remain >> there (in the hope for compilers actually to support this), iirc it's >> not useful to have when the value is a constant: Last time I checked, >> the compiler would not instantiate an anonymous (stack) variable to >> fulfill this constraint (as can be seen when dropping the "r" part of >> the constraint). > > This is "rm" because it is what we use elsewhere in Xen for selectors, > and because it is the correct constraints based on the legal instruction > encodings. grep-ing for "%%[defgs]s" reveals: efi_arch_post_exit_boot(), svm_ctxt_switch_to(), and do_set_segment_base() all use just "r". This grep has not produced any use of "rm". What are you talking about? > If you want to work around what you perceive to be bugs in compilers > then submit a independent change yourself. I don't perceive this as a bug; perhaps a desirable feature. I also did start my response with "While I don't strictly mind the "m" part ..." - was this not careful enough to indicate I'm not going to insist on the change, but I'd prefer it to be made? >>> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c) >>> else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ >>> amd_init_lfence(c); >>> >>> + /* Probe for NSCB on Zen2 CPUs when not virtualised */ >>> + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && >>> + c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f) >> DYM 0x30 here? > > 0x30, although it turns out that some of the mobile Zen2 CPUs exceed > 0x60 in terms of model number. > > As Zen3 changes the family number to 0x19, I'd just drop the upper bound. Minor note: Even if it didn't, the !cpu_has_nscb would also be enough to avoid the probing there. >> Or 0x1e? In any event 0x5f should be accompanied by >> another hex constant. And it would also help if in the description >> you said where these bounds > > From talking to people at AMD. > >> as well as ... >> >>> --- a/xen/arch/x86/cpu/hygon.c >>> +++ b/xen/arch/x86/cpu/hygon.c >>> @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c) >>> >>> amd_init_lfence(c); >>> >>> + /* Probe for NSCB on Zen2 CPUs when not virtualised */ >>> + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && >>> + c->x86 == 0x18 && c->x86_model >= 4) >> ... this one come from. > > From talking to people at Hygon. Fair enough, but imo this wants mentioning in the description. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-07 6:09 ` Jan Beulich @ 2021-09-07 13:27 ` Andrew Cooper 2021-09-07 14:21 ` Jan Beulich 0 siblings, 1 reply; 13+ messages in thread From: Andrew Cooper @ 2021-09-07 13:27 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel, Jane Malalane On 07/09/2021 07:09, Jan Beulich wrote: > On 06.09.2021 20:07, Andrew Cooper wrote: >> On 06/09/2021 16:17, Jan Beulich wrote: >>> On 06.09.2021 14:00, Jane Malalane wrote: >>>> --- a/xen/arch/x86/cpu/amd.c >>>> +++ b/xen/arch/x86/cpu/amd.c >>>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c) >>>> c->x86_capability); >>>> } >>>> >>>> +void detect_zen2_null_seg_behaviour(void) >>> This can in principle be marked __init. >>> >>>> +{ >>>> + uint64_t base; >>>> + >>>> + wrmsrl(MSR_FS_BASE, 1); >>>> + asm volatile ( "mov %0, %%fs" :: "rm" (0) ); >>> While I don't strictly mind the "m" part of the constraint to remain >>> there (in the hope for compilers actually to support this), iirc it's >>> not useful to have when the value is a constant: Last time I checked, >>> the compiler would not instantiate an anonymous (stack) variable to >>> fulfill this constraint (as can be seen when dropping the "r" part of >>> the constraint). >> This is "rm" because it is what we use elsewhere in Xen for selectors, >> and because it is the correct constraints based on the legal instruction >> encodings. > grep-ing for "%%[defgs]s" reveals: > > efi_arch_post_exit_boot(), svm_ctxt_switch_to(), These are writing multiple selectors in one go, and a register constraint is the only sane option. > and do_set_segment_base() all use just "r". I had missed this one. > This grep has not produced > any use of "rm". What are you talking about? TRY_LOAD_SEG(), pv_emul_read_descriptor() for both lar and lsl, do_double_fault() for another lsl, lldt(), ltr(). So ok - not everything, but most. > >> If you want to work around what you perceive to be bugs in compilers >> then submit a independent change yourself. > I don't perceive this as a bug; perhaps a desirable feature. I also > did start my response with "While I don't strictly mind the "m" > part ..." - was this not careful enough to indicate I'm not going > to insist on the change, but I'd prefer it to be made? No, because a maintainer saying "I'd prefer this to be changed" is still an instruction to the submitter to make the change. But the request is inappropriate. "Last time I checked, the compiler would" presumably means you've checked GCC and not Clang, and therefore any conclusions about the behaviour are incomplete. Unless there is a real concrete compiler bug to work around, "rm" is the appropriate constraint to use, all other things being equal. If the complier is merely doing something dumb with the flexibility it has been permitted, then fix the compiler and the problem will resolve itself the proper way. > >>>> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c) >>>> else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ >>>> amd_init_lfence(c); >>>> >>>> + /* Probe for NSCB on Zen2 CPUs when not virtualised */ >>>> + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && >>>> + c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f) >>> DYM 0x30 here? >> 0x30, although it turns out that some of the mobile Zen2 CPUs exceed >> 0x60 in terms of model number. >> >> As Zen3 changes the family number to 0x19, I'd just drop the upper bound. > Minor note: Even if it didn't, the !cpu_has_nscb would also be enough > to avoid the probing there. There is actually a problem. From a non-AMD source, I've found the Sucor Z+ CPU which is a Fam17h Model 0x50 Zen1. So instead I'm going to recommend dropping all model checks and just keeping the family checks. This will extend the probe function to Zen1 too, but it is once on boot, trivial in terms of complexity, and really not worth the time/effort it has taken to discover that the model list wasn't correct to start with. ~Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-07 13:27 ` Andrew Cooper @ 2021-09-07 14:21 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2021-09-07 14:21 UTC (permalink / raw) To: Andrew Cooper Cc: Wei Liu, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel, Jane Malalane On 07.09.2021 15:27, Andrew Cooper wrote: > On 07/09/2021 07:09, Jan Beulich wrote: >> On 06.09.2021 20:07, Andrew Cooper wrote: >>> On 06/09/2021 16:17, Jan Beulich wrote: >>>> On 06.09.2021 14:00, Jane Malalane wrote: >>>>> --- a/xen/arch/x86/cpu/amd.c >>>>> +++ b/xen/arch/x86/cpu/amd.c >>>>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c) >>>>> c->x86_capability); >>>>> } >>>>> >>>>> +void detect_zen2_null_seg_behaviour(void) >>>> This can in principle be marked __init. >>>> >>>>> +{ >>>>> + uint64_t base; >>>>> + >>>>> + wrmsrl(MSR_FS_BASE, 1); >>>>> + asm volatile ( "mov %0, %%fs" :: "rm" (0) ); >>>> While I don't strictly mind the "m" part of the constraint to remain >>>> there (in the hope for compilers actually to support this), iirc it's >>>> not useful to have when the value is a constant: Last time I checked, >>>> the compiler would not instantiate an anonymous (stack) variable to >>>> fulfill this constraint (as can be seen when dropping the "r" part of >>>> the constraint). >>> This is "rm" because it is what we use elsewhere in Xen for selectors, >>> and because it is the correct constraints based on the legal instruction >>> encodings. >> grep-ing for "%%[defgs]s" reveals: >> >> efi_arch_post_exit_boot(), svm_ctxt_switch_to(), > > These are writing multiple selectors in one go, and a register > constraint is the only sane option. > >> and do_set_segment_base() all use just "r". > > I had missed this one. > >> This grep has not produced >> any use of "rm". What are you talking about? > > TRY_LOAD_SEG(), pv_emul_read_descriptor() for both lar and lsl, > do_double_fault() for another lsl, lldt(), ltr(). TRY_LOAD_SEG() and pv_emul_read_descriptor() don't pass constants as asm() argument values. do_double_fault()'s use of lsl is indeed an example matching the pattern here. lldt() and ltr() are generic inline helpers, so validly allow for both because they should not make assumptions on what the caller passes. Plus "m" there is okay, because if the caller passes a constant there will be a named variable (the function parameter), i.e. the compiler does not need to instantiate any anonymous one. > So ok - not everything, but most. > >> >>> If you want to work around what you perceive to be bugs in compilers >>> then submit a independent change yourself. >> I don't perceive this as a bug; perhaps a desirable feature. I also >> did start my response with "While I don't strictly mind the "m" >> part ..." - was this not careful enough to indicate I'm not going >> to insist on the change, but I'd prefer it to be made? > > No, because a maintainer saying "I'd prefer this to be changed" is still > an instruction to the submitter to make the change. It was a request to _consider_ dropping the m part, yes. But (see below) now that you've forced me to re-check (I presume you didn't check yourself, or else I would expect you would have drawn the same conclusion as I did), I actually feel stronger about this needing adjustment. > But the request is inappropriate. "Last time I checked, the compiler > would" presumably means you've checked GCC and not Clang, and therefore > any conclusions about the behaviour are incomplete. Not really, no. IIRC I did check the version of clang that I have easy access to. (For gcc I've just now re-checked with 10.x and 11.x.) > Unless there is a real concrete compiler bug to work around, "rm" is the > appropriate constraint to use, all other things being equal. If the > complier is merely doing something dumb with the flexibility it has been > permitted, then fix the compiler and the problem will resolve itself the > proper way. I disagree. When an asm() constraint permits multiple kinds of values, dropping one or more of the alternatives should IMO still yield valid (perhaps sub-optimal) code (IOW every one of the supplied kinds should be valid). The issue here is that it is not spelled out clearly whether something like "m" (0) is actually legal. The error messages, however, suggest this is intended to be illegal: gcc says "memory input ... is not directly addressable", whereas clang says "invalid lvalue in asm input for constraint 'm'". IOW I do think the one case of LSL in do_double_fault() does need adjusting. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-06 12:00 ` [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs Jane Malalane 2021-09-06 15:17 ` Jan Beulich @ 2021-09-08 8:19 ` Jane Malalane 2021-09-08 12:08 ` Jan Beulich 1 sibling, 1 reply; 13+ messages in thread From: Jane Malalane @ 2021-09-08 8:19 UTC (permalink / raw) To: Xen-devel Cc: Jane Malalane, Wei Liu, Jan Beulich, Andrew Cooper, Roger Pau Monné, Pu Wen, Andy Lutomirski Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be introduced into Zen2 due to a lack of leaves. So, it was added in a new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID bit in software. So, Xen probes for NSCB (NullSelectorClearsBit) and synthesizes the bit, if the behaviour is present. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jane Malalane <jane.malalane@citrix.com> --- CC: Wei Liu <wl@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: "Roger Pau Monné" <roger.pau@citrix.com> CC: Pu Wen <puwen@hygon.cn> CC: Andy Lutomirski <luto@kernel.org> v2: * Mark detect_zen2_null_seg_behaviour() with __init * Remove model checks --- xen/arch/x86/cpu/amd.c | 18 ++++++++++++++++++ xen/arch/x86/cpu/cpu.h | 1 + xen/arch/x86/cpu/hygon.c | 5 +++++ xen/include/asm-x86/cpufeature.h | 1 + 4 files changed, 25 insertions(+) diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index 2260eef3aa..cb12861481 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c) c->x86_capability); } +void __init detect_zen2_null_seg_behaviour(void) +{ + uint64_t base; + + wrmsrl(MSR_FS_BASE, 1); + asm volatile ( "mov %0, %%fs" :: "rm" (0) ); + rdmsrl(MSR_FS_BASE, base); + + if (base == 0) + setup_force_cpu_cap(X86_FEATURE_NSCB); + +} + static void init_amd(struct cpuinfo_x86 *c) { u32 l, h; @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c) else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ amd_init_lfence(c); + /* Probe for NSCB on Zen2 CPUs when not virtualised */ + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && + c->x86 == 0x17) + detect_zen2_null_seg_behaviour(); + /* * If the user has explicitly chosen to disable Memory Disambiguation * to mitigiate Speculative Store Bypass, poke the appropriate MSR. diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h index 1ac3b2867a..0dd1b762ff 100644 --- a/xen/arch/x86/cpu/cpu.h +++ b/xen/arch/x86/cpu/cpu.h @@ -21,3 +21,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c); void early_init_amd(struct cpuinfo_x86 *c); void amd_log_freq(const struct cpuinfo_x86 *c); void amd_init_lfence(struct cpuinfo_x86 *c); +void detect_zen2_null_seg_behaviour(void); diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c index 67e23c5df9..d7a04af2bb 100644 --- a/xen/arch/x86/cpu/hygon.c +++ b/xen/arch/x86/cpu/hygon.c @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c) amd_init_lfence(c); + /* Probe for NSCB on Zen2 CPUs when not virtualised */ + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data && + c->x86 == 0x18) + detect_zen2_null_seg_behaviour(); + /* * If the user has explicitly chosen to disable Memory Disambiguation * to mitigiate Speculative Store Bypass, poke the appropriate MSR. diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h index 5f6b83f71c..4faf9bff29 100644 --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -146,6 +146,7 @@ #define cpu_has_cpuid_faulting boot_cpu_has(X86_FEATURE_CPUID_FAULTING) #define cpu_has_aperfmperf boot_cpu_has(X86_FEATURE_APERFMPERF) #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH) +#define cpu_has_nscb boot_cpu_has(X86_FEATURE_NSCB) #define cpu_has_xen_lbr boot_cpu_has(X86_FEATURE_XEN_LBR) #define cpu_has_xen_shstk boot_cpu_has(X86_FEATURE_XEN_SHSTK) -- 2.11.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-08 8:19 ` [PATCH v2 " Jane Malalane @ 2021-09-08 12:08 ` Jan Beulich 2021-09-08 12:28 ` Andrew Cooper 0 siblings, 1 reply; 13+ messages in thread From: Jan Beulich @ 2021-09-08 12:08 UTC (permalink / raw) To: Jane Malalane Cc: Wei Liu, Andrew Cooper, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel On 08.09.2021 10:19, Jane Malalane wrote: > Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be > introduced into Zen2 due to a lack of leaves. So, it was added in a > new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID > bit in software. Considering the prior model checks, I understand this isn't all Zen2s? No matter what the answer, I'd like to ask that the first sentence start with either "All" or "Some" (or something along these lines). Which is of course fine to insert while committing, so not need to send a v3. > So, Xen probes for NSCB (NullSelectorClearsBit) and > synthesizes the bit, if the behaviour is present. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jane Malalane <jane.malalane@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs 2021-09-08 12:08 ` Jan Beulich @ 2021-09-08 12:28 ` Andrew Cooper 0 siblings, 0 replies; 13+ messages in thread From: Andrew Cooper @ 2021-09-08 12:28 UTC (permalink / raw) To: Jan Beulich, Jane Malalane Cc: Wei Liu, Roger Pau Monné, Pu Wen, Andy Lutomirski, Xen-devel On 08/09/2021 13:08, Jan Beulich wrote: > On 08.09.2021 10:19, Jane Malalane wrote: >> Zen2 CPUs actually have this behaviour, but the CPUID bit couldn't be >> introduced into Zen2 due to a lack of leaves. So, it was added in a >> new leaf in Zen3. Nonetheless, hypervisors can synthesize the CPUID >> bit in software. > Considering the prior model checks, I understand this isn't all Zen2s? > No matter what the answer, I'd like to ask that the first sentence > start with either "All" or "Some" (or something along these lines). > Which is of course fine to insert while committing, so not need to > send a v3. All Zen2's have this behaviour. The model checks were trying to avoid running the probe on Zen1, but "model >= 0x30 && mode != 0x50" is error prone. There are no faults in the probe, and its fast, so running on all Zen1/2 is better than hoping that we got the model list correct. ~Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-08 12:28 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-06 12:00 [PATCH v1 0/2] x86/cpuid: Use AMD's NullSelectorClearsBase CPUID bit Jane Malalane 2021-09-06 12:00 ` [PATCH v1 1/2] x86/cpuid: Expose NullSelectorClearsBase CPUID bit to guests Jane Malalane 2021-09-06 15:04 ` Jan Beulich 2021-09-06 19:20 ` Andrew Cooper 2021-09-06 12:00 ` [PATCH v1 2/2] x86/cpuid: Detect null segment behaviour on Zen2 CPUs Jane Malalane 2021-09-06 15:17 ` Jan Beulich 2021-09-06 18:07 ` Andrew Cooper 2021-09-07 6:09 ` Jan Beulich 2021-09-07 13:27 ` Andrew Cooper 2021-09-07 14:21 ` Jan Beulich 2021-09-08 8:19 ` [PATCH v2 " Jane Malalane 2021-09-08 12:08 ` Jan Beulich 2021-09-08 12:28 ` Andrew Cooper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.