[PATCH v2 0/2] crypto: arm/sha-neon - avoid ADRL instructions

[PATCH v2 0/2] crypto: arm/sha-neon - avoid ADRL instructions

[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 <ndesaulniers@google.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Peter Smith <Peter.Smith@arm.com>

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

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

[Ard Biesheuvel]

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 <ardb@kernel.org>
---
 arch/arm/crypto/sha256-armv4.pl       | 4 ++--
 arch/arm/crypto/sha256-core.S_shipped | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/crypto/sha256-armv4.pl b/arch/arm/crypto/sha256-armv4.pl
index 9f96ff48e4a8..f3a2b54efd4e 100644
--- a/arch/arm/crypto/sha256-armv4.pl
+++ b/arch/arm/crypto/sha256-armv4.pl
@@ -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
diff --git a/arch/arm/crypto/sha256-core.S_shipped b/arch/arm/crypto/sha256-core.S_shipped
index ea04b2ab0c33..6363014a50d7 100644
--- a/arch/arm/crypto/sha256-core.S_shipped
+++ b/arch/arm/crypto/sha256-core.S_shipped
@@ -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
-- 
2.17.1

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

[Ard Biesheuvel]

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 <ardb@kernel.org>
---
 arch/arm/crypto/sha512-armv4.pl       | 4 ++--
 arch/arm/crypto/sha512-core.S_shipped | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/crypto/sha512-armv4.pl b/arch/arm/crypto/sha512-armv4.pl
index 69df68981acd..2fc3516912fa 100644
--- a/arch/arm/crypto/sha512-armv4.pl
+++ b/arch/arm/crypto/sha512-armv4.pl
@@ -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:
 ___
diff --git a/arch/arm/crypto/sha512-core.S_shipped b/arch/arm/crypto/sha512-core.S_shipped
index cb147db5cbfe..03014624f2ab 100644
--- a/arch/arm/crypto/sha512-core.S_shipped
+++ b/arch/arm/crypto/sha512-core.S_shipped
@@ -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
-- 
2.17.1

Re: [PATCH v2 0/2] crypto: arm/sha-neon - avoid ADRL instructions

[Nick Desaulniers]

On Tue, Sep 15, 2020 at 11:14 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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 <ndesaulniers@google.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Peter Smith <Peter.Smith@arm.com>

Tested-by: Nick Desaulniers <ndesaulniers@google.com>

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/20200914095706.3985-1-ardb@kernel.org/
3. unrelated fix for -next #1:
https://lore.kernel.org/lkml/20200916200255.1382086-1-ndesaulniers@google.com/
4. unrelated fix for -next #2:
https://lore.kernel.org/linux-mm/20200917001934.2793370-1-ndesaulniers@google.com/
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?

>
> 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
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2] crypto: arm/sha-neon - avoid ADRL instructions

[Ard Biesheuvel]

On Thu, 17 Sep 2020 at 03:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Sep 15, 2020 at 11:14 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > 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 <ndesaulniers@google.com>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Peter Smith <Peter.Smith@arm.com>
>
> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
>

Thanks!

> 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/20200914095706.3985-1-ardb@kernel.org/
> 3. unrelated fix for -next #1:
> https://lore.kernel.org/lkml/20200916200255.1382086-1-ndesaulniers@google.com/
> 4. unrelated fix for -next #2:
> https://lore.kernel.org/linux-mm/20200917001934.2793370-1-ndesaulniers@google.com/
> 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.
>

My pleasure :-)

> 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?
>

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.

Re: [PATCH v2 0/2] crypto: arm/sha-neon - avoid ADRL instructions

[Nick Desaulniers]

On Wed, Sep 16, 2020 at 11:08 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 17 Sep 2020 at 03:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > 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?
> >
>
> 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).
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2] crypto: arm/sha-neon - avoid ADRL instructions

[Ard Biesheuvel]

On Fri, 18 Sep 2020 at 22:08, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Wed, Sep 16, 2020 at 11:08 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 17 Sep 2020 at 03:53, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > 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?
> > >
> >
> > 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).
>

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

Re: [PATCH v2 0/2] crypto: arm/sha-neon - avoid ADRL instructions

[Herbert Xu]

On Wed, Sep 16, 2020 at 09:14:16AM +0300, Ard Biesheuvel wrote:
> 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 <ndesaulniers@google.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Peter Smith <Peter.Smith@arm.com>
> 
> 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(-)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt