crypto: arm/sha-neon - avoid ADRL instructions

PATCH In review Ard Biesheuvel 3 participants 8 messages Sep 16, 2020 – Sep 25, 2020

Subsystems (2)

Arm (32-bit) Crypto API

lore

Share & tools

lore

Ard Biesheuvel

author

2020-09-16 06:14 UTC #0 lore raw

v2 cover crypto: arm/sha-neon - avoid ADRL instructions

Remove some occurrences of ADRL in the SHA NEON code adopted from the OpenSSL project.

I will leave it to the Clang folks to decide whether this needs to be backported and how far, but a Cc stable seems reasonable here.

Cc Nick Desaulniers <[email protected]>

Cc Stefan Agner <[email protected]>

Cc Peter Smith <[email protected]>

Ard Biesheuvel (2):
crypto: arm/sha256-neon - avoid ADRL pseudo instruction
crypto: arm/sha512-neon - avoid ADRL pseudo instruction

 arch/arm/crypto/sha256-armv4.pl       | 4 ++--
 arch/arm/crypto/sha256-core.S_shipped | 4 ++--
 arch/arm/crypto/sha512-armv4.pl       | 4 ++--
 arch/arm/crypto/sha512-core.S_shipped | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

Signature · 1 line

2.17.1

Ard Biesheuvel

author

2020-09-16 06:14 UTC #1 lore raw

PATCH v2 1/2 crypto: arm/sha256-neon - avoid ADRL pseudo instruction

The ADRL pseudo instruction is not an architectural construct, but a convenience macro that was supported by the ARM proprietary assembler and adopted by binutils GAS as well, but only when assembling in 32-bit ARM mode. Therefore, it can only be used in assembler code that is known to assemble in ARM mode only, but as it turns out, the Clang assembler does not implement ADRL at all, and so it is better to get rid of it entirely.

So replace the ADRL instruction with a ADR instruction that refers to a nearer symbol, and apply the delta explicitly using an additional instruction.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/crypto/sha256-armv4.pl       | 4 ++--
 arch/arm/crypto/sha256-core.S_shipped | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

arch/arm/crypto/sha256-armv4.pl +2 -2

index 9f96ff48e4a8..f3a2b54efd4e 100644
@@ -175,7 +175,6 @@ $code=<<___;
 #else
 .syntax unified
 # ifdef __thumb2__
-#  define adrl adr
 .thumb
 # else
 .code   32

@@ -471,7 +470,8 @@ sha256_block_data_order_neon:
 	stmdb	sp!,{r4-r12,lr}
 
 	sub	$H,sp,#16*4+16
-	adrl	$Ktbl,K256
+	adr	$Ktbl,.Lsha256_block_data_order
+	sub	$Ktbl,$Ktbl,#.Lsha256_block_data_order-K256
 	bic	$H,$H,#15		@ align for 128-bit stores
 	mov	$t2,sp
 	mov	sp,$H			@ alloca

arch/arm/crypto/sha256-core.S_shipped +2 -2

index ea04b2ab0c33..6363014a50d7 100644
@@ -56,7 +56,6 @@
 #else
 .syntax unified
 # ifdef __thumb2__
-#  define adrl adr
 .thumb
 # else
 .code   32

@@ -1885,7 +1884,8 @@ sha256_block_data_order_neon:
 	stmdb	sp!,{r4-r12,lr}
 
 	sub	r11,sp,#16*4+16
-	adrl	r14,K256
+	adr	r14,.Lsha256_block_data_order
+	sub	r14,r14,#.Lsha256_block_data_order-K256
 	bic	r11,r11,#15		@ align for 128-bit stores
 	mov	r12,sp
 	mov	sp,r11			@ alloca

Signature · 1 line

2.17.1

Ard Biesheuvel

author

2020-09-16 06:14 UTC #2 lore raw

PATCH v2 2/2 crypto: arm/sha512-neon - avoid ADRL pseudo instruction

In reply to Ard Biesheuvel

So replace the ADRL instruction with a ADR instruction that refers to a nearer symbol, and apply the delta explicitly using an additional instruction.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/crypto/sha512-armv4.pl       | 4 ++--
 arch/arm/crypto/sha512-core.S_shipped | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

arch/arm/crypto/sha512-armv4.pl +2 -2

index 69df68981acd..2fc3516912fa 100644
@@ -212,7 +212,6 @@ $code=<<___;
 #else
 .syntax unified
 # ifdef __thumb2__
-#  define adrl adr
 .thumb
 # else
 .code   32

@@ -602,7 +601,8 @@ sha512_block_data_order_neon:
 	dmb				@ errata #451034 on early Cortex A8
 	add	$len,$inp,$len,lsl#7	@ len to point at the end of inp
 	VFP_ABI_PUSH
-	adrl	$Ktbl,K512
+	adr	$Ktbl,.Lsha512_block_data_order
+	sub	$Ktbl,$Ktbl,.Lsha512_block_data_order-K512
 	vldmia	$ctx,{$A-$H}		@ load context
 .Loop_neon:
 ___

arch/arm/crypto/sha512-core.S_shipped +2 -2

index cb147db5cbfe..03014624f2ab 100644
@@ -79,7 +79,6 @@
 #else
 .syntax unified
 # ifdef __thumb2__
-#  define adrl adr
 .thumb
 # else
 .code   32

@@ -543,7 +542,8 @@ sha512_block_data_order_neon:
 	dmb				@ errata #451034 on early Cortex A8
 	add	r2,r1,r2,lsl#7	@ len to point at the end of inp
 	VFP_ABI_PUSH
-	adrl	r3,K512
+	adr	r3,.Lsha512_block_data_order
+	sub	r3,r3,.Lsha512_block_data_order-K512
 	vldmia	r0,{d16-d23}		@ load context
 .Loop_neon:
 	vshr.u64	d24,d20,#14	@ 0

Signature · 1 line

2.17.1

Nick Desaulniers

Tested-by on the v2 series

2020-09-17 01:01 UTC #3 lore raw

In reply to Ard Biesheuvel

Quoted 10 lines · Ard Biesheuvel

Remove some occurrences of ADRL in the SHA NEON code adopted from the OpenSSL project.
I will leave it to the Clang folks to decide whether this needs to be backported and how far, but a Cc stable seems reasonable here.
Cc Nick Desaulniers <[email protected]>
Cc Stefan Agner <[email protected]>
Cc Peter Smith <[email protected]>

Tested-by Nick Desaulniers <[email protected]>

Thanks Ard:
compile+boot tested each combination of:

[gcc, clang]x[defconfig, defconfig+CONFIG_THUMB2_KERNEL=y].

Now, if I additionally apply:
1. this series
2. the adr_l series:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
3. unrelated fix for -next #1:
https://lore.kernel.org/lkml/[email protected]/
4. unrelated fix for -next #2:
https://lore.kernel.org/linux-mm/[email protected]/
5. small fixup to 01/12 from #2:
https://lore.kernel.org/linux-arm-kernel/CAMj1kXFmF_24d-7W8AWTJR-PCcja7bUdHhYKptGQmsV4vNp=sA@mail.gmail.com/
6. vfp fixup for thumb+gcc:
https://lore.kernel.org/linux-arm-kernel/CAMj1kXHEpPc0MSoMrCxEkyb44AkLM2NJJGtOXLpr6kxBHS0vfA@mail.gmail.com/
7. disable CONFIG_IWMMXT https://github.com/ClangBuiltLinux/linux/issues/975

Then build with Clang's integrated assembler:

$ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 LLVM_IAS=1 -j71

I see a bunch of warnings
(https://github.com/ClangBuiltLinux/linux/issues/858) which we will fix, but I am able to build and boot. (CONFIG_THUMB2_KERNEL=y has many more issues, so I didn't pursue that further).

Either way, these two adrl patches go a long way towards getting Clang to assemble an ARCH=arm kernel; thank you for all of the work that went into them.

One thing I noticed was that if I grep for adrl with all of the above applied within arch/arm, I do still see two more instances:

crypto/sha256-armv4.pl
609: adrl $Ktbl,K256

crypto/sha256-core.S_shipped
2679: adrl r3,K256

Maybe those can be fixed up in patch 01/02 of this series for a v2? I guess in this cover letter, you did specify *some occurrences of ADRL*. It looks like those are guarded by
605 # ifdef __thumb2__
...
608 # else
609 adrl $Ktbl,K256

So are these always built as thumb2?

Quoted 14 lines

Ard Biesheuvel (2):
crypto: arm/sha256-neon - avoid ADRL pseudo instruction
crypto: arm/sha512-neon - avoid ADRL pseudo instruction
 arch/arm/crypto/sha256-armv4.pl       | 4 ++--
 arch/arm/crypto/sha256-core.S_shipped | 4 ++--
 arch/arm/crypto/sha512-armv4.pl       | 4 ++--
 arch/arm/crypto/sha512-core.S_shipped | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)
--
2.17.1

Signature · 2 lines

Thanks,
~Nick Desaulniers

Ard Biesheuvel

author

2020-09-17 06:15 UTC #4 lore raw

Quoted 6 lines · Nick Desaulniers

Quoted 4 lines · Ard Biesheuvel
Remove some occurrences of ADRL in the SHA NEON code adopted from the OpenSSL project.

Quoted 9 lines

Quoted 6 lines
I will leave it to the Clang folks to decide whether this needs to be backported and how far, but a Cc stable seems reasonable here.
Cc Nick Desaulniers <[email protected]>
Cc Stefan Agner <[email protected]>
Cc Peter Smith <[email protected]>
Tested-by Nick Desaulniers <[email protected]>

Thanks!

Quoted 22 lines

Thanks Ard:
compile+boot tested each combination of:
[gcc, clang]x[defconfig, defconfig+CONFIG_THUMB2_KERNEL=y].
Now, if I additionally apply:
1. this series
2. the adr_l series:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
3. unrelated fix for -next #1:
https://lore.kernel.org/lkml/[email protected]/
4. unrelated fix for -next #2:
https://lore.kernel.org/linux-mm/[email protected]/
5. small fixup to 01/12 from #2:
https://lore.kernel.org/linux-arm-kernel/CAMj1kXFmF_24d-7W8AWTJR-PCcja7bUdHhYKptGQmsV4vNp=sA@mail.gmail.com/
6. vfp fixup for thumb+gcc:
https://lore.kernel.org/linux-arm-kernel/CAMj1kXHEpPc0MSoMrCxEkyb44AkLM2NJJGtOXLpr6kxBHS0vfA@mail.gmail.com/
7. disable CONFIG_IWMMXT https://github.com/ClangBuiltLinux/linux/issues/975
Then build with Clang's integrated assembler:
$ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 LLVM_IAS=1 -j71

Quoted 9 lines

I see a bunch of warnings
(https://github.com/ClangBuiltLinux/linux/issues/858) which we will fix, but I am able to build and boot. (CONFIG_THUMB2_KERNEL=y has many more issues, so I didn't pursue that further).
Either way, these two adrl patches go a long way towards getting Clang to assemble an ARCH=arm kernel; thank you for all of the work that went into them.

My pleasure :-)

Quoted 17 lines

One thing I noticed was that if I grep for adrl with all of the above applied within arch/arm, I do still see two more instances:
crypto/sha256-armv4.pl
609: adrl $Ktbl,K256
crypto/sha256-core.S_shipped
2679: adrl r3,K256
Maybe those can be fixed up in patch 01/02 of this series for a v2? I guess in this cover letter, you did specify *some occurrences of ADRL*. It looks like those are guarded by
605 # ifdef __thumb2__
...
608 # else
609 adrl $Ktbl,K256

Quoted 2 lines

So are these always built as thumb2?

No need. The code in question is never assembled when built as part of the kernel, only when building OpenSSL for user space. It appears upstream has removed this already, but they have also been playing weird games with the license blocks, so I'd prefer fixing the code here rather than pulling the latest version.

Nick Desaulniers

2020-09-18 20:08 UTC #5 lore raw

Quoted 20 lines · Ard Biesheuvel

Quoted 18 lines · Nick Desaulniers
One thing I noticed was that if I grep for adrl with all of the above applied within arch/arm, I do still see two more instances:
crypto/sha256-armv4.pl
609: adrl $Ktbl,K256
crypto/sha256-core.S_shipped
2679: adrl r3,K256
Maybe those can be fixed up in patch 01/02 of this series for a v2? I guess in this cover letter, you did specify *some occurrences of ADRL*. It looks like those are guarded by
605 # ifdef __thumb2__
...
608 # else
609 adrl $Ktbl,K256

Quoted 8 lines

Quoted 2 lines
So are these always built as thumb2?
No need. The code in question is never assembled when built as part of the kernel, only when building OpenSSL for user space. It appears upstream has removed this already, but they have also been playing weird games with the license blocks, so I'd prefer fixing the code here rather than pulling the latest version.

Oh, like mixing and matching licenses throughout the source itself? Or changing the source license?

(I've always wondered if software licenses apply to an entire repository, or were per source file? Could you mix and match licenses throughout your project? Not sure why you'd do that; maybe to make some parts reusable for some other project. But if you could, could you do different sections of a file under different licenses? Again, probably a worthless hypothetical; you could just split up your source files better).

Signature · 2 lines

Thanks,
~Nick Desaulniers

Ard Biesheuvel

author

2020-09-18 20:30 UTC #6 lore raw

Quoted 34 lines · Nick Desaulniers

Quoted 20 lines · Ard Biesheuvel
Quoted 18 lines · Nick Desaulniers
One thing I noticed was that if I grep for adrl with all of the above applied within arch/arm, I do still see two more instances:
crypto/sha256-armv4.pl
609: adrl $Ktbl,K256
crypto/sha256-core.S_shipped
2679: adrl r3,K256
Maybe those can be fixed up in patch 01/02 of this series for a v2? I guess in this cover letter, you did specify *some occurrences of ADRL*. It looks like those are guarded by
605 # ifdef __thumb2__
...
608 # else
609 adrl $Ktbl,K256
Quoted 8 lines
Quoted 2 lines
So are these always built as thumb2?
No need. The code in question is never assembled when built as part of the kernel, only when building OpenSSL for user space. It appears upstream has removed this already, but they have also been playing weird games with the license blocks, so I'd prefer fixing the code here rather than pulling the latest version.
Oh, like mixing and matching licenses throughout the source itself? Or changing the source license?

Quoted 8 lines

(I've always wondered if software licenses apply to an entire repository, or were per source file? Could you mix and match licenses throughout your project? Not sure why you'd do that; maybe to make some parts reusable for some other project. But if you could, could you do different sections of a file under different licenses? Again, probably a worthless hypothetical; you could just split up your source files better).

https://github.com/openssl/openssl/blob/master/crypto/sha/asm/sha256-armv4.pl

Herbert Xu

maintainer

2020-09-25 08:11 UTC #7 lore raw

In reply to Ard Biesheuvel

Quoted 14 lines · Ard Biesheuvel

Remove some occurrences of ADRL in the SHA NEON code adopted from the OpenSSL project.
I will leave it to the Clang folks to decide whether this needs to be backported and how far, but a Cc stable seems reasonable here.
Cc Nick Desaulniers <[email protected]>
Cc Stefan Agner <[email protected]>
Cc Peter Smith <[email protected]>
Ard Biesheuvel (2):
crypto: arm/sha256-neon - avoid ADRL pseudo instruction
crypto: arm/sha512-neon - avoid ADRL pseudo instruction

Quoted 5 lines

 arch/arm/crypto/sha256-armv4.pl       | 4 ++--
 arch/arm/crypto/sha256-core.S_shipped | 4 ++--
 arch/arm/crypto/sha512-armv4.pl       | 4 ++--
 arch/arm/crypto/sha512-core.S_shipped | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

All applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt