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

[Kim Phillips]

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.

> >> extended later when someone else has one.
> >
> > maybe, but we can try to avoid people copying existing code patterns,
> > i.e. polluting common code when adding crypto routines for their h/w
> > which are basically the same function declarations but with different
> > names.
> 
> Are you referring to adding code in into the hash algorithm table in
> hash.c? I specifically designed it so that people could add their

yes.

> algorithms in there. Once we have device model in place then I'm sure
> we can do better, but for now that's the U-Boot way.

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.

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

Also, can we try to leave the existing void return type for the hash
functions for the rest of u-boot.  What do you think about changing
common/hash.c to do the s/w fallback if h/w failed instead of in the
driver(s)?

> >> > Something like CONFIG_HW_SHA{1,256} ought to do it.
> >> >
> >> >> But I don't think crypto units other than ACE will use the code in
> >> >> this patch - it is intended to implement ACE support, and put it ahead
> >> >> of software support in terms of priority.
> >> >
> >> > the same priority that any h/w accelerated device would get - higher
> >> > than that of software crypto.
> >> >
> >> > Another question for Akshay wrt the timeout (since I never got a
> >> > reply re: documentation):  how can the h/w fault?
> [...]
> 
> Regards,
> Simon
> 
> >
> > Kim
> >
> 

you're doing it again :)

Kim