[PATCH] hash: Kconfig option for SHA512 hardware acceleration

[Heinrich Schuchardt]

On 5/18/21 5:36 AM, Joel Stanley wrote:
> On Fri, 14 May 2021 at 09:12, Heinrich Schuchardt <xypron.glpk at 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 at 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 at 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
>>