diff mbox series

crypto: aegis128/simd - build 32-bit ARM for v8 architecture explicitly

Message ID 20191002075448.6453-1-ard.biesheuvel@linaro.org
State New
Headers show
Series crypto: aegis128/simd - build 32-bit ARM for v8 architecture explicitly | expand

Commit Message

Ard Biesheuvel Oct. 2, 2019, 7:54 a.m. UTC 
Now that the Clang compiler has taken it upon itself to police the
compiler command line, and reject combinations for arguments it views
as incompatible, the AEGIS128 no longer builds correctly, and errors
out like this:

  clang-10: warning: ignoring extension 'crypto' because the 'armv7-a'
  architecture does not support it [-Winvalid-command-line-argument]

So let's switch to armv8-a instead, which matches the crypto-neon-fp-armv8
FPU profile we specify. Since neither were actually supported by GCC
versions before 4.8, let's tighten the Kconfig dependencies as well so
we won't run into errors when building with an ancient compiler.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 crypto/Kconfig  | 1 +
 crypto/Makefile | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.20.1

Comments

Nathan Chancellor Oct. 2, 2019, 2:47 p.m. UTC | #1 
Thank you for fixing this, it is much appreciated!

On Wed, Oct 02, 2019 at 09:54:48AM +0200, Ard Biesheuvel wrote:
> Now that the Clang compiler has taken it upon itself to police the

> compiler command line, and reject combinations for arguments it views

> as incompatible, the AEGIS128 no longer builds correctly, and errors

> out like this:

> 

>   clang-10: warning: ignoring extension 'crypto' because the 'armv7-a'

>   architecture does not support it [-Winvalid-command-line-argument]

> 

> So let's switch to armv8-a instead, which matches the crypto-neon-fp-armv8

> FPU profile we specify. Since neither were actually supported by GCC

> versions before 4.8, let's tighten the Kconfig dependencies as well so

> we won't run into errors when building with an ancient compiler.

> 

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Minor nit below but regardless.

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

Tested-by: Nathan Chancellor <natechancellor@gmail.com>


> ---

>  crypto/Kconfig  | 1 +

>  crypto/Makefile | 2 +-

>  2 files changed, 2 insertions(+), 1 deletion(-)

> 

> diff --git a/crypto/Kconfig b/crypto/Kconfig

> index e928f88b6206..b138b68329dc 100644

> --- a/crypto/Kconfig

> +++ b/crypto/Kconfig

> @@ -331,6 +331,7 @@ config CRYPTO_AEGIS128

>  config CRYPTO_AEGIS128_SIMD

>  	bool "Support SIMD acceleration for AEGIS-128"

>  	depends on CRYPTO_AEGIS128 && ((ARM || ARM64) && KERNEL_MODE_NEON)

> +	depends on !ARM || CC_IS_CLANG || GCC_VERSION >= 40800


This !ARM is a bit misleading to me given the (ARM || ARM64) requirement
right above it and the fact that crypto/Makefile gates the addition of
the NEON files to aegis128-y to $(ARCH) = arm or arm64.

>  	default y

>  

>  config CRYPTO_AEGIS128_AESNI_SSE2

> diff --git a/crypto/Makefile b/crypto/Makefile

> index fcb1ee679782..aa740c8492b9 100644

> --- a/crypto/Makefile

> +++ b/crypto/Makefile

> @@ -93,7 +93,7 @@ obj-$(CONFIG_CRYPTO_AEGIS128) += aegis128.o

>  aegis128-y := aegis128-core.o

>  

>  ifeq ($(ARCH),arm)

> -CFLAGS_aegis128-neon-inner.o += -ffreestanding -march=armv7-a -mfloat-abi=softfp

> +CFLAGS_aegis128-neon-inner.o += -ffreestanding -march=armv8-a -mfloat-abi=softfp

>  CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8

>  aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o

>  endif

> -- 

> 2.20.1

>
Nick Desaulniers Oct. 2, 2019, 4:47 p.m. UTC | #2 
On Wed, Oct 2, 2019 at 12:55 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>

> Now that the Clang compiler has taken it upon itself to police the

> compiler command line, and reject combinations for arguments it views

> as incompatible, the AEGIS128 no longer builds correctly, and errors

> out like this:

>

>   clang-10: warning: ignoring extension 'crypto' because the 'armv7-a'

>   architecture does not support it [-Winvalid-command-line-argument]

>

> So let's switch to armv8-a instead, which matches the crypto-neon-fp-armv8

> FPU profile we specify. Since neither were actually supported by GCC

> versions before 4.8, let's tighten the Kconfig dependencies as well so

> we won't run into errors when building with an ancient compiler.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Thank you Ard, this fixes the build error for us.  Do you know if the
"crypto extensions" are mandatory ISA extensions?  I'm running into
some inconsistencies between how clang parses target arch between
command line flag, function __attribute__, assembler directive, and
disassembler.  I see arch's like: armv8-a+crc, armv8-a+sve,
armv8-a+fp16, armv8-a+memtag, armv8-a+lse, but I'm not familiar with
the `+...` part of the target arch.

Either way, thanks for the patch:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

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

Reported-by: <ci_notify@linaro.org>
Link: https://github.com/ClangBuiltLinux/linux/issues/730

> ---

>  crypto/Kconfig  | 1 +

>  crypto/Makefile | 2 +-

>  2 files changed, 2 insertions(+), 1 deletion(-)

>

> diff --git a/crypto/Kconfig b/crypto/Kconfig

> index e928f88b6206..b138b68329dc 100644

> --- a/crypto/Kconfig

> +++ b/crypto/Kconfig

> @@ -331,6 +331,7 @@ config CRYPTO_AEGIS128

>  config CRYPTO_AEGIS128_SIMD

>         bool "Support SIMD acceleration for AEGIS-128"

>         depends on CRYPTO_AEGIS128 && ((ARM || ARM64) && KERNEL_MODE_NEON)

> +       depends on !ARM || CC_IS_CLANG || GCC_VERSION >= 40800

>         default y

>

>  config CRYPTO_AEGIS128_AESNI_SSE2

> diff --git a/crypto/Makefile b/crypto/Makefile

> index fcb1ee679782..aa740c8492b9 100644

> --- a/crypto/Makefile

> +++ b/crypto/Makefile

> @@ -93,7 +93,7 @@ obj-$(CONFIG_CRYPTO_AEGIS128) += aegis128.o

>  aegis128-y := aegis128-core.o

>

>  ifeq ($(ARCH),arm)

> -CFLAGS_aegis128-neon-inner.o += -ffreestanding -march=armv7-a -mfloat-abi=softfp

> +CFLAGS_aegis128-neon-inner.o += -ffreestanding -march=armv8-a -mfloat-abi=softfp

>  CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8

>  aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o

>  endif

> --

> 2.20.1

>



--
Thanks,
~Nick Desaulniers
Catalin Marinas Oct. 2, 2019, 5:23 p.m. UTC | #3 
On Wed, Oct 02, 2019 at 09:47:41AM -0700, Nick Desaulniers wrote:
> On Wed, Oct 2, 2019 at 12:55 AM Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

> >

> > Now that the Clang compiler has taken it upon itself to police the

> > compiler command line, and reject combinations for arguments it views

> > as incompatible, the AEGIS128 no longer builds correctly, and errors

> > out like this:

> >

> >   clang-10: warning: ignoring extension 'crypto' because the 'armv7-a'

> >   architecture does not support it [-Winvalid-command-line-argument]

> >

> > So let's switch to armv8-a instead, which matches the crypto-neon-fp-armv8

> > FPU profile we specify. Since neither were actually supported by GCC

> > versions before 4.8, let's tighten the Kconfig dependencies as well so

> > we won't run into errors when building with an ancient compiler.

> >

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> 

> Thank you Ard, this fixes the build error for us.  Do you know if the

> "crypto extensions" are mandatory ISA extensions? 


I think they are optional (or at least most of them).

> I'm running into some inconsistencies between how clang parses target

> arch between command line flag, function __attribute__, assembler

> directive, and disassembler.  I see arch's like: armv8-a+crc,

> armv8-a+sve, armv8-a+fp16, armv8-a+memtag, armv8-a+lse, but I'm not

> familiar with the `+...` part of the target arch.


This page shows the possible combinations:

https://sourceware.org/binutils/docs/as/AArch64-Extensions.html#AArch64-Extensions

Basically if it's an optional feature in ARMv8.0, you pass armv8-a+...
For optional features only in higher versions, it would be
armv8.5-a+memtag. The table above also states whether it's enabled by
default (i.e. mandatory) in an architecture version. SB for example is
supported from 8.0 but only required in 8.5.

-- 
Catalin
Ard Biesheuvel Oct. 2, 2019, 6:09 p.m. UTC | #4 
On Wed, 2 Oct 2019 at 19:23, Catalin Marinas <catalin.marinas@arm.com> wrote:
>

> On Wed, Oct 02, 2019 at 09:47:41AM -0700, Nick Desaulniers wrote:

> > On Wed, Oct 2, 2019 at 12:55 AM Ard Biesheuvel

> > <ard.biesheuvel@linaro.org> wrote:

> > >

> > > Now that the Clang compiler has taken it upon itself to police the

> > > compiler command line, and reject combinations for arguments it views

> > > as incompatible, the AEGIS128 no longer builds correctly, and errors

> > > out like this:

> > >

> > >   clang-10: warning: ignoring extension 'crypto' because the 'armv7-a'

> > >   architecture does not support it [-Winvalid-command-line-argument]

> > >

> > > So let's switch to armv8-a instead, which matches the crypto-neon-fp-armv8

> > > FPU profile we specify. Since neither were actually supported by GCC

> > > versions before 4.8, let's tighten the Kconfig dependencies as well so

> > > we won't run into errors when building with an ancient compiler.

> > >

> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >

> > Thank you Ard, this fixes the build error for us.  Do you know if the

> > "crypto extensions" are mandatory ISA extensions?

>

> I think they are optional (or at least most of them).

>


This is 32-bit ARM so I don't think any of the extensions are
mandatory. And the crypto ones are export controlled, so they are
definitely not mandatory, except for certain combinations (PMULL
requires AES, and SHA512 requires SHA256).

I don't think 32-bit ARM cores with crypto extensions are widely
available in the field, but since the intrinsics can be compiled to
either ISA, it was rather trivial to retain support for it (and 32-bit
VMs running on a 64-bit hosts may benefit as well)

> > I'm running into some inconsistencies between how clang parses target

> > arch between command line flag, function __attribute__, assembler

> > directive, and disassembler.  I see arch's like: armv8-a+crc,

> > armv8-a+sve, armv8-a+fp16, armv8-a+memtag, armv8-a+lse, but I'm not

> > familiar with the `+...` part of the target arch.

>

> This page shows the possible combinations:

>

> https://sourceware.org/binutils/docs/as/AArch64-Extensions.html#AArch64-Extensions

>

> Basically if it's an optional feature in ARMv8.0, you pass armv8-a+...

> For optional features only in higher versions, it would be

> armv8.5-a+memtag. The table above also states whether it's enabled by

> default (i.e. mandatory) in an architecture version. SB for example is

> supported from 8.0 but only required in 8.5.

>


I am not convinced (but I haven't checked) that this is used in the
same way on 32-bit.
Catalin Marinas Oct. 2, 2019, 9:32 p.m. UTC | #5 
On Wed, Oct 02, 2019 at 08:09:18PM +0200, Ard Biesheuvel wrote:
> On Wed, 2 Oct 2019 at 19:23, Catalin Marinas <catalin.marinas@arm.com> wrote:

> > On Wed, Oct 02, 2019 at 09:47:41AM -0700, Nick Desaulniers wrote:

> > > I'm running into some inconsistencies between how clang parses target

> > > arch between command line flag, function __attribute__, assembler

> > > directive, and disassembler.  I see arch's like: armv8-a+crc,

> > > armv8-a+sve, armv8-a+fp16, armv8-a+memtag, armv8-a+lse, but I'm not

> > > familiar with the `+...` part of the target arch.

> >

> > This page shows the possible combinations:

> >

> > https://sourceware.org/binutils/docs/as/AArch64-Extensions.html#AArch64-Extensions

> >

> > Basically if it's an optional feature in ARMv8.0, you pass armv8-a+...

> > For optional features only in higher versions, it would be

> > armv8.5-a+memtag. The table above also states whether it's enabled by

> > default (i.e. mandatory) in an architecture version. SB for example is

> > supported from 8.0 but only required in 8.5.

> 

> I am not convinced (but I haven't checked) that this is used in the

> same way on 32-bit.


Ah, I didn't realise this was about 32-bit. I don't think the above
applies in this case.

-- 
Catalin
Herbert Xu Oct. 10, 2019, 12:55 p.m. UTC | #6 
On Wed, Oct 02, 2019 at 09:54:48AM +0200, Ard Biesheuvel wrote:
> Now that the Clang compiler has taken it upon itself to police the

> compiler command line, and reject combinations for arguments it views

> as incompatible, the AEGIS128 no longer builds correctly, and errors

> out like this:

> 

>   clang-10: warning: ignoring extension 'crypto' because the 'armv7-a'

>   architecture does not support it [-Winvalid-command-line-argument]

> 

> So let's switch to armv8-a instead, which matches the crypto-neon-fp-armv8

> FPU profile we specify. Since neither were actually supported by GCC

> versions before 4.8, let's tighten the Kconfig dependencies as well so

> we won't run into errors when building with an ancient compiler.

> 

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  crypto/Kconfig  | 1 +

>  crypto/Makefile | 2 +-

>  2 files changed, 2 insertions(+), 1 deletion(-)


Patch 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
diff mbox series

Patch

diff --git a/crypto/Kconfig b/crypto/Kconfig
index e928f88b6206..b138b68329dc 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -331,6 +331,7 @@  config CRYPTO_AEGIS128
 config CRYPTO_AEGIS128_SIMD
 	bool "Support SIMD acceleration for AEGIS-128"
 	depends on CRYPTO_AEGIS128 && ((ARM || ARM64) && KERNEL_MODE_NEON)
+	depends on !ARM || CC_IS_CLANG || GCC_VERSION >= 40800
 	default y
 
 config CRYPTO_AEGIS128_AESNI_SSE2
diff --git a/crypto/Makefile b/crypto/Makefile
index fcb1ee679782..aa740c8492b9 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -93,7 +93,7 @@  obj-$(CONFIG_CRYPTO_AEGIS128) += aegis128.o
 aegis128-y := aegis128-core.o
 
 ifeq ($(ARCH),arm)
-CFLAGS_aegis128-neon-inner.o += -ffreestanding -march=armv7-a -mfloat-abi=softfp
+CFLAGS_aegis128-neon-inner.o += -ffreestanding -march=armv8-a -mfloat-abi=softfp
 CFLAGS_aegis128-neon-inner.o += -mfpu=crypto-neon-fp-armv8
 aegis128-$(CONFIG_CRYPTO_AEGIS128_SIMD) += aegis128-neon.o aegis128-neon-inner.o
 endif