[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 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.
+ {
+ 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.
+
+ 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...).
Your suggestion will just save 4 bytes but add more code to find out
what is the system-wide decision for the mitigation.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
