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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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