On Sun, Jun 17, 2018 at 01:10:41PM +0200, Ard Biesheuvel wrote:

quoted

quoted

quoted

quoted

quoted

+
+     // One-time XTS preparation
+
+     /*
+      * Allocate stack space to store 128 bytes worth of tweaks.  For
+      * performance, this space is aligned to a 16-byte boundary so that we
+      * can use the load/store instructions that declare 16-byte alignment.
+      */
+     sub             sp, #128
+     bic             sp, #0xf

This fails here when building with CONFIG_THUMB2_KERNEL=y

  AS      arch/arm/crypto/speck-neon-core.o

arch/arm/crypto/speck-neon-core.S: Assembler messages:

arch/arm/crypto/speck-neon-core.S:419: Error: r13 not allowed here --
`bic sp,#0xf'
arch/arm/crypto/speck-neon-core.S:423: Error: r13 not allowed here --
`bic sp,#0xf'
arch/arm/crypto/speck-neon-core.S:427: Error: r13 not allowed here --
`bic sp,#0xf'
arch/arm/crypto/speck-neon-core.S:431: Error: r13 not allowed here --
`bic sp,#0xf'

In a quick hack this change seems to address it:


-       sub             sp, #128
-       bic             sp, #0xf
+       mov             r6, sp
+       sub             r6, #128
+       bic             r6, #0xf
+       mov             sp, r6

But there is probably a better solution to address this.

Given that there is no NEON on M class cores, I recommend we put something like

THUMB(bx pc)
THUMB(nop.w)
THUMB(.arm)

at the beginning and be done with it.

I mean nop.n or just nop, of course, and we may need a '.align 2' at
the beginning as well.

Wouldn't it be preferable to have it assemble it in Thumb2 too? It seems
that bic sp,#0xf is the only issue...

Well, in general, yes. In the case of NEON code, not really, since the
resulting code will not be smaller anyway, because the Thumb2 NEON
opcodes are all 4 bytes. Also, Thumb2-only cores don't have NEON
units, so all cores that this code can run on will be able to run in
ARM mode.

So from a maintainability pov, having code that only assembles in one
way is better than having code that must compile both to ARM and to
Thumb2 opcodes.

Just my 2 cents, anyway.

I don't have too much of a preference, though Stefan's suggested 4 instructions
can be reduced to 3, which also matches what aes-neonbs-core.S does:

        sub             r12, sp, #128
        bic             r12, #0xf
        mov             sp, r12

Ard, is the following what you're suggesting instead?

diff --git a/arch/arm/crypto/speck-neon-core.S b/arch/arm/crypto/speck-neon-core.S
index 3c1e203e53b9..c989ce3dc057 100644
--- a/arch/arm/crypto/speck-neon-core.S
+++ b/arch/arm/crypto/speck-neon-core.S

@@ -8,6 +8,7 @@
  */
 
 #include <linux/linkage.h>
+#include <asm/assembler.h>
 
 	.text
 	.fpu		neon

@@ -233,6 +234,12 @@
  * nonzero multiple of 128.
  */
 .macro _speck_xts_crypt	n, decrypting
+
+	.align		2
+	THUMB(bx pc)
+	THUMB(nop)
+	THUMB(.arm)
+
 	push		{r4-r7}
 	mov		r7, sp
 

@@ -413,6 +420,8 @@
 	mov		sp, r7
 	pop		{r4-r7}
 	bx		lr
+
+	THUMB(.thumb)
 .endm
 
 ENTRY(speck128_xts_encrypt_neon)