|
[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 Thu, 24 May 2018, Julien Grall wrote:
> 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.
OK, maybe it would be good to explain this in the commit message but it
is not necessary.
> > > +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.
All right, I re-read the whole loop and looks fine to me.
> >
> >
> > > + 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.
Why does "required" not make sense in the previous patch? I think it
would be fine.
In any case, I prefer that both of us spend time on useful stuff rather
than choosing which patch should set the variable name :-) So I am going
to drop this regardless.
> >
> >
> > > 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.
I see. You could keep the comment, removing "We only need to do anything
in the last case." Either way is OK.
> >
> >
> > > 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.
The split is a bit iffy to me, but if you don't want to change it, I can
live with it anyway.
> >
> > > + {
> > > + 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.
This is the case where mitigation is not required but issuing the SMCCC
is safe. Instead of returning immediately, we go through the next
switch:
1) if ARM_SSBD_FORCE_DISABLE, we make the SMCCC
2) if ARM_SSBD_RUNTIME, we do nothing
3) if ARM_SSBD_FORCE_ENABLE, we make the SMCCC
What is the desired outcome for this situation? Obviously, continuing for
case 2) is pointless, we might as well return immediately. For 1) and 3)
is the intention that the SMCCC will actually have an effect even if the
mitigation is not required?
> 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.
I understand now, and thanks for the clarification on the call as well.
> >
> >
> > > +
> > > + 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.
Good explanation. Please add something like this to one of the commit
messages. Please also consider the following suggestion.
Wouldn't it make sense to remove ssbd_callback_required and make
ssbd_state a per-cpu variable? The Xen command line option would remain
the same, global, but it would initialize the value of ssbd_state on all
cpus. Then, has_ssbd_mitigation would further modify ssbd_state on a
specific cpu to ARM_SSBD_UNKNOWN (if ARM_SMCCC_NOT_SUPPORTED),
ARM_SSBD_MITIGATED (if ARM_SMCCC_NOT_REQUIRED"), etc. In the common
case, the CPUs that need the workaround will have ssbd_state set to
ARM_SSBD_RUNTIME, and the others will have ARM_SSBD_UNKNOWN or
ARM_SSBD_MITIGATED, or maybe a new value ARM_SSBD_UNNECESSARY. It looks
like it would still be simple to check on ssbd_state from assembly as
well, it can still be done with one instruction, we just need to make
sure to assign integer values to the enum, such as:
ARM_SSBD_UNKNOWN = 0,
ARM_SSBD_FORCE_DISABLE = 1,
etc.
> > > + 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_.
All right. Please add a note about this in the commit message or the code.
> >
> > > + 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |