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

[Simon Glass]

Hi Kim,

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.

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

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.

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.

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

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

Yes I was thinking about that - probably something for a follow-on
patch though. I have just finished getting crc32 in, so will add it to
the queue to think about. Patches welcome.

Regards,
Simon

[...]