[PATCH 3/3] hash: Allow for SHA512 hardware implementations

[Simon Glass]

Hi Heinrich,

On Thu, 13 May 2021 at 09:38, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 5/13/21 4:36 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 12 May 2021 at 15:27, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> Am 12. Mai 2021 19:32:42 MESZ schrieb Simon Glass <sjg at chromium.org>:
> >>> Hi Heinrich,
> >>>
> >>> On Wed, 12 May 2021 at 10:25, Heinrich Schuchardt <xypron.glpk at gmx.de>
> >>> wrote:
> >>>>
> >>>> On 12.05.21 18:05, Simon Glass wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Wed, 12 May 2021 at 10:01, Heinrich Schuchardt
> >>> <xypron.glpk at gmx.de> wrote:
> >>>>>>
> >>>>>> On 17.02.21 04:20, Joel Stanley wrote:
> >>>>>>> Similar to support for SHA1 and SHA256, allow the use of hardware
> >>> hashing
> >>>>>>> engine by enabling the algorithm and setting  CONFIG_SHA_HW_ACCEL
> >>> /
> >>>>>>> CONFIG_SHA_PROG_HW_ACCEL.
> >>>>>>>
> >>>>>>> Signed-off-by: Joel Stanley <joel at jms.id.au>
> >>>>>>
> >>>>>> This merged patch leads to errors compiling the EFI TCG2 protocol
> >>> on
> >>>>>> boards with CONFIG_SHA_HW_ACCEL.
> >>>>>>
> >>>>>> There is not as single implementation of hw_sha384 and hw_sha512.
> >>> You
> >>>>>> could only use CONFIG_SHA_HW_ACCEL for selecting these functions
> >>> if
> >>>>>> these were implemented for *all* boards with
> >>> CONFIG_SHA_HW_ACCEL=y. But
> >>>>>> this will never happen.
> >>>>>>
> >>>>>> *This patch needs to be reverted.*
> >>>>>>
> >>>>>> Why do we have CONFIG_SHA_HW_ACCEL at all and don't use weak
> >>> functions
> >>>>>> instead?
> >>>>>
> >>>>> This is all a mess. We should not use weak functions IMO, but
> >>> instead
> >>>>> have a driver interface, like we do with filesystems.
> >>>>>
> >>>>> Part of the challenge is the need to keep code size small for
> >>>>> platforms that only need one algorithm.
> >>>>
> >>>> If a function is not referenced, the linker will eliminate it. But
> >>> with
> >>>> a driver interface every function in the interface is referenced.
> >>>
> >>> Indeed, but we can still have an option for enabling the progressive
> >>> interface. My point is that the implementation (software or hardware)
> >>> should use a driver.
> >>>
> >>> Regards,
> >>> Simon
> >>
> >> Anyway that should not stop us from reverting a problematic patch. We can work on refactoring afterwards.
> >
> > Well the patch was applied because tests passed. We should be careful
> > about reverting a patch due to problems it causes in the future.
>
> The code compiled because SHA384 and SHA512 were not used together with
> CONFIG_SHA_HW_ACCEL=y. But we need these functions to implement the TCG2
> protocol on boards with TPMv2.

Let's fix it then. The patch makes all the hashing algorithms work the
same, which seems right to me. If we come to converting this to a
linker list, which we should, then it will be easier with this patch
applied than not.

>
> Gitlab CI had no issues with reverting the patch.

OK. It's up to Tom what he wants to do. But how about sending a patch
to do what you want, instead?

Regards,
SImon