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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
