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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 29 May 2018, Julien Grall wrote:
> On 25/05/18 21:51, Stefano Stabellini wrote:
> > On Thu, 24 May 2018, Julien Grall wrote:
> > > On 23/05/18 23:34, Stefano Stabellini wrote:
> > > > On Tue, 22 May 2018, Julien Grall  >>>>
> > > > 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.
> 
> I don't think the other way will help. But I will do it.

Thank you


> > > > 
> > > > > +    {
> > > > > +    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?
> 
> While the SMCCC call in 1) and 3) will do nothing for those CPUs, you will
> still print a warning message if the user choose to force enable/disable the
> mitigation.

Printing warnings could be a good idea. However, I think we should do
the same thing for "1" and for "ARM_SMCCC_NOT_REQUIRED", and maybe even
for "ARM_SMCCC_NOT_SUPPORTED": printing warnings for all or for none.

I also noticed that if the first SMCCC returns "1" and we continue, in
case ssbd_state == ARM_SSBD_FORCE_ENABLE, "required" gets changed to
"true".  Do we want to let the user force-enable the mitigation even
when it will do nothing? I am not really sure, probably not? In any case
I would prefer if we kept the same behavior across "1" and
"ARM_SMCCC_NOT_REQUIRED".


> > > > 
> > > > 
> > > > > +
> > > > > +    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.
> 
> As I said in my previous e-mail, we need to know the global state of the
> mitigation. This is because a vCPU may move from a affected CPU to a
> non-affected one. Therefore we need to inform the same on every vCPU (i.e
> mitigated, dynamic...).

All right, but if the SMCCC(ARM_SMCCC_ARCH_FEATURES_FID,
ARM_SMCCC_ARCH_WORKAROUND_2_FID) returns ARM_SMCCC_SUCCESS on cpu0 and
ARM_SMCCC_NOT_REQUIRED on cpu1, the result will be that ssbd_state is
set to ARM_SSBD_MITIGATED for all cpus. Which is not what we want?
It doesn't look like the vsmc would return the right value anymore.

One solution would be that has_ssbd_mitigation is not allowed to set
ssbd_state to ARM_SSBD_UNKNOWN or ARM_SSBD_MITIGATED if previously, or
afterwards, the SMCCC returns ARM_SMCCC_SUCCESS. In other words,
ARM_SMCCC_SUCCESS trumps any other return values.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel