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

[Kim Phillips]

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.

> > 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??

> 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.

> >> > The primary objective here is to not add h/w vendor dependencies in
> >> > common areas when they can easily be avoided.
> >>
> >> Please can you point specifically to the lines of code you are wanting changed?
> >
> > common/hash.c and include/ace_sha.h should have all occurrences of
> > 'ace' and 'ACE' replaced with something like 'hw' or
> > 'accel'...including the ace_sha.h filename itself.
> >
> > Since it's probably common enough, the 0-length handler could move
> > into the common code, and be enabled iff the hw-accelerated config is
> > set.
> 
> To be honest I think we are getting ahead of ourselves. We are dealing
> here with a patch to enable hardware acceleration in one SOC, with a
> few lines of changes to common code, changing it in a way that fits
> nicely with how hash.c was designed.

sorry, I disagree:  This is a brand new driver class that's being
added, and the design enforces adding h/w vendor specific symbols in
common code.  This tells me the design is broken.  Net doesn't do
this - why should crypto get away with it?  Plus, as I explain
above, it's not that hard to fix (well, for now at least): just
change the common ace parts to have a generic name.

Kim