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

[Simon Glass]

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.

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

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

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

I'm willing to come up with patches to move to a hash_register() type
implementation which will allow us to address this shortcoming - but
people will need to understand that it will increase code size a
little more. I was intending to wait for device model, but I don't
mind doing something in the interim. Then I will happily adjust this
driver to use that new mechanism. In the meantime, it would help if
you could post some Freescale patches for us to compare / review.

Regards.
Simon