(sorry for the formatting) On Thu, 31 May 2018, 22:00 Stefano Stabellini, <sstabell...@kernel.org> wrote:
> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
