Re: [PATCH 05/13] xen/arm: Add command line option to control SSBD mitigation

[Stefano Stabellini <sstabellini@kernel.org>]

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@arm.com>
> > ---
> >  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel