|
[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 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)?
> + 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 |