[U-Boot] [PATCH v6] socfpga: Add socfpga preloader signing to mkimage

[Charles Manning]

On Friday 28 February 2014 10:57:21 Wolfgang Denk wrote:
> Dear Charles,
>
> In message <1393472979-7522-1-git-send-email-cdhmanning at gmail.com> you 
wrote:
> > Like many platforms, the Altera socfpga platform requires that the
> > preloader be "signed" in a certain way or the built-in boot ROM will
> > not boot the code.
>
> ...
>
> > diff --git a/include/crc32_alt.h b/include/crc32_alt.h
> > new file mode 100644
> > index 0000000..813d55d
> > --- /dev/null
> > +++ b/include/crc32_alt.h
>
> "alt" is a bad name as there is more than one alternative. Please use
> a descriptive name.

Ok I shall do that.


>
> > --- /dev/null
> > +++ b/lib/crc32_alt.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * Copyright (C) 2014 Charles Manning <cdhmanning at gmail.com>
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + *
> > + * Note that the CRC is **not** the zlib/Adler crc32 in crc32.c.
> > + * It is the CRC-32 used in bzip2, ethernet and elsewhere.
> > + */
>
> I understand this was copied from "lib/bzlib_crctable.c" ? BUt you
> claim copyright on this, without any attribution where you got it
> form.  This is very, vary bad.

You understand incorrectly. I did not copy it from bzlib.  I generated it.
I had generated this table before I even knew it was part of 
ib/bzlib_crctable.c.

Of course it **must** have the same values in it because that is how the 
mathematics works out.

I hope you are as free with your retractions as you are with your accusations!

>
> > +static uint32_t crc_table[256] = {
> > +	0x00000000, 0x04c11db7, 0x09823b6e, 0x0d4326d9,
>
> ...
>
> > +	0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4,
> > +};
>
> Indeed this looks very much like a duplication of code we have
> elsewhere:
>
> - in "lib/bzlib_crctable.c" (as BZ2_crc32Table[])
> - in "drivers/mtd/ubi/crc32table.h" (as crc32table_be[])
>
> > +uint32_t crc32_alt(uint32_t crc, const void *_buf, int length)
> > +{
> > +	const uint8_t *buf = _buf;
> > +
> > +	crc ^= ~0;
> > +
> > +	while (length--) {
> > +		crc = (crc << 8) ^ crc_table[((crc >> 24) ^ *buf) & 0xff];
> > +		buf++;
> > +	}
> > +
> > +	crc ^= ~0;
> > +
> > +	return crc;
> > +}
>
> In addition to this, we also have
> - crc32 in "lib/crc32.c"
> - crc32() in "tools/mxsimage.c"
> - make_crc_table() and pbl_crc32() in "tools/pblimage.c"

>
>
> I really think we should factor out the common tables and code here.
> I will not accept yet another duplication of this - we already have
> way too many of these.

Based on your comments in another thread, I have suggested that I do one of 
two things:

1) Either have a new C file in lib that uses the bzlib table or
2) Extend the bzlib in a way that exposes a function called
crc32_bzlib() or bzlib_crc32().

Whichever you like.

It seems to me that refactoring is best kept as a different patch.

May I humbly submit that it would be a good idea to bed this socfpga signer 
down. Then, as a separate commit and a separate patch, I will refactor with 
pbllimage and mxsimage. 

Does that sound OK with you?

>
> > --- /dev/null
> > +++ b/tools/socfpgaimage.c
> > @@ -0,0 +1,278 @@
>
> ...
>
> > + * Endian is LSB.
>
> What does that mean? When talking about endianess, I know
> "Big-endian" and "Little-endian" - LSB is meaningless in this context
> (unless you write something like "LSB first" or "LSB last", but even
> this would not be really clear).

Sorry typo. I will fix.

>
> > + * Note that the CRC used here is **not** the zlib/Adler crc32. It is
> > the + * CRC-32 used in bzip2, ethernet and elsewhere.
>
> Does this have a name?
CRC-32 ... the real one. Adler was really a bit naughty in using the crc32 
name for something that:
a) is not the "most standard" crc
b) is not even really a crc anyway -i t is more correctly a checksum.

>
> > + * The image is padded out to 64k, because that is what is
> > + * typically used to write the image to the boot medium.
> > + */
>
> "typically used" - by what or whom?  Is there any rechnical reason for
> such padding?  If not, can we not rather omit this?

The files are often concatenated into blocks of 4 repeats and are also often 
written into NAND and aligning them to 64k makes some sense.

>
> > +/*
> > + * Some byte marshalling functions...
> > + * These load or get little endian values to/from the
> > + * buffer.
> > + */
> > +static void load_le16(uint8_t *buf, uint16_t v)
> > +{
> > +	buf[0] = (v >> 0) & 0xff;
> > +	buf[1] = (v >> 8) & 0xff;
> > +}
> > +
> > +static void load_le32(uint8_t *buf, uint32_t v)
> > +{
> > +	buf[0] = (v >>  0) & 0xff;
> > +	buf[1] = (v >>  8) & 0xff;
> > +	buf[2] = (v >> 16) & 0xff;
> > +	buf[3] = (v >> 24) & 0xff;
> > +}
>
> These are misnomers.  You do not load something, but instead you store
> the value of 'v' into the buffer 'buf'.
>
> And why do you invent new functions here instead of using existing
> code (like put_unaligned_le16(), put_unaligned_le32()) ?

I was not aware of the existence of these functions.

Thank you.

>
> > +static uint16_t get_le16(const uint8_t *buf)
> > +{
> > +	uint16_t retval;
> > +
> > +	retval = (((uint16_t) buf[0]) << 0) |
> > +		 (((uint16_t) buf[1]) << 8);
> > +	return retval;
> > +}
> > +
> > +static uint32_t get_le32(const uint8_t *buf)
> > +{
> > +	uint32_t retval;
> > +
> > +	retval = (((uint32_t) buf[0]) <<  0) |
> > +		 (((uint32_t) buf[1]) <<  8) |
> > +		 (((uint32_t) buf[2]) << 16) |
> > +		 (((uint32_t) buf[3]) << 24);
> > +	return retval;
> > +}
>
> Why do you not use existing code (like get_unaligned_le16(),
> get_unaligned_le32()) ?

I was not aware of the existence of these functions.

Thank you.

>
> > +static int align4(int v)
> > +{
> > +	return ((v + 3) / 4) * 4;
> > +}
>
> Don't we have macros to do this?

Possibly, I will look.

Thanks

Charles