[RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

[Diana Madalina Craciun]

On 05/29/2018 10:16 PM, Scott Wood wrote:
> On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:
>> Hi Scott,
>>
>> Thanks for the review.
>>
>> On 05/22/2018 11:31 PM, Scott Wood wrote:
>>> On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:
>>>> Implement the barrier_nospec as a isync;sync instruction sequence.
>>>> The implementation uses the infrastructure built for BOOK3S 64
>>>> with the difference that for NXP platforms there is no firmware involved
>>>> and the need for a speculation barrier is read from the device tree.
>>>> I have used the same name for the property:
>>>> fsl,needs-spec-barrier-for-bounds-check
>>> Using the device tree this way means that anyone without an updated device
>>> tree won't get the protection.  I also don't see any device tree updates
>>> --
>>> which chips are affected?
>> I was planning to have the device tree changes in a different patch-set.
>> The affected cores are e500, e500mc, e5500, e6500.
> So, all supported FSL/NXP book E chips.  Why not just enable the workaround
> unconditionally (and revisit if NXP ever produces a book E chip that doesn't
> need it and/or e200 is ever supported if that's simple enough to be immune)?

I think it makes sense having in mind that all the NXP book E chips are
vulnerable. e200 is not vulnerable, but it is not properly supported in
the kernel anyway. So I guess I can enable the workaround
unconditionally. I am wondering if it does make sense patching the
instructions at all (instead just use the barrier as an isync; sync
sequence always), but in this case we will loose the possibility of
controlling it via debugfs at runtime.

>
>>> Why patch nops in if not enabled?  Aren't those locations already
>>> nops?  For
>>> that matter, how can this function even be called on FSL_BOOK3E with
>>> enable !=
>>> true?
>> There is some code in arch/powerpc/kernel/security.c which allows
>> control of barrier_nospec via debugfs.
> OK.
>
>>> Should there be a way for the user to choose not to enable this (editing
>>> the
>>> device tree doesn't count), for a use case that is not sufficiently
>>> security
>>> sensitive to justify the performance loss?  What is the performance impact
>>> of
>>> this patch?
>> My reason was that on the other architectures Spectre variant 1
>> mitigations are not disabled either. But I think that it might be a good
>> idea to add a bootarg parameter to disable the barrier.
> Is there a specific policy reason why they allow spectre v2 to be disabled but
> not v1, or just a matter of not having a mechanism to disable it, or the parts
> which could practically be disabled not impacting performance much?

I do not know for sure but I can speculate. The other architectures read
some flags set by the firmware, so I might think that they might use
different versions of firmware if they do not want the mitigations. On
the other hand the other architectures defined special barriers just for
the purpose of preventing speculations which might be more lightweight
and maybe they are not impacting the performance much. But having in
mind that the NXP parts are used in embedded scenarios that might run in
isolation (so no vulnerability) and that the barrier we are using is not
that lightweight I think that we should have a way to disable it.

Diana