diff mbox series

hash: Kconfig option for SHA512 hardware acceleration

Message ID 20210514091203.23717-1-xypron.glpk@gmx.de
State Accepted, archived
Delegated to: Heinrich Schuchardt
Headers show
Series hash: Kconfig option for SHA512 hardware acceleration | expand

Commit Message

Heinrich Schuchardt May 14, 2021, 9:12 a.m. UTC 
Commit a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
defined function definitions for hardware accelerated SHA384 and SHA512.
If CONFIG_SHA_HW_ACCEL=y, these functions are used.

We already have boards using CONFIG_SHA_HW_ACCEL=y but none implements the
new functions hw_sha384() and hw_sha512().

For implementing the EFI TCG2 protocol we need SHA384 and SHA512. The
missing hardware acceleration functions lead to build errors on boards like
peach-pi_defconfig.

Introduce a new Kconfig symbol CONFIG_SHA512_HW_ACCEL to control if the
functions hw_sha384() and hw_sha512() shall be used to implement the SHA384
and SHA512 algorithms.

Fixes: a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
This patch replaces
hash: revert Allow for SHA512 hardware implementations
https://lists.denx.de/pipermail/u-boot/2021-May/449648.html
https://patchwork.ozlabs.org/project/uboot/patch/20210512170040.137058-1-xypron.glpk@gmx.de/
---
 common/hash.c |  8 ++++----
 lib/Kconfig   | 21 ++++++++++++++++-----
 2 files changed, 20 insertions(+), 9 deletions(-)

--
2.30.2

Comments

Simon Glass May 15, 2021, 3:20 p.m. UTC | #1 
Hi Heinrich,

On Fri, 14 May 2021 at 03:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Commit a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
> defined function definitions for hardware accelerated SHA384 and SHA512.
> If CONFIG_SHA_HW_ACCEL=y, these functions are used.
>
> We already have boards using CONFIG_SHA_HW_ACCEL=y but none implements the
> new functions hw_sha384() and hw_sha512().
>
> For implementing the EFI TCG2 protocol we need SHA384 and SHA512. The
> missing hardware acceleration functions lead to build errors on boards like
> peach-pi_defconfig.
>
> Introduce a new Kconfig symbol CONFIG_SHA512_HW_ACCEL to control if the
> functions hw_sha384() and hw_sha512() shall be used to implement the SHA384
> and SHA512 algorithms.
>
> Fixes: a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> This patch replaces
> hash: revert Allow for SHA512 hardware implementations
> https://lists.denx.de/pipermail/u-boot/2021-May/449648.html
> https://patchwork.ozlabs.org/project/uboot/patch/20210512170040.137058-1-xypron.glpk@gmx.de/
> ---
>  common/hash.c |  8 ++++----
>  lib/Kconfig   | 21 ++++++++++++++++-----
>  2 files changed, 20 insertions(+), 9 deletions(-)

This is good as a stopgap, thanks.

Reviewed-by: Simon Glass <sjg@chromium.org>
Joel Stanley May 18, 2021, 3:36 a.m. UTC | #2 
On Fri, 14 May 2021 at 09:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Commit a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
> defined function definitions for hardware accelerated SHA384 and SHA512.
> If CONFIG_SHA_HW_ACCEL=y, these functions are used.
>
> We already have boards using CONFIG_SHA_HW_ACCEL=y but none implements the
> new functions hw_sha384() and hw_sha512().
>
> For implementing the EFI TCG2 protocol we need SHA384 and SHA512. The
> missing hardware acceleration functions lead to build errors on boards like
> peach-pi_defconfig.
>
> Introduce a new Kconfig symbol CONFIG_SHA512_HW_ACCEL to control if the
> functions hw_sha384() and hw_sha512() shall be used to implement the SHA384
> and SHA512 algorithms.
>
> Fixes: a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> This patch replaces
> hash: revert Allow for SHA512 hardware implementations
> https://lists.denx.de/pipermail/u-boot/2021-May/449648.html
> https://patchwork.ozlabs.org/project/uboot/patch/20210512170040.137058-1-xypron.glpk@gmx.de/

This looks to be a good compromise. Thanks for fixing it.

Reviewed-by: Joel Stanley <joel@jms.id.au>

The CONFIG maze that is the hash acceleration code could do with some
overarching cleanup at some point.


> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -389,21 +389,32 @@ config SHA384
>           (digest).
>
>  config SHA_HW_ACCEL
> -       bool "Enable hashing using hardware"
> +       bool "Enable hardware acceleration for SHA hash functions"
>         help
> -         This option enables hardware acceleration for SHA hashing.
> -         This affects the 'hash' command and also the hash_lookup_algo()
> -         function.
> +         This option enables hardware acceleration for the SHA1 and SHA256
> +         hashing algorithms. This affects the 'hash' command and also the
> +         hash_lookup_algo() function.
> +
> +if SHA_HW_ACCEL
> +
> +config SHA512_HW_ACCEL
> +       bool "Enable hardware acceleration for SHA512"
> +       depends on SHA512_ALGO

This dependency is new, does it make sense? We don't have an
equivalent one for SHA_HW_ACCEL, but perhaps we should introduce one?

> +       help
> +         This option enables hardware acceleration for the SHA384 andSHA512

Nit: there's a missing space after "and" here.

Perhaps you could add to the help text that this option should be
disabled if the platform requires SHA512 support but does not have
hardware acceleration.

> +         hashing algorithms. This affects the 'hash' command and also the
> +         hash_lookup_algo() function.
>
>  config SHA_PROG_HW_ACCEL
>         bool "Enable Progressive hashing support using hardware"
> -       depends on SHA_HW_ACCEL
>         help
>           This option enables hardware-acceleration for SHA progressive
>           hashing.
>           Data can be streamed in a block at a time and the hashing is
>           performed in hardware.
>
> +endif
> +
>  config MD5
>         bool "Support MD5 algorithm"
>         help
> --
> 2.30.2
>
Heinrich Schuchardt May 18, 2021, 10:15 a.m. UTC | #3 
On 5/18/21 5:36 AM, Joel Stanley wrote:
> On Fri, 14 May 2021 at 09:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Commit a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
>> defined function definitions for hardware accelerated SHA384 and SHA512.
>> If CONFIG_SHA_HW_ACCEL=y, these functions are used.
>>
>> We already have boards using CONFIG_SHA_HW_ACCEL=y but none implements the
>> new functions hw_sha384() and hw_sha512().
>>
>> For implementing the EFI TCG2 protocol we need SHA384 and SHA512. The
>> missing hardware acceleration functions lead to build errors on boards like
>> peach-pi_defconfig.
>>
>> Introduce a new Kconfig symbol CONFIG_SHA512_HW_ACCEL to control if the
>> functions hw_sha384() and hw_sha512() shall be used to implement the SHA384
>> and SHA512 algorithms.
>>
>> Fixes: a479f103dc1c ("hash: Allow for SHA512 hardware implementations")
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> This patch replaces
>> hash: revert Allow for SHA512 hardware implementations
>> https://lists.denx.de/pipermail/u-boot/2021-May/449648.html
>> https://patchwork.ozlabs.org/project/uboot/patch/20210512170040.137058-1-xypron.glpk@gmx.de/
>
> This looks to be a good compromise. Thanks for fixing it.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> The CONFIG maze that is the hash acceleration code could do with some
> overarching cleanup at some point.
>
>
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -389,21 +389,32 @@ config SHA384
>>            (digest).
>>
>>   config SHA_HW_ACCEL
>> -       bool "Enable hashing using hardware"
>> +       bool "Enable hardware acceleration for SHA hash functions"
>>          help
>> -         This option enables hardware acceleration for SHA hashing.
>> -         This affects the 'hash' command and also the hash_lookup_algo()
>> -         function.
>> +         This option enables hardware acceleration for the SHA1 and SHA256
>> +         hashing algorithms. This affects the 'hash' command and also the
>> +         hash_lookup_algo() function.
>> +
>> +if SHA_HW_ACCEL
>> +
>> +config SHA512_HW_ACCEL
>> +       bool "Enable hardware acceleration for SHA512"
>> +       depends on SHA512_ALGO
>
> This dependency is new, does it make sense? We don't have an
> equivalent one for SHA_HW_ACCEL, but perhaps we should introduce one?

It does not make sense to show SHA512_HW_ACCEL is we cannot have SHA512
or SHA384.

>
>> +       help
>> +         This option enables hardware acceleration for the SHA384 andSHA512
>
> Nit: there's a missing space after "and" here.

I will add it.

>
> Perhaps you could add to the help text that this option should be
> disabled if the platform requires SHA512 support but does not have
> hardware acceleration.

The current text already says that it "enables hardware acceleration".

It would provide little value to add to all Kconfig options:
"Don't enable this option if you don't have a hardware for it.".

Best regards

Heinrich

>
>> +         hashing algorithms. This affects the 'hash' command and also the
>> +         hash_lookup_algo() function.
>>
>>   config SHA_PROG_HW_ACCEL
>>          bool "Enable Progressive hashing support using hardware"
>> -       depends on SHA_HW_ACCEL
>>          help
>>            This option enables hardware-acceleration for SHA progressive
>>            hashing.
>>            Data can be streamed in a block at a time and the hashing is
>>            performed in hardware.
>>
>> +endif
>> +
>>   config MD5
>>          bool "Support MD5 algorithm"
>>          help
>> --
>> 2.30.2
>>
diff mbox series

Patch

diff --git a/common/hash.c b/common/hash.c
index 10dff7ddb0..90cf46bcba 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -260,12 +260,12 @@  static struct hash_algo hash_algo[] = {
 		.name		= "sha384",
 		.digest_size	= SHA384_SUM_LEN,
 		.chunk_size	= CHUNKSZ_SHA384,
-#ifdef CONFIG_SHA_HW_ACCEL
+#ifdef CONFIG_SHA512_HW_ACCEL
 		.hash_func_ws	= hw_sha384,
 #else
 		.hash_func_ws	= sha384_csum_wd,
 #endif
-#ifdef CONFIG_SHA_PROG_HW_ACCEL
+#if defined(CONFIG_SHA512_HW_ACCEL) && defined(CONFIG_SHA_PROG_HW_ACCEL)
 		.hash_init	= hw_sha_init,
 		.hash_update	= hw_sha_update,
 		.hash_finish	= hw_sha_finish,
@@ -281,12 +281,12 @@  static struct hash_algo hash_algo[] = {
 		.name		= "sha512",
 		.digest_size	= SHA512_SUM_LEN,
 		.chunk_size	= CHUNKSZ_SHA512,
-#ifdef CONFIG_SHA_HW_ACCEL
+#ifdef CONFIG_SHA512_HW_ACCEL
 		.hash_func_ws	= hw_sha512,
 #else
 		.hash_func_ws	= sha512_csum_wd,
 #endif
-#ifdef CONFIG_SHA_PROG_HW_ACCEL
+#if defined(CONFIG_SHA512_HW_ACCEL) && defined(CONFIG_SHA_PROG_HW_ACCEL)
 		.hash_init	= hw_sha_init,
 		.hash_update	= hw_sha_update,
 		.hash_finish	= hw_sha_finish,
diff --git a/lib/Kconfig b/lib/Kconfig
index 6d2d41de30..e9b7b5dc49 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -389,21 +389,32 @@  config SHA384
 	  (digest).

 config SHA_HW_ACCEL
-	bool "Enable hashing using hardware"
+	bool "Enable hardware acceleration for SHA hash functions"
 	help
-	  This option enables hardware acceleration for SHA hashing.
-	  This affects the 'hash' command and also the hash_lookup_algo()
-	  function.
+	  This option enables hardware acceleration for the SHA1 and SHA256
+	  hashing algorithms. This affects the 'hash' command and also the
+	  hash_lookup_algo() function.
+
+if SHA_HW_ACCEL
+
+config SHA512_HW_ACCEL
+	bool "Enable hardware acceleration for SHA512"
+	depends on SHA512_ALGO
+	help
+	  This option enables hardware acceleration for the SHA384 andSHA512
+	  hashing algorithms. This affects the 'hash' command and also the
+	  hash_lookup_algo() function.

 config SHA_PROG_HW_ACCEL
 	bool "Enable Progressive hashing support using hardware"
-	depends on SHA_HW_ACCEL
 	help
 	  This option enables hardware-acceleration for SHA progressive
 	  hashing.
 	  Data can be streamed in a block at a time and the hashing is
 	  performed in hardware.

+endif
+
 config MD5
 	bool "Support MD5 algorithm"
 	help