Re: [PATCH v2 4/5] lib: sha256: Add support for hardware specific sha256_process

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4895 bytes --]

On Wed, Feb 08, 2023 at 11:28:21AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 7 Feb 2023 at 17:10, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Feb 07, 2023 at 03:25:16PM -0700, Simon Glass wrote:
> > > Hi Loic,
> > >
> > > On Tue, 7 Feb 2023 at 14:47, Loic Poulain <loic.poulain@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Tue, 7 Feb 2023 at 05:05, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Loic,
> > > > >
> > > > > On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg@chromium.org> a écrit :
> > > > > >>
> > > > > >> Hi Loic,
> > > > > >>
> > > > > >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain@linaro.org> wrote:
> > > > > >> >
> > > > > >> > Mark sha256_process as weak to allow hardware specific implementation.
> > > > > >> > Add parameter for supporting multiple blocks processing.
> > > > > >> >
> > > > > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > > > > >> > ---
> > > > > >> >  lib/sha256.c | 26 +++++++++++++++++++-------
> > > > > >> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > > > > >> >
> > > > [...]
> > > > > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data,
> > > > > >> > +                          unsigned int blocks)
> > > > > >> > +{
> > > > > >> > +       if (!blocks)
> > > > > >> > +               return;
> > > > > >> > +
> > > > > >> > +       while (blocks--) {
> > > > > >> > +               sha256_process_one(ctx, data);
> > > > > >> > +               data += 64;
> > > > > >> > +       }
> > > > > >> > +}
> > > > > >> > +
> > > > > >> >  void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length)
> > > > > >> >  {
> > > > > >> >         uint32_t left, fill;
> > > > > >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length)
> > > > > >> >
> > > > > >> >         if (left && length >= fill) {
> > > > > >> >                 memcpy((void *) (ctx->buffer + left), (void *) input, fill);
> > > > > >> > -               sha256_process(ctx, ctx->buffer);
> > > > > >> > +               sha256_process(ctx, ctx->buffer, 1);
> > > > > >> >                 length -= fill;
> > > > > >> >                 input += fill;
> > > > > >> >                 left = 0;
> > > > > >> >         }
> > > > > >> >
> > > > > >> > -       while (length >= 64) {
> > > > > >> > -               sha256_process(ctx, input);
> > > > > >> > -               length -= 64;
> > > > > >> > -               input += 64;
> > > > > >> > -       }
> > > > > >> > +       sha256_process(ctx, input, length / 64);
> > > > > >> > +       input += length / 64 * 64;
> > > > > >> > +       length = length % 64;
> > > > > >> >
> > > > > >> >         if (length)
> > > > > >> >                 memcpy((void *) (ctx->buffer + left), (void *) input, length);
> > > > > >> > --
> > > > > >> > 2.7.4
> > > > > >> >
> > > > > >>
> > > > > >> I just came across this patch as it broke minnowmax.
> > > > > >
> > > > > >
> > > > > > Ok, is it a build time or runtime break?
> > > > >
> > > > > Build, but you need the binary blobs to see it :-(
> > > > > >>
> > > > > >> This should be using driver model, not weak functions. Please can you
> > > > > >> take a look?
> > > >
> > > > Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig;
> > > > gcc-11.3.0), and I've not observed any issue (but I had to fake some
> > > > of the binary blobs...). Could you share the build problem/error you
> > >
> > > Unfortunately you need the blobs!
> > >
> > > > encountered? As you mentioned it, Is the error specifically related to
> > > > _weak function linking? Would like to have a simple and quick fix
> > > > before trying to move on to a more proper DM based solution.
> > >
> > > It is just because of the code size increase, I believe. I am planning
> > > to dig into it a bit as Bin Meng asked for more info as to why I sent
> > > a revert for his patch moving U-Boot.
> >
> > That honestly makes more sense, having stared at the commit in
> > question.  Perhaps Minnow needs LTO enabled.
> 
> Yes, that would likely help. But Bin's point is that it should be
> possible to move the text base.

I suspect that might just be more fall-out of the size problem and
possibly some hard coded assumptions in the blobs?

> Anyway, the point of this thread is that this needs to be done as
> drivers rather than weak functions. Unfortunately hash.c has grown a
> few appendages...this is yet another.

I don't know, it seems fine, especially since we aren't really talking
about a "hash driver" but rather introducing some performance sensitive
code.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]