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

[Wolfgang Denk]

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.

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

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

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

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

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

> +/*
> + * 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()) ?

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

> +static int align4(int v)
> +{
> +	return ((v + 3) / 4) * 4;
> +}

Don't we have macros to do this?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Doubt isn't the opposite of faith; it is an element of faith.
- Paul Tillich, German theologian and historian