Subject: Re: [PATCH] powerpc/lib: Split xor_vmx file to guarantee instruction ordering

From: Michael Ellerman <[email-masked]>
Date: Fri, 26 May 2017 14:12:09 +1000

Matt Brown <[email-masked]> writes:

> On Wed, May 24, 2017 at 11:36 PM, Paul Clarke <[email-masked]> wrote:
>> On 05/23/2017 06:45 PM, Matt Brown wrote:
>>> The xor_vmx.c file is used for the RAID5 xor operations. In these functions
>>> altivec is enabled to run the operation and then disabled. However due to
>>> compiler instruction reordering, altivec instructions are being run before
>>> enable_altivec() and after disable_altivec().
>>
>> If altivec instructions can be reordered after disable_altivec(), then disable_altivec() is broken, I'd think.
>>
>> Could it be because the isync in mtmsr_isync() is after the mtmsr?
>>
>> disable_kernel_altivec
>> - msr_check_and_clear
>>   - __msr_check_and_clear
>>     - mtmsr_isync
>
> So it turns out the enable / disable functions don't actually enable
> or disable the use of vector instructions.
> If we have marked the file to be compiled with altivec the compiler
> has free reign to reorder the vector instructions wherever it likes.
> Including reordering it before or after the enable/disable_altivec
> commands.

That's true. Though it's not that the compiler is reordering vector
instructions, it's that it's generating them directly to save/restore
vector arguments.

In enable_kernel_altivec() we have a mtmsr() which is implemented with
inline asm, with a "memory" clobber, so that is effectively a full
compiler barrier. ie. the compiler will not reorder code across that.

> The enable_kernel_altivec and disable_kernel_altivec functions are
> mainly there to empty and restore the vector registers which could
> have been used in user-space. So those functions work as intended,
> although not particularly intuitive.

Yeah, perhaps "allow" would be better. In fact the whole API is a bit
crap and we need to rework it.

cheers