[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 05/13] xen/arm: Add command line option to control SSBD mitigation
On Wed, 23 May 2018, Stefano Stabellini wrote: > On Tue, 22 May 2018, Julien Grall wrote: > > On a system where the firmware implements ARCH_WORKAROUND_2, it may be > > useful to either permanently enable or disable the workaround for cases > > where the user decides that they'd rather not get a trap overhead, and > > keep the mitigation permanently on or off instead of switching it on > > exception entry/exit. > > > > In any case, default to mitigation being enabled. > > > > At the same time provide a accessor to know the state of the mitigation. > > > > SIgned-off-by: Julien Grall <julien.grall@xxxxxxx> > > --- > > docs/misc/xen-command-line.markdown | 18 ++++++ > > xen/arch/arm/cpuerrata.c | 115 > > ++++++++++++++++++++++++++++++++---- > > xen/include/asm-arm/cpuerrata.h | 21 +++++++ > > xen/include/asm-arm/smccc.h | 1 + > > 4 files changed, 144 insertions(+), 11 deletions(-) > > > > diff --git a/docs/misc/xen-command-line.markdown > > b/docs/misc/xen-command-line.markdown > > index 8712a833a2..962028b6ed 100644 > > --- a/docs/misc/xen-command-line.markdown > > +++ b/docs/misc/xen-command-line.markdown > > @@ -1756,6 +1756,24 @@ enforces the maximum theoretically necessary timeout > > of 670ms. Any number > > is being interpreted as a custom timeout in milliseconds. Zero or boolean > > false disable the quirk workaround, which is also the default. > > > > +### spec-ctrl (Arm) > > +> `= List of [ ssbd=force-disable|runtime|force-enable ]` > > Why a list? Shouldn't it be one or the other? > > > +Controls for speculative execution sidechannel mitigations. > > + > > +The option `ssbd=` is used to control the state of Speculative Store > > +Bypass Disable (SSBD) mitigation. > > + > > +* `ssbd=force-disable` will keep the mitigation permanently off. The guest > > +will not be able to control the state of the mitigation. > > +* `ssbd=runtime` will always turn on the mitigation when running in the > > +hypervisor context. The guest will be to turn on/off the mitigation for > > +itself by using the firmware interface ARCH\_WORKAROUND\_2. > > +* `ssbd=force-enable` will keep the mitigation permanently on. The guest > > will > > +not be able to control the state of the mitigation. > > + > > +By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`). > > + > > ### spec-ctrl (x86) > > > `= List of [ <bool>, xen=<bool>, {pv,hvm,msr-sc,rsb}=<bool>, > > > bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd}=<bool> ]` > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c > > index bcea2eb6e5..f921721a66 100644 > > --- a/xen/arch/arm/cpuerrata.c > > +++ b/xen/arch/arm/cpuerrata.c > > @@ -237,6 +237,41 @@ static int enable_ic_inv_hardening(void *data) > > > > #ifdef CONFIG_ARM_SSBD > > > > +enum ssbd_state ssbd_state = ARM_SSBD_RUNTIME; > > + > > +static int __init parse_spec_ctrl(const char *s) > > +{ > > + const char *ss; > > + int rc = 0; > > + > > + do { > > + ss = strchr(s, ','); > > + if ( !ss ) > > + ss = strchr(s, '\0'); > > It doesn't look like it is necessary to parse ',' at all. I would remove > the while loop too. > > > > + if ( !strncmp(s, "ssbd=", 5) ) > > + { > > + s += 5; > > + > > + if ( !strncmp(s, "force-disable", ss - s) ) > > + ssbd_state = ARM_SSBD_FORCE_DISABLE; > > + else if ( !strncmp(s, "runtime", ss - s) ) > > + ssbd_state = ARM_SSBD_RUNTIME; > > + else if ( !strncmp(s, "force-enable", ss - s) ) > > + ssbd_state = ARM_SSBD_FORCE_ENABLE; > > + else > > + rc = -EINVAL; > > + } > > + else > > + rc = -EINVAL; > > + > > + s = ss + 1; > > + } while ( *ss ); > > + > > + return rc; > > +} > > +custom_param("spec-ctrl", parse_spec_ctrl); > > + > > /* > > * Assembly code may use the variable directly, so we need to make sure > > * it fits in a register. > > @@ -246,25 +281,82 @@ DEFINE_PER_CPU_READ_MOSTLY(register_t, > > ssbd_callback_required); > > static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry) > > { > > struct arm_smccc_res res; > > - bool supported = true; > > + bool required = true; > > Please avoid this renaming. Choose one name or the other from the start. > > > > if ( smccc_ver < SMCCC_VERSION(1, 1) ) > > return false; > > > > - /* > > - * The probe function return value is either negative (unsupported > > - * or mitigated), positive (unaffected), or zero (requires > > - * mitigation). We only need to do anything in the last case. > > - */ > > I would keep the comment > > > > arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID, > > ARM_SMCCC_ARCH_WORKAROUND_2_FID, &res); > > - if ( (int)res.a0 != 0 ) > > - supported = false; > > > > - if ( supported ) > > - this_cpu(ssbd_callback_required) = 1; > > + switch ( (int)res.a0 ) > > Please introduce this switch in the previous patch. But it makes sense > to add the ssbd_state variable in this patch. > > > > + { > > + case ARM_SMCCC_NOT_SUPPORTED: > > + ssbd_state = ARM_SSBD_UNKNOWN; > > + return false; > > + > > + case ARM_SMCCC_NOT_REQUIRED: > > + ssbd_state = ARM_SSBD_MITIGATED; > > + return false; > > + > > + case ARM_SMCCC_SUCCESS: > > + required = true; > > + break; > > + > > + case 1: /* Mitigation not required on this CPU. */ > > + required = false; > > + break; > > This should "return false". Also, it might make sense to set ssbd_state > to ARM_SSBD_MITIGATED? > > > > + > > + default: > > + ASSERT_UNREACHABLE(); > > + return false; > > + } > > + > > + switch ( ssbd_state ) > > + { > > + case ARM_SSBD_FORCE_DISABLE: > > + { > > + static bool once = true; > > + > > + if ( once ) > > + printk("%s disabled from command-line\n", entry->desc); > > + once = false; > > + > > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL); > > + required = false; > > + > > + break; > > + } > > + > > + case ARM_SSBD_RUNTIME: > > + if ( required ) > > + { > > + this_cpu(ssbd_callback_required) = 1; > > We have the ARM_SSBD bit, the ssbd_state variable and > ssbd_callback_required. Both ARM_SSBD and ssbd_state are shared across > cores while ssbd_callback_required is per-cpu. Does > ssbd_callback_required really need to be per-cpu? Do we need both > variables? For instance, we could just return ssbd_state == > ARM_SSBD_RUNTIME instead of this_cpu(ssbd_callback_required)? After reading the whole series, I think ssbd_state should be a per_cpu variable. parse_spec_ctrl initializes ssbd_state to the same value on all cpus. has_ssbd_mitigation modifies ssbd_state only on the CPUs it is running on. We get rid of ssbd_callback_required. The assembly fast past reads ssbd_state instead of ssbd_callback_required. What do you think? > > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > > + } > > + > > + break; > > + > > + case ARM_SSBD_FORCE_ENABLE: > > + { > > + static bool once = true; > > + > > + if ( once ) > > + printk("%s forced from command-line\n", entry->desc); > > + once = false; > > + > > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > > + required = true; > > This function is supposed to detect whether a workaround is needed, not > enable it, right? Should this switch and relative code be moved to the > .enable function for this capability? > > > > + break; > > + } > > + > > + default: > > + ASSERT_UNREACHABLE(); > > + return false; > > + } > > > > - return supported; > > + return required; > > } > > #endif > > > > @@ -371,6 +463,7 @@ static const struct arm_cpu_capabilities arm_errata[] = > > { > > #endif > > #ifdef CONFIG_ARM_SSBD > > { > > + .desc = "Speculative Store Bypass Disabled", > > .capability = ARM_SSBD, > > .matches = has_ssbd_mitigation, > > }, > > diff --git a/xen/include/asm-arm/cpuerrata.h > > b/xen/include/asm-arm/cpuerrata.h > > index e628d3ff56..7fbb3dc0be 100644 > > --- a/xen/include/asm-arm/cpuerrata.h > > +++ b/xen/include/asm-arm/cpuerrata.h > > @@ -31,10 +31,26 @@ CHECK_WORKAROUND_HELPER(ssbd, ARM_SSBD, CONFIG_ARM_SSBD) > > > > #undef CHECK_WORKAROUND_HELPER > > > > +enum ssbd_state > > +{ > > + ARM_SSBD_UNKNOWN, > > + ARM_SSBD_FORCE_DISABLE, > > + ARM_SSBD_RUNTIME, > > + ARM_SSBD_FORCE_ENABLE, > > + ARM_SSBD_MITIGATED, > > +}; > > + > > #ifdef CONFIG_ARM_SSBD > > > > #include <asm/current.h> > > > > +extern enum ssbd_state ssbd_state; > > + > > +static inline enum ssbd_state get_ssbd_state(void) > > +{ > > + return ssbd_state; > > +} > > + > > DECLARE_PER_CPU(register_t, ssbd_callback_required); > > > > static inline bool cpu_require_ssbd_mitigation(void) > > @@ -49,6 +65,11 @@ static inline bool cpu_require_ssbd_mitigation(void) > > return false; > > } > > > > +static inline enum ssbd_state get_sbdd_state(void) > > +{ > > + return ARM_SSBD_UNKNOWN; > > +} > > + > > #endif > > > > #endif /* __ARM_CPUERRATA_H__ */ > > diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h > > index 650744d28b..a6804cec99 100644 > > --- a/xen/include/asm-arm/smccc.h > > +++ b/xen/include/asm-arm/smccc.h > > @@ -265,6 +265,7 @@ struct arm_smccc_res { > > 0x7FFF) > > > > /* SMCCC error codes */ > > +#define ARM_SMCCC_NOT_REQUIRED (-2) > > #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1) > > #define ARM_SMCCC_NOT_SUPPORTED (-1) > > #define ARM_SMCCC_SUCCESS (0) > > -- > > 2.11.0 > > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |