[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
Hi Stefano,
On 23/05/18 23:34, 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?
Because I am thinking to extend it and add the possibility to disable
branch predictor hardening. So I decided to get the code and
documentation ready right now.
+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.
It matters, you want to catch and warn user that the command line is not
valid. Imagine someone decide to add ",..." after. It also make easier
to integrate new option without reworking it.
+ 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.
This is what happen when you want to split a series in a logical way.
The name "required" does not make sense with the previous patch. So the
renaming make sense here.
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
The comment is not correct after this patch. The may need to act
differently depending on the comment line. Regarding the values, the
switch is more explanatory than those 3 lines.
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.
Well, that's not going to make the diff simpler here as the switch will
be different. So I would keep the patch like that.
+ {
+ 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".
It is perfectly fine to continue as it is safe to execute
ARCH_WORKAROUND_2 on that CPU.
Also, it might make sense to set ssbd_state
to ARM_SSBD_MITIGATED?
No, the mitigation is not required on *that* CPU. It does not mean it
will not be required for all CPUs. So it makes sense to not update
ssbd_state.
+
+ 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)?
Let me start with because a guest vCPU may run on any pCPU, you always
have to tell the guest the mitigation is required for all vCPUs.
By default, Linux is calling the workaround at entry from EL0 to enable
it and at exit to EL0 to disable it. The workaround will first trap in
EL2 and then get forwarded to EL3.
You can imagine that the trap to EL2 and then EL3 has a cost. If the
workaround is not necessary, then you can reduce that cost by avoiding
to trap at EL3. As you can have a platform with heterogenous CPUs, you
need that workaround per-CPU.
The ARM_SSBD feature bit is useful in order to put shortcut in place
using alternative (see check_workaround_ssbd). So on platform where the
mitigation is not required, all the new code is nearly a NOP.
The ssbd_state is used in various place to know what is the global state
of the mitigation:
- To initialize the vCPU state for the mitigation
- To report the guest what is the state of the mitigation using SMCCC
So all those variables have a specific purposes and cannot really be
replaced by another way.
+ 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?
I had the split before but it is difficult to get a nice split between
.enable and .matches. So I decided to follow what Linux/KVM did and put
everything in has_.
+ 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
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
