[U-Boot] [PATCH 4/5 v4] gen: Add ACE acceleration to hash

[Simon Glass]

Hi Kim,

On Mon, Mar 11, 2013 at 5:44 PM, Kim Phillips <kim.phillips at freescale.com>wrote:

> On Thu, 7 Mar 2013 19:11:16 -0800
> Simon Glass <sjg at chromium.org> wrote:
>
> > Hi Kim,
> >
> > On Thu, Mar 7, 2013 at 6:18 PM, Kim Phillips <kim.phillips at freescale.com>
> wrote:
> > > On Thu, 7 Mar 2013 17:05:15 -0800
> > > Simon Glass <sjg at chromium.org> wrote:
> > >
> > >> On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <
> kim.phillips at freescale.com> wrote:
> > >> > On Wed, 6 Mar 2013 18:08:21 -0800
> > >> > Simon Glass <sjg at chromium.org> wrote:
> > >> >
> > >> >> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <
> kim.phillips at freescale.com> wrote:
> > >> >> > On Tue, 5 Mar 2013 22:22:09 -0800
> > >> >> > Simon Glass <sjg at chromium.org> wrote:
> > >> >> >
> > >> >> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <
> kim.phillips at freescale.com> wrote:
> > >> >> >> > On Tue, 5 Mar 2013 17:51:00 -0800
> > >> >> >> > Simon Glass <sjg at chromium.org> wrote:
> > >> >> >> >> >> Changes sice v3:
> > >> >> >> >> >>       - Changed command names to lower case in algo
> struct.
> > >> >> >> >> >>       - Added generic ace_sha config.
> > >> >> >> >> >
> > >> >> >> >> > I wouldn't call "ace" a generic name - crypto units other
> than
> > >> >> >> >> > ACE should be able to use this code.
> > >> >> >> >>
> > >> >> >> >> I don't really understand this comment. A new CONFIG has
> been added,
> > >> >> >> >> and this is specific to that unit. Are you suggesting that
> it be
> > >> >> >> >
> > >> >> >> > right, and that's the problem - it needn't be specific to
> that unit.
> > >> >> >>
> > >> >> >> Really? I think here we have a patch for an ACE unit, and
> currently
> > >> >> >> the only implementation is in an Exynos chip. It can easily be
> > >> >> >
> > >> >> > so make the ace_sha.o build depend on whichever level of chip
> config
> > >> >> > applies - CONFIG_S5P, CONFIG_EXYNOS5, or CONFIG_SMDK5250.  No
> need
> > >> >> > for a new define specifically for ACE, right?
> > >> >>
> > >> >> No, the ACE may appear in multiple chips, and anyway we may want to
> > >> >> enable it or disable it. Drivers tend to have their own configs
> since
> > >> >> some boards want to use (for example) USB, crypto, mmc, and some
> > >> >> don't.
> > >> >
> > >> > ok, if you really need the ACE define, restrict it to platform code
> > >> > and the driver, but not common code.
> > >>
> > >> That is in the design of the hash.c module. It is intended to permit
> > >> insertion of different algorithm implementations. We could perhaps
> > >> introduce a hash_register() function to deal with this, but that's the
> > >> way it is at present.
> > >
> > > ok, well this is the first time a new alternate algorithm
> > > implementation is being posted, and it's bringing up a flaw in the
> > > design - no vendor-specific stuff in common areas.
> >
> > OK so let's look at adding the hash_register() idea. But not in this
> > series. That should come later in a revision of the hash.c
> > infrastructure, since it needs to adjust sha1, sha255 and crc32.
>
> I don't understand: why not s/ace/hw/g in common/ and include/ on
> this patchseries, then move straight to the device model at some
> later point?  It's a compromise, but it works fine for the time
> being - other vendors can add their hash support without having to
> touch common code, code size is not affected, etc.
>

Fine with me. The effect is the same - this is just a rename. Should not be
done in the ace.h file though, only in the naming of the functions called
from hash.c, right?


>
> > >> > the problem is there are only two algorithms for all - the
> > >> > accelerated, and the non-.  Presumably we get the acceleration for
> > >> > free, so we always will prefer to use the accelerated version, and
> > >> > if it doesn't work, then the core s/w implementation.  The
> > >> > common/hash.c infrastructure currently doesn't support that: it
> > >> > assumes the existence of multiple heterogeneous crypto units will
> > >> > exist on a single u-boot instance, each used depending on its
> > >> > priority, which is not the case.
> > >>
> > >> Fair enough, and that might be a good idea, but that is an issue for
> > >> the hash infrastructure. It would be good to get a second hardware
> > >> accelerator upstream so we can judge where to take this.
> > >
> > > well right now as it stands the 2nd h/w accelerator would have to
> > > duplicate hash entry point function signatures, just changing the
> > > ACE in the name to that of its h/w, and then spin on a tough
> > > decision: what priority does my h/w have vs. Samsung's ACE??
> >
> > I thought you said that only one h/w accelerator would be used on a
> board?
>
> yes, I was trying to be funny, but as usual, it didn't work.
>

OK, sorry I missed it.


>
> > >> If you have one in a Freescale SOC can I please request that you send
> > >> some patches upstream and we can then evaluate how best to arrange the
> > >> code.
> > >
> > > they'll come, eventually I hope.  Other than the driver internals,
> > > the only thing different from the ACE functional capacity provided
> > > in this patchseries for the analogous Freescale parts is that the
> > > hash infrastructure would need to be adapted for runtime detection
> > > of the crypto unit's existence.
> > >
> > > But let's not use this as an excuse to let things slide, please.
> > >
> > > this is new infrastructure, right?  It's common to make mistakes
> > > without seeing the entire picture, and this patchset represents the
> > > next piece.
> >
> > I would prefer to invent new infrastructure when we have two users,
> > not one, otherwise we run the risk of over-engineering. I already hit
> > a code size snag with crc32 integration. This is just the first user
> > and there is not yet a clear picture of what other users will want. If
> > you are saying that Freescale will need things to be done a particular
> > way, please post the patches and we can take a look at something that
> > fits both SOCs.
>
> I'm saying that - at least with the current patchseries as-is -
> other crypto vendors would have to touch common code.
>

Yes.


>
> Freescale would need runtime device detection, but that's completely
> orthogonal to this patchseries.
>

You can use device tree - CONFIG_OF_CONTROL - in fact ACE could move to
that also.

[snip]

Regards,
Simon