[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
(sorry for the formatting)
On Thu, 31 May 2018, Julien Grall wrote:
> Hi,
>
> On 30/05/18 21:10, Stefano Stabellini wrote:
> > 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 >>>>
> > 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.
> A vendor not able to do a simple return "ARM_SMCCC_NOT_SUPPORTED" in their
> firmware are not worth to support it in Xen. Most likely, more important bits
> of that firmware would be broken.
>
> >
> > 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.
>
> I hope you are aware that this will only save 8 byte per-CPU. On most of
> embedded platform you will have less than 16 CPUs. So you would save at most
> 128 bytes (woah!). If you are that tight in memory, then there are better
> place to reduce the footprint.
>
> I am also not sure to understand the problem of having ldr_this_cpu around.
> The macro is simple and in any case, you would still require at least a load
> for the bitmask.
>
> Feel free to suggest an assembly version for the bitmask.
OK, this is very simple, the first that came to mind, I am sure you can
improve it:
// 65 is the cpu number, in this example
MOV X1, #65
// X1 tells us which doubleword to consider
// X2 has the bit shift for right doubleword
// X3 is the shifted X2, we'll use it to check the bitmask
AND X2, X1, #(64-1)
LSR X1, X1, #3
MOV X3, #0x1
LSL X3, X3, X2
// we load the pointer to the bitmask in X4
LDR X4, =cpumask
// increase the pointer to point to the right doubleword
ADD X4, X4, X1
// load the doubleword
LDR X4, [X4]
// mask with X3, the result is in X1
AND X1, X4, X3
Well, because of the SMCC v1.1 convention, you can only use x0-x3. So x4 is a no go.
You also cannot use x1 because it contains the enable/disable boolean.
Furthermore ldr_this_cpu is only 3 instructions. With your current solution, you have 9 instructions. Even optimized, I honestly doubt you will manage 3 instructions. This is 30% more instructions!
So you are maybe going to save few bytes in memory, but everytime you execute the SMC you will lose some time. As this is happening at every entry/exit from EL0, this will have a significant impact on your workload.
At this stage, I will keep with the percpu variable. That's the best trade-off between performance and footprint.
Cheers,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
