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

[Julien Grall <julien.grall@xxxxxxx>]

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.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel