[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 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.
+
+ 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.
There seem to be a misunderstanding of the spec. If
ARM_SMCCC_NOT_REQUIRED is returned on one CPU, it will also be returned
for all the others. It means that *all* CPUs have the mitigations
permanently enabled/disabled.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
