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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 30 May 2018, Julien Grall wrote:
> On 05/29/2018 11:34 PM, Stefano Stabellini wrote:
> > 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".
> 
> I don't think it is right ot expect the same behavior for "1"and
> "ARM_SMCCC_NOT_REQUIRED". There are majors difference between those 2 and
> ARM_SMCCC_NOT_SUPPORTED.
> 
> From the spec ARM DEN 0070A section 2.2.5:
>       - If your firmware does not support the call, ARM_SMCCC_NOT_SUPPORTED
> will be returned for *all* CPUs. This will happen on current firmwares.
>       - If your firmware has mitigation permanently disabled/enabled for
> *all* CPUs, ARM_SMCCC_NOT_REQUIRED will be returned.
>       - If one of the CPUs in the platform require dynamic mitigation, the
> call will return 0 for them. The others CPUs will return 1.
> 
> Printing a warning for the first two will likely scare the user because it is
> going to be printed on most of the current platforms.
> 
> Printing a warning for the last one makes sense because you know that one of
> the CPU may be affected. That CPU may be bring up later on. So you offer a
> print in similar place in the logs whatever the platform is.

I should have read the spec more carefully, thanks for the pointer.
Sorry about that. Finally, these patches are starting to make sense :-)

All right. I can see why ssbd_state and ssbd_callback_required are
separate and their purpose. Aside from adding more info to the commit
message, I'll make a couple of different suggestions:

1) Let's check if ssbd_state == ARM_SSBD_UNKNOWN || ssbd_state ==
ARM_SSBD_MITIGATED at the beginning of has_ssbd_mitigation and return
early in that case. This will help clarify the intended behavior and
mitigate broken firmware returning ARM_SMCCC_NOT_SUPPORTED only on some
cpus. This is just optional, I am fine either way.

2) Can we turn ssbd_callback_required from a this_cpu variable to a
single cpu bitmask? It is not great to introduce a new per-cpu varible
for just one bit. It would save space and make it easier to access from
assembly as a bitmask as it would remove the need for the ldr_this_cpu
macro. If I am wrong and the bitmask makes things more complicated
rather than simpler, then keep the code as is and just mention it in the
next version of the patch.

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