| Message ID | 20221011160245.56735-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | amd/virt_ssbd: refactoring and cleanup | expand |
On 11.10.2022 18:02, Roger Pau Monne wrote: > @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the > By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and > when hardware hints recommend using it as a blanket setting. > > -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` > -option can be used to force or prevent Xen using the feature itself. Why would we want to take away this level of control? Shouldn't we turn this on while in Xen if so requested? Which would then either mean enabling it on VMEXIT if a guest has it off, or running with it turned on using the OR of guest and host settings. Jan
On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote: > On 11.10.2022 18:02, Roger Pau Monne wrote: > > @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the > > By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and > > when hardware hints recommend using it as a blanket setting. > > > > -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` > > -option can be used to force or prevent Xen using the feature itself. > > Why would we want to take away this level of control? Shouldn't we turn this > on while in Xen if so requested? Which would then either mean enabling it on > VMEXIT if a guest has it off, or running with it turned on using the OR of > guest and host settings. Right, but then we need to context switch the value on vm{entry,exit} which is problematic. I could move the context switch code code out of the GIF=0 region, and assume that NMIs executing with the guest selection of SSBD are OK. Alternatively setting ssbd= on the command line could be taken as a value to enforce for the whole system and prevent guest attempts to change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't looked at whether not exposing the SSBD CPUID related to SPEC_CTRL.SSBD will have impact on other features). I was under the impression that the command line ssbd option was added to cope with Xen not exposing the feature to guests. Now that the feature is exposed guests should be free to make use of it, and hence there's no need to force a value for Xen. Thanks, Roger.
On 13.10.2022 15:50, Roger Pau Monné wrote: > On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote: >> On 11.10.2022 18:02, Roger Pau Monne wrote: >>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the >>> By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and >>> when hardware hints recommend using it as a blanket setting. >>> >>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` >>> -option can be used to force or prevent Xen using the feature itself. >> >> Why would we want to take away this level of control? Shouldn't we turn this >> on while in Xen if so requested? Which would then either mean enabling it on >> VMEXIT if a guest has it off, or running with it turned on using the OR of >> guest and host settings. > > Right, but then we need to context switch the value on vm{entry,exit} > which is problematic. I could move the context switch code code out > of the GIF=0 region, and assume that NMIs executing with the guest > selection of SSBD are OK. > > Alternatively setting ssbd= on the command line could be taken as a > value to enforce for the whole system and prevent guest attempts to > change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't > looked at whether not exposing the SSBD CPUID related to > SPEC_CTRL.SSBD will have impact on other features). That would be my preference (albeit I'm uncertain about the "not exposing" part, as we don't want to misguide guests into thinking they're unsafe or can't guarantee safety when requested by user mode code), but ... > I was under the impression that the command line ssbd option was added > to cope with Xen not exposing the feature to guests. Now that the > feature is exposed guests should be free to make use of it, and hence > there's no need to force a value for Xen. ... me not having had this understanding may have been wrong on my part. Andrew - any chance you could clarify (original) intentions here? Jan
On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote: > On 13.10.2022 15:50, Roger Pau Monné wrote: > > On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote: > >> On 11.10.2022 18:02, Roger Pau Monne wrote: > >>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the > >>> By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and > >>> when hardware hints recommend using it as a blanket setting. > >>> > >>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` > >>> -option can be used to force or prevent Xen using the feature itself. > >> > >> Why would we want to take away this level of control? Shouldn't we turn this > >> on while in Xen if so requested? Which would then either mean enabling it on > >> VMEXIT if a guest has it off, or running with it turned on using the OR of > >> guest and host settings. > > > > Right, but then we need to context switch the value on vm{entry,exit} > > which is problematic. I could move the context switch code code out > > of the GIF=0 region, and assume that NMIs executing with the guest > > selection of SSBD are OK. > > > > Alternatively setting ssbd= on the command line could be taken as a > > value to enforce for the whole system and prevent guest attempts to > > change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't > > looked at whether not exposing the SSBD CPUID related to > > SPEC_CTRL.SSBD will have impact on other features). > > That would be my preference (albeit I'm uncertain about the "not exposing" > part, as we don't want to misguide guests into thinking they're unsafe or > can't guarantee safety when requested by user mode code), but ... For ssbd=1 we could expose the SSBD controls, as the guest trying to turn it off would have no effect and it would still be protected. OTOH if the user sets ssbd=0 on the command line then exposing the SSBD controls to the guest would be misleading, as the guest setting SSBD will have no effect and thus it won't be protected when it thinks it is. Thanks, Roger.
On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote: > On 13.10.2022 15:50, Roger Pau Monné wrote: > > I was under the impression that the command line ssbd option was added > > to cope with Xen not exposing the feature to guests. Now that the > > feature is exposed guests should be free to make use of it, and hence > > there's no need to force a value for Xen. > > ... me not having had this understanding may have been wrong on my part. > Andrew - any chance you could clarify (original) intentions here? I realized I wasn't taking PV into account, and PV guests on AMD cannot set ssbd, so the option cannot be removed. Thanks, Roger.
On 14.10.2022 10:17, Roger Pau Monné wrote: > On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote: >> On 13.10.2022 15:50, Roger Pau Monné wrote: >>> On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote: >>>> On 11.10.2022 18:02, Roger Pau Monne wrote: >>>>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the >>>>> By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and >>>>> when hardware hints recommend using it as a blanket setting. >>>>> >>>>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` >>>>> -option can be used to force or prevent Xen using the feature itself. >>>> >>>> Why would we want to take away this level of control? Shouldn't we turn this >>>> on while in Xen if so requested? Which would then either mean enabling it on >>>> VMEXIT if a guest has it off, or running with it turned on using the OR of >>>> guest and host settings. >>> >>> Right, but then we need to context switch the value on vm{entry,exit} >>> which is problematic. I could move the context switch code code out >>> of the GIF=0 region, and assume that NMIs executing with the guest >>> selection of SSBD are OK. >>> >>> Alternatively setting ssbd= on the command line could be taken as a >>> value to enforce for the whole system and prevent guest attempts to >>> change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't >>> looked at whether not exposing the SSBD CPUID related to >>> SPEC_CTRL.SSBD will have impact on other features). >> >> That would be my preference (albeit I'm uncertain about the "not exposing" >> part, as we don't want to misguide guests into thinking they're unsafe or >> can't guarantee safety when requested by user mode code), but ... > > For ssbd=1 we could expose the SSBD controls, as the guest trying to > turn it off would have no effect and it would still be protected. > > OTOH if the user sets ssbd=0 on the command line then exposing the > SSBD controls to the guest would be misleading, as the guest setting > SSBD will have no effect and thus it won't be protected when it thinks > it is. Irrespective of your subsequent reply: Unlike "cpuid=no-ssbd", "spec-ctrl=no-ssbd" ought to affect only Xen itself: "On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` option can be used to force or prevent Xen using the feature itself." Jan
On Fri, Oct 14, 2022 at 11:00:10AM +0200, Jan Beulich wrote: > On 14.10.2022 10:17, Roger Pau Monné wrote: > > On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote: > >> On 13.10.2022 15:50, Roger Pau Monné wrote: > >>> On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote: > >>>> On 11.10.2022 18:02, Roger Pau Monne wrote: > >>>>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the > >>>>> By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and > >>>>> when hardware hints recommend using it as a blanket setting. > >>>>> > >>>>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` > >>>>> -option can be used to force or prevent Xen using the feature itself. > >>>> > >>>> Why would we want to take away this level of control? Shouldn't we turn this > >>>> on while in Xen if so requested? Which would then either mean enabling it on > >>>> VMEXIT if a guest has it off, or running with it turned on using the OR of > >>>> guest and host settings. > >>> > >>> Right, but then we need to context switch the value on vm{entry,exit} > >>> which is problematic. I could move the context switch code code out > >>> of the GIF=0 region, and assume that NMIs executing with the guest > >>> selection of SSBD are OK. > >>> > >>> Alternatively setting ssbd= on the command line could be taken as a > >>> value to enforce for the whole system and prevent guest attempts to > >>> change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't > >>> looked at whether not exposing the SSBD CPUID related to > >>> SPEC_CTRL.SSBD will have impact on other features). > >> > >> That would be my preference (albeit I'm uncertain about the "not exposing" > >> part, as we don't want to misguide guests into thinking they're unsafe or > >> can't guarantee safety when requested by user mode code), but ... > > > > For ssbd=1 we could expose the SSBD controls, as the guest trying to > > turn it off would have no effect and it would still be protected. > > > > OTOH if the user sets ssbd=0 on the command line then exposing the > > SSBD controls to the guest would be misleading, as the guest setting > > SSBD will have no effect and thus it won't be protected when it thinks > > it is. > > Irrespective of your subsequent reply: Unlike "cpuid=no-ssbd", > "spec-ctrl=no-ssbd" ought to affect only Xen itself: > > "On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` > option can be used to force or prevent Xen using the feature itself." So that brings us back to having to context switch SSBD on guest entry and exit, and we could only do the SSBD switch at context switch if no ssbd= option is used. That would also prevent us from dropping the synthetic feature leaf. I will wait for Andrews opinion on this one, I would like to make sure we have reached consensus before I send a new version. Thanks, Roger.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 68389843b2..f2666b881a 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2297,7 +2297,7 @@ By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`). ### spec-ctrl (x86) > `= List of [ <bool>, xen=<bool>, {pv,hvm}=<bool>, > {msr-sc,rsb,md-clear,ibpb-entry}=<bool>|{pv,hvm}=<bool>, -> bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,psfd, +> bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,psfd, > eager-fpu,l1d-flush,branch-harden,srb-lock, > unpriv-mmio}=<bool> ]` @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and when hardware hints recommend using it as a blanket setting. -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=` -option can be used to force or prevent Xen using the feature itself. On AMD -hardware, this is a global option applied at boot, and not virtualised for -guest use. On Intel hardware, the feature is virtualised for guests, -independently of Xen's choice of setting. - On hardware supporting PSFD (Predictive Store Forwarding Disable), the `psfd=` option can be used to force or prevent Xen using the feature itself. By default, Xen will not use PSFD. PSFD is implied by SSBD, and SSBD is off by diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index c28f2d5220..cfeb8d1daf 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -730,11 +730,12 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c) } if (cpu_has_virt_ssbd) { - wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0); + /* Handled by context switch logic. */ return; } - if (!set_legacy_ssbd(c, opt_ssbd)) { + /* Test whether legacy SSBD is available. */ + if (!set_legacy_ssbd(c, 0)) { printk_once(XENLOG_ERR "No SSBD controls available\n"); if (amd_legacy_ssbd) panic("CPU feature mismatch: no legacy SSBD\n"); @@ -777,12 +778,8 @@ bool __init amd_setup_legacy_ssbd(void) if (!ssbd_ls_cfg) return false; - for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++) { - /* Set initial state, applies to any (hotplug) CPU. */ - ssbd_ls_cfg[i].count = opt_ssbd ? boot_cpu_data.x86_num_siblings - : 0; + for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++) spin_lock_init(&ssbd_ls_cfg[i].lock); - } return true; } diff --git a/xen/arch/x86/include/asm/spec_ctrl.h b/xen/arch/x86/include/asm/spec_ctrl.h index 9403b81dc7..ee5c7b8d54 100644 --- a/xen/arch/x86/include/asm/spec_ctrl.h +++ b/xen/arch/x86/include/asm/spec_ctrl.h @@ -66,7 +66,6 @@ void init_speculation_mitigations(void); void spec_ctrl_init_domain(struct domain *d); extern int8_t opt_ibpb_ctxt_switch; -extern bool opt_ssbd; extern int8_t opt_eager_fpu; extern int8_t opt_l1d_flush; diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 0b94af6b86..dcee9795a5 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -56,7 +56,6 @@ static enum ind_thunk { static int8_t __initdata opt_ibrs = -1; int8_t __initdata opt_stibp = -1; -bool __ro_after_init opt_ssbd; int8_t __initdata opt_psfd = -1; int8_t __ro_after_init opt_ibpb_ctxt_switch = -1; @@ -126,7 +125,6 @@ static int __init cf_check parse_spec_ctrl(const char *s) opt_thunk = THUNK_JMP; opt_ibrs = 0; opt_ibpb_ctxt_switch = false; - opt_ssbd = false; opt_l1d_flush = 0; opt_branch_harden = false; opt_srb_lock = 0; @@ -263,8 +261,6 @@ static int __init cf_check parse_spec_ctrl(const char *s) opt_ibrs = val; else if ( (val = parse_boolean("stibp", s, ss)) >= 0 ) opt_stibp = val; - else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 ) - opt_ssbd = val; else if ( (val = parse_boolean("psfd", s, ss)) >= 0 ) opt_psfd = val; @@ -471,7 +467,7 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps) "\n"); /* Settings for Xen's protection, irrespective of guests. */ - printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s%s%s, Other:%s%s%s%s%s\n", + printk(" Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s%s, Other:%s%s%s%s%s\n", thunk == THUNK_NONE ? "N/A" : thunk == THUNK_RETPOLINE ? "RETPOLINE" : thunk == THUNK_LFENCE ? "LFENCE" : @@ -482,9 +478,6 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps) (!boot_cpu_has(X86_FEATURE_STIBP) && !boot_cpu_has(X86_FEATURE_AMD_STIBP)) ? "" : (default_xen_spec_ctrl & SPEC_CTRL_STIBP) ? " STIBP+" : " STIBP-", - (!boot_cpu_has(X86_FEATURE_SSBD) && - !boot_cpu_has(X86_FEATURE_AMD_SSBD)) ? "" : - (default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-", (!boot_cpu_has(X86_FEATURE_PSFD) && !boot_cpu_has(X86_FEATURE_INTEL_PSFD)) ? "" : (default_xen_spec_ctrl & SPEC_CTRL_PSFD) ? " PSFD+" : " PSFD-", @@ -1274,16 +1267,6 @@ void __init init_speculation_mitigations(void) boot_cpu_has(X86_FEATURE_AMD_STIBP)) ) default_xen_spec_ctrl |= SPEC_CTRL_STIBP; - if ( opt_ssbd && (boot_cpu_has(X86_FEATURE_SSBD) || - boot_cpu_has(X86_FEATURE_AMD_SSBD)) ) - { - /* SSBD implies PSFD */ - if ( opt_psfd == -1 ) - opt_psfd = 1; - - default_xen_spec_ctrl |= SPEC_CTRL_SSBD; - } - /* * Don't use PSFD by default. AMD designed the predictor to * auto-clear on privilege change. PSFD is implied by SSBD, which is
Like on Intel AMD guests are now capable of setting SSBD on their own, either from SPEC_CTRL or from VIRT_SPEC_CTRL. As a result the unconditional setting of SSBD from Xen in order to cope with the bit not being exposed to guests is no longer needed. Remove the Xen command line `spec-ctrl=ssbd` option and related settings. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- docs/misc/xen-command-line.pandoc | 8 +------- xen/arch/x86/cpu/amd.c | 11 ++++------- xen/arch/x86/include/asm/spec_ctrl.h | 1 - xen/arch/x86/spec_ctrl.c | 19 +------------------ 4 files changed, 6 insertions(+), 33 deletions(-)