[U-Boot] [PATCH] crc32: Correct endianness of crc32 result

[Albert ARIBAUD]

Hi Wolfgang,

On Thu, 18 Apr 2013 12:36:00 +0200, Wolfgang Denk <wd at denx.de> wrote:

> Dear Albert ARIBAUD,
> 
> In message <20130418082027.4b5ea191 at lilith> you wrote:
> > 
> > > #ifdef USE_HOSTCC
> > >    crc = htobe32(crc);
> > >    memcpy(output, &crc, sizeof(crc));
> > > #else
> > >    crc = cpu_to_be32(crc);
> > >    put_unaligned(crc, (uint32_t *)output);
> > > #endif
> > > 
> > > This produces the same code as my original patch. If this is
> > > acceptable then I will do that, although it doesn't really seem any
> > > better.
> > 
> > Wolfgang may not like it any more than put_unaligned_be32() as it
> > builds upon it (and the disk patch could have used the be32 version as
> 
> Indeed.
> 
> > well). Personally, I think we should allow and use these...
> > 
> > ... and work on optimizing their implementation for ARM; we should be
> > able to reduce such put()s and get()s to a few instructions. And to
> 
> OK - and what about the other architectures that suffer from the same
> issues?

They should/could provide their own optimized versions, obviously.

> > avoid any misunderstanding, yes, I volunteer for the optimizing work. :)
> 
> I really dislike introducing such custom functions when we have
> standard functions available that implement the same purposes.

I understand the point; this is a classical opposition of generic vs
optimized.

> Checking Linux code (as U-Boot is not representative here):
> 
> 	-> find * -name '*.c' | wc -l
> 	18362
> 	-> find * -name '*.c' | xargs fgrep -l put_unaligned | wc -l
> 	136
> 
> i. e. just 0.75% of the source files actually use any of the
> "put_unaligned*()" variants - it is a highly exotic function.
> 
> htobe32() is even worse - just a single source file in the whole Lnux
> tree uses it (arch/um/drivers/cow_user.c).
> 
> 
> Can we not stick to standard functions?  Instead of htobe32() we
> should use htonl() which is available both in U-Boot and in the host
> environment.

Note that there are two problems here: endianness conversion and
unaligned storage. We can indeed use htoln() instead of htobe32(), but
that only affects/solved endianness issues, not alignment ones, for
which there is no solution that is efficient across all ARM targets.

Note too that if the hash infrastructure mandated that the output
buffer be 4-byte-aligned, i.e. a u32* or similar, we would not even
have to worry about unalignment; such a constraint on the output
buffer makes all the more sense if we consider the fact that current
hash sizes are 4 (crc32), 20 (SHA1) and 32 (SHA256), all multiples of
4, and future hashes will most certainly not be less aligned.

So how about changing the element type of output in the definition of
hash_func_ws, adapting the corresponding implementations sha1_csum_wd,
sha256_csum_wd and crc32_wd_buf, and adapting the output argument
of the sole call to hash_func_ws, that is, the local "u8
output[HASH_MAX_DIGEST_SIZE];" in hash.c? Then we'be done with
alignment.

Note finally that we *need* unaligned access macros/inline
functions/whatever, if only for exceptions laid out in
doc/README.arm-unaligned-accesses. If we avoid calling them too often,
we can live with generic.

> Best regards,
> 
> Wolfgang Denk

Amicalement,
-- 
Albert.