powerpc/lib: Split xor_vmx file to guarantee instruction ordering

PATCH Applied Matt Brown 4 participants 6 messages May 23, 2017 – Jun 5, 2017 PowerPC

Share & tools

Matt Brown

author

2017-05-23 23:46 UTC #0 lore raw

PATCH 1/1 powerpc/lib: Split xor_vmx file to guarantee instruction ordering

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().

This patch splits the non-altivec code into xor_vmx_glue.c which calls the altivec functions in xor_vmx.c. By compiling xor_vmx_glue.c without -maltivec we can guarantee that altivec instruction will not be reordered outside of the enable/disable block.

Signed-off-by Matt Brown <[email protected]>

---
 arch/powerpc/lib/Makefile       |  2 +-
 arch/powerpc/lib/xor_vmx.c      | 53 ++++++++---------------------------
 arch/powerpc/lib/xor_vmx.h      | 20 +++++++++++++
 arch/powerpc/lib/xor_vmx_glue.c | 62 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 43 deletions(-)
 create mode 100644 arch/powerpc/lib/xor_vmx.h
 create mode 100644 arch/powerpc/lib/xor_vmx_glue.c

arch/powerpc/lib/Makefile +1 -1

index 309361e8..a448464 100644
@@ -31,7 +31,7 @@ obj-$(CONFIG_PPC_LIB_RHEAP) += rheap.o
 
 obj-$(CONFIG_FTR_FIXUP_SELFTEST) += feature-fixups-test.o
 
-obj-$(CONFIG_ALTIVEC)	+= xor_vmx.o
+obj-$(CONFIG_ALTIVEC)	+= xor_vmx.o xor_vmx_glue.o
 CFLAGS_xor_vmx.o += -maltivec $(call cc-option,-mabi=altivec)
 
 obj-$(CONFIG_PPC64) += $(obj64-y)

arch/powerpc/lib/xor_vmx.c +11 ~~-42~~

116 lines not on this page · raw message

arch/powerpc/lib/xor_vmx.h new file +20 -0

new file mode 100644
index 0000000..4414a24
@@ -0,0 +1,20 @@
+/*
+ * Simple interface to link xor_vmx.c and xor_vmx_glue.c
+ *
+ * Seperating these file ensures that no altivec instructions are run 
+ * outside of the enable/disable altivec block.
+ */
+
+void __xor_altivec_2(unsigned long bytes, unsigned long *v1_in,
+			     unsigned long *v2_in);
+
+void __xor_altivec_3(unsigned long bytes, unsigned long *v1_in,
+			     unsigned long *v2_in, unsigned long *v3_in);
+
+void __xor_altivec_4(unsigned long bytes, unsigned long *v1_in,
+			     unsigned long *v2_in, unsigned long *v3_in,
+			     unsigned long *v4_in);
+
+void __xor_altivec_5(unsigned long bytes, unsigned long *v1_in,
+			     unsigned long *v2_in, unsigned long *v3_in,
+			     unsigned long *v4_in, unsigned long *v5_in);

arch/powerpc/lib/xor_vmx_glue.c new file +62 -0

62 lines not on this page · raw message

Signature · 1 line

2.9.3

Paul Clarke

2017-05-24 13:36 UTC #1 lore raw

Quoted 4 lines · Matt Brown

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

Matt Brown

author

2017-05-25 04:56 UTC #2 lore raw

Quoted 6 lines · Paul Clarke

Quoted 4 lines · Matt Brown
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().

Quoted 8 lines

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.

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.

Splitting the files and only compiling the xor_vmx.c file with altivec will guarantee that there are no vector instructions in the xor_vmx_glue.c file, and that no vector instructions are outside of the enable/disable block.

- Matt Brown

Quoted 3 lines

PC

Paul Clarke

2017-05-25 13:26 UTC #3 lore raw

Quoted 28 lines · Matt Brown

Quoted 6 lines · Paul Clarke
Quoted 4 lines · Matt Brown
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().
Quoted 8 lines
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.
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.

Quoted 4 lines

Splitting the files and only compiling the xor_vmx.c file with altivec will guarantee that there are no vector instructions in the xor_vmx_glue.c file, and that no vector instructions are outside of the enable/disable block.

Thanks for the explanation! That makes sense.

Michael Ellerman

reviewer

2017-05-26 04:12 UTC #4 lore raw

In reply to Matt Brown

Quoted 16 lines · Matt Brown

Quoted 6 lines · Paul Clarke
Quoted 4 lines · Matt Brown
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().
Quoted 8 lines
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

Quoted 6 lines

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.

Quoted 4 lines

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

Michael Ellerman

Applied

2017-06-05 10:21 UTC #5 lore raw

In reply to Matt Brown

Quoted 11 lines · Matt Brown

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().
This patch splits the non-altivec code into xor_vmx_glue.c which calls the altivec functions in xor_vmx.c. By compiling xor_vmx_glue.c without -maltivec we can guarantee that altivec instruction will not be reordered outside of the enable/disable block.
Signed-off-by Matt Brown <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/f718d426d7e42eec6e5d2932f52a51

cheers