diff mbox

[v2] crypto/arm: accelerated SHA-512 using ARM generic ASM and NEON

Message ID 2845656.4sNJ3XBxnz@wuerfel (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show

Commit Message

Arnd Bergmann April 11, 2015, 8:48 a.m. UTC
On Saturday 11 April 2015 09:35:15 Ard Biesheuvel wrote:
> On 10 April 2015 at 22:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> >> On 10 apr. 2015, at 22:08, Arnd Bergmann <arnd@arndb.de> wrote:
> >>
> >>> On Friday 10 April 2015 16:29:08 Ard Biesheuvel wrote:
> >>> +#if __ARM_MAX_ARCH__>=7
> >>> +.arch  armv7-a
> >>> +.fpu   neon
> >>> +
> >>
> >> This will cause a build failure on an ARMv7-M build, which is incompatible
> >> with .arch  armv7-a and .fpu   neon.
> >>
> >
> > The neon part depends on CONFIG_KERNEL_MODE_NEON, which would never be set for that platform, I suppose
> 
> On second thought, that is not entirely true, but I still don't think
> there is problem here:
> the .arch/.fpu declarations are understood perfectly fine by GAS when
> targeting ARMv7-M. Only, it will emit code that is incompatible with
> it. However, this code is invoked at runtime only if a NEON unit has
> been detected, so it will just be ignored on ARMv7-M

Sorry, I should have collected my findings better when replying to your
patch. What I remembered was that I saw a problem in this area in linux-next
with randconfig builds, but I did not notice that it was for a different
file, and I had not double-checked that patch yet in order to send it
out.

See below for the patch I'm currently using for my randconfig builder.
Before you apply this, please check again which files are affected, as
it's possible that there are other modules that suffer from the same
problem.

	Arnd

8<-----------
Subject: [PATCH] ARM: crypto: avoid sha256 code on ARMv7-M

The sha256 assembly implementation can deal with all architecture levels
from ARMv4 to ARMv7-A, but not with ARMv7-M. Enabling it in an
ARMv7-M kernel results in this build failure:

arm-linux-gnueabi-ld: error: arch/arm/crypto/sha256_glue.o: Conflicting architecture profiles M/A
arm-linux-gnueabi-ld: failed to merge target specific data of file arch/arm/crypto/sha256_glue.o

This adds a Kconfig dependency to prevent the code from being disabled
for ARMv7-M.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ard Biesheuvel April 11, 2015, 10:27 a.m. UTC | #1
> On 11 apr. 2015, at 10:48, Arnd Bergmann <arnd@arndb.de> wrote:
> 
>> On Saturday 11 April 2015 09:35:15 Ard Biesheuvel wrote:
>>> On 10 April 2015 at 22:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> 
>>>>> On 10 apr. 2015, at 22:08, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> 
>>>>> On Friday 10 April 2015 16:29:08 Ard Biesheuvel wrote:
>>>>> +#if __ARM_MAX_ARCH__>=7
>>>>> +.arch  armv7-a
>>>>> +.fpu   neon
>>>>> +
>>>> 
>>>> This will cause a build failure on an ARMv7-M build, which is incompatible
>>>> with .arch  armv7-a and .fpu   neon.
>>> 
>>> The neon part depends on CONFIG_KERNEL_MODE_NEON, which would never be set for that platform, I suppose
>> 
>> On second thought, that is not entirely true, but I still don't think
>> there is problem here:
>> the .arch/.fpu declarations are understood perfectly fine by GAS when
>> targeting ARMv7-M. Only, it will emit code that is incompatible with
>> it. However, this code is invoked at runtime only if a NEON unit has
>> been detected, so it will just be ignored on ARMv7-M
> 
> Sorry, I should have collected my findings better when replying to your
> patch. What I remembered was that I saw a problem in this area in linux-next
> with randconfig builds, but I did not notice that it was for a different
> file, and I had not double-checked that patch yet in order to send it
> out.
> 
> See below for the patch I'm currently using for my randconfig builder.
> Before you apply this, please check again which files are affected, as
> it's possible that there are other modules that suffer from the same
> problem.
> 

Ah i see it now. The new Sha256 module as well as the Sha512 i am proposing here both use a single .o containing the !neon and neon implementations, and only expose the latter if KERNEL_MODE_NEON. This way, we can use the exact same .S file ad OpenSSL, which should mean less maintenance burden.

So your fix seems the most appropriate, even if it means v7m won't be able to use the !neon part either.


>    Arnd
> 
> 8<-----------
> Subject: [PATCH] ARM: crypto: avoid sha256 code on ARMv7-M
> 
> The sha256 assembly implementation can deal with all architecture levels
> from ARMv4 to ARMv7-A, but not with ARMv7-M. Enabling it in an
> ARMv7-M kernel results in this build failure:
> 
> arm-linux-gnueabi-ld: error: arch/arm/crypto/sha256_glue.o: Conflicting architecture profiles M/A
> arm-linux-gnueabi-ld: failed to merge target specific data of file arch/arm/crypto/sha256_glue.o
> 
> This adds a Kconfig dependency to prevent the code from being disabled
> for ARMv7-M.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/arch/arm/crypto/Kconfig b/arch/arm/crypto/Kconfig
> index 458729d2ce22..76463da22f81 100644
> --- a/arch/arm/crypto/Kconfig
> +++ b/arch/arm/crypto/Kconfig
> @@ -49,6 +49,7 @@ config CRYPTO_SHA2_ARM_CE
> config CRYPTO_SHA256_ARM
>    tristate "SHA-224/256 digest algorithm (ARM-asm and NEON)"
>    select CRYPTO_HASH
> +    depends on !CPU_V7M
>    help
>      SHA-256 secure hash standard (DFIPS 180-2) implemented
>      using optimized ARM assembler and NEON, when available.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel April 11, 2015, 7:15 p.m. UTC | #2
On 11 April 2015 at 10:48, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 11 April 2015 09:35:15 Ard Biesheuvel wrote:
>> On 10 April 2015 at 22:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> >
>> >> On 10 apr. 2015, at 22:08, Arnd Bergmann <arnd@arndb.de> wrote:
>> >>
>> >>> On Friday 10 April 2015 16:29:08 Ard Biesheuvel wrote:
>> >>> +#if __ARM_MAX_ARCH__>=7
>> >>> +.arch  armv7-a
>> >>> +.fpu   neon
>> >>> +
>> >>
>> >> This will cause a build failure on an ARMv7-M build, which is incompatible
>> >> with .arch  armv7-a and .fpu   neon.
>> >>
>> >
>> > The neon part depends on CONFIG_KERNEL_MODE_NEON, which would never be set for that platform, I suppose
>>
>> On second thought, that is not entirely true, but I still don't think
>> there is problem here:
>> the .arch/.fpu declarations are understood perfectly fine by GAS when
>> targeting ARMv7-M. Only, it will emit code that is incompatible with
>> it. However, this code is invoked at runtime only if a NEON unit has
>> been detected, so it will just be ignored on ARMv7-M
>
> Sorry, I should have collected my findings better when replying to your
> patch. What I remembered was that I saw a problem in this area in linux-next
> with randconfig builds, but I did not notice that it was for a different
> file, and I had not double-checked that patch yet in order to send it
> out.
>
> See below for the patch I'm currently using for my randconfig builder.
> Before you apply this, please check again which files are affected, as
> it's possible that there are other modules that suffer from the same
> problem.
>
>         Arnd
>
> 8<-----------
> Subject: [PATCH] ARM: crypto: avoid sha256 code on ARMv7-M
>
> The sha256 assembly implementation can deal with all architecture levels
> from ARMv4 to ARMv7-A, but not with ARMv7-M. Enabling it in an
> ARMv7-M kernel results in this build failure:
>
> arm-linux-gnueabi-ld: error: arch/arm/crypto/sha256_glue.o: Conflicting architecture profiles M/A
> arm-linux-gnueabi-ld: failed to merge target specific data of file arch/arm/crypto/sha256_glue.o
>
> This adds a Kconfig dependency to prevent the code from being disabled

... enabled?

> for ARMv7-M.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/arch/arm/crypto/Kconfig b/arch/arm/crypto/Kconfig
> index 458729d2ce22..76463da22f81 100644
> --- a/arch/arm/crypto/Kconfig
> +++ b/arch/arm/crypto/Kconfig
> @@ -49,6 +49,7 @@ config CRYPTO_SHA2_ARM_CE
>  config CRYPTO_SHA256_ARM
>         tristate "SHA-224/256 digest algorithm (ARM-asm and NEON)"
>         select CRYPTO_HASH
> +       depends on !CPU_V7M
>         help
>           SHA-256 secure hash standard (DFIPS 180-2) implemented
>           using optimized ARM assembler and NEON, when available.
>

@Herbert: could you please apply this onto cryptodev before sending
out your pull request for v4.1?
And please disregard $subject, I will post a v3 with a similar
'depends on' added (unless you're ok to add it yourself)

Thanks,
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 11, 2015, 8:43 p.m. UTC | #3
On Saturday 11 April 2015 12:27:18 Ard Biesheuvel wrote:
> 
> Ah i see it now. The new Sha256 module as well as the Sha512 i am proposing here both use a single .o containing the !neon and neon implementations, and only expose the latter if KERNEL_MODE_NEON. This way, we can use the exact same .S file ad OpenSSL, which should mean less maintenance burden.
> 
> So your fix seems the most appropriate, even if it means v7m won't be able to use the !neon part either.
> 
> 

Ok, sounds good. If someone wants to change that code to work on ARMv7-M,
they probably want that fix in the openssl version as well, and then we
can update both.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu April 13, 2015, 4:13 a.m. UTC | #4
On Sat, Apr 11, 2015 at 09:15:10PM +0200, Ard Biesheuvel wrote:
>
> @Herbert: could you please apply this onto cryptodev before sending
> out your pull request for v4.1?

Done.

> And please disregard $subject, I will post a v3 with a similar
> 'depends on' added (unless you're ok to add it yourself)

Please resend the patch.  But I'll process it after the merge
window closes so no hurry.

Thanks,
Ard Biesheuvel April 13, 2015, 9:34 a.m. UTC | #5
On 13 April 2015 at 06:13, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sat, Apr 11, 2015 at 09:15:10PM +0200, Ard Biesheuvel wrote:
>>
>> @Herbert: could you please apply this onto cryptodev before sending
>> out your pull request for v4.1?
>
> Done.
>
>> And please disregard $subject, I will post a v3 with a similar
>> 'depends on' added (unless you're ok to add it yourself)
>
> Please resend the patch.  But I'll process it after the merge
> window closes so no hurry.
>


OK, all fine.

Thanks Herbert!
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/crypto/Kconfig b/arch/arm/crypto/Kconfig
index 458729d2ce22..76463da22f81 100644
--- a/arch/arm/crypto/Kconfig
+++ b/arch/arm/crypto/Kconfig
@@ -49,6 +49,7 @@  config CRYPTO_SHA2_ARM_CE
 config CRYPTO_SHA256_ARM
 	tristate "SHA-224/256 digest algorithm (ARM-asm and NEON)"
 	select CRYPTO_HASH
+	depends on !CPU_V7M
 	help
 	  SHA-256 secure hash standard (DFIPS 180-2) implemented
 	  using optimized ARM assembler and NEON, when available.