Re: [U-Boot] [PATCH v6] socfpga: Add socfpga preloader signing to mkimage
Hello Wolfgang
Further to my last response
On Friday 28 February 2014 11:43:47 Charles Manning wrote:
On Friday 28 February 2014 10:57:21 Wolfgang Denk wrote:
+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()) ?
From what I see these get_aligned_xxx() functions and friends exist in target
space, not host land.
From my limited understanding of these matters, it is unwise to call
these functions here.
Are you Ok with that explanation? I will be fixing the other issues you raised
one way or another.
Best regards
Charles
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v6] socfpga: Add socfpga preloader signing to mkimage
Dear Charles,
In message 1393472979-7522-1-git-send-email-cdhmann...@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 000..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 cdhmann...@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] = {
+ 0x, 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: w...@denx.de
Doubt isn't the opposite of faith; it is an element of faith.
- Paul Tillich, German theologian and historian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v6] socfpga: Add socfpga preloader signing to mkimage
Hi Charles,
I hit error when trying to apply the patch
bash-3.2$ git apply signing.patch
fatal: corrupt patch at line 205
On Thu, 2014-02-27 at 16:49 +1300, Charles Manning 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.
This change automatically creates an appropriately signed preloader
from an SPL image.
The signed image includes a CRC which must, of course, be generated
with a CRC generator that the SoCFPGA boot ROM agrees with otherwise
the boot ROM will reject the image.
Unfortunately the CRC used in this boot ROM is not the same as the
Adler CRC in lib/crc32.c. Indeed the Adler code is not technically a
CRC but is more correctly described as a checksum.
Thus, the appropriate CRC generator is added to lib/ as crc32_alt.c.
Signed-off-by: Charles Manning cdhmann...@gmail.com
---
Changes for v3:
- Fix some coding style issues.
- Move from a standalone tool to the mkimgae framework.
Changes for v4:
- Fix more coding style issues.
- Fix typos in Makefile.
- Rebase on master (previous version was not on master, but on a
working socfpga branch).
Changes for v5:
- Fix more coding style issues.
- Add some more comments.
- Remove some unused defines.
- Move the local CRC32 code into lib/crc32_alt.c.
Changes for v6:
- Fix more coding style issues.
- Rejig socfpgaimage_vrec_header() function so that it has one return
path and does stricter size checks.
Note: Building a SOCFPGA preloader will currently not produe a working
image if built in master, but that is due to issues in building SPL,
not in this signer.
common/image.c |1 +
include/crc32_alt.h | 17 +++
include/image.h |1 +
lib/Makefile |1 +
lib/crc32_alt.c | 94 +
spl/Makefile |5 +
tools/Makefile |2 +
tools/crc32_alt.c|1 +
tools/imagetool.c|2 +
tools/imagetool.h|1 +
tools/socfpgaimage.c | 278
++
11 files changed, 403 insertions(+)
create mode 100644 include/crc32_alt.h
create mode 100644 lib/crc32_alt.c
create mode 100644 tools/crc32_alt.c
create mode 100644 tools/socfpgaimage.c
diff --git a/common/image.c b/common/image.c
index 9c6bec5..e7dc8cc 100644
--- a/common/image.c
+++ b/common/image.c
@@ -135,6 +135,7 @@ static const table_entry_t uimage_type[] = {
{ IH_TYPE_PBLIMAGE, pblimage, Freescale PBL Boot Image,},
{ IH_TYPE_RAMDISK,ramdisk,RAMDisk Image, },
{ IH_TYPE_SCRIPT, script, Script, },
+ { IH_TYPE_SOCFPGAIMAGE, socfpgaimage, Altera SOCFPGA
preloader,},
{ IH_TYPE_STANDALONE, standalone, Standalone Program, },
{ IH_TYPE_UBLIMAGE, ublimage, Davinci UBL image,},
{ IH_TYPE_MXSIMAGE, mxsimage, Freescale MXS Boot Image,},
diff --git a/include/crc32_alt.h b/include/crc32_alt.h
new file mode 100644
index 000..813d55d
--- /dev/null
+++ b/include/crc32_alt.h
@@ -0,0 +1,17 @@
+/*
+ * Copyright (C) 2014 Charles Manning cdhmann...@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.
+ */
+
+#ifndef __CRC32_ALT_H__
+#define __CRC32_ALT_H__
+
+#include stdint.h
+
+uint32_t crc32_alt(uint32_t crc, const void *_buf, int length);
+
+#endif
diff --git a/include/image.h b/include/image.h
index 6afd57b..bde31d9 100644
--- a/include/image.h
+++ b/include/image.h
@@ -215,6 +215,7 @@ struct lmb;
#define IH_TYPE_KERNEL_NOLOAD14 /* OS Kernel Image, can run
from any load address */
#define IH_TYPE_PBLIMAGE 15 /* Freescale PBL Boot Image */
#define IH_TYPE_MXSIMAGE 16 /* Freescale MXSBoot Image */
+#define IH_TYPE_SOCFPGAIMAGE 17 /* Altera SOCFPGA Preloader */
/*
* Compression Types
diff --git a/lib/Makefile b/lib/Makefile
index 8c483c9..7ee07a5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -52,6 +52,7 @@ obj-y += errno.o
obj-y += display_options.o
obj-$(CONFIG_BCH) += bch.o
obj-y += crc32.o
+obj-y += crc32_alt.o
obj-y += ctype.o
obj-y += div64.o
obj-y += hang.o
diff --git a/lib/crc32_alt.c b/lib/crc32_alt.c
new file mode 100644
index 000..e0db335
--- /dev/null
+++ b/lib/crc32_alt.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2014 Charles Manning cdhmann...@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.
+ */
+
+#include crc32_alt.h
+#include stdint.h
+
+static uint32_t crc_table[256] = {
+ 0x, 0x04c11db7, 0x09823b6e, 0x0d4326d9,
+ 0x130476dc,
Re: [U-Boot] [PATCH v6] socfpga: Add socfpga preloader signing to mkimage
On Friday 28 February 2014 10:57:21 Wolfgang Denk wrote:
Dear Charles,
In message 1393472979-7522-1-git-send-email-cdhmann...@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 000..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 cdhmann...@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] = {
+ 0x, 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)
+{
[U-Boot] [PATCH v6] socfpga: Add socfpga preloader signing to mkimage
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.
This change automatically creates an appropriately signed preloader
from an SPL image.
The signed image includes a CRC which must, of course, be generated
with a CRC generator that the SoCFPGA boot ROM agrees with otherwise
the boot ROM will reject the image.
Unfortunately the CRC used in this boot ROM is not the same as the
Adler CRC in lib/crc32.c. Indeed the Adler code is not technically a
CRC but is more correctly described as a checksum.
Thus, the appropriate CRC generator is added to lib/ as crc32_alt.c.
Signed-off-by: Charles Manning cdhmann...@gmail.com
---
Changes for v3:
- Fix some coding style issues.
- Move from a standalone tool to the mkimgae framework.
Changes for v4:
- Fix more coding style issues.
- Fix typos in Makefile.
- Rebase on master (previous version was not on master, but on a
working socfpga branch).
Changes for v5:
- Fix more coding style issues.
- Add some more comments.
- Remove some unused defines.
- Move the local CRC32 code into lib/crc32_alt.c.
Changes for v6:
- Fix more coding style issues.
- Rejig socfpgaimage_vrec_header() function so that it has one return
path and does stricter size checks.
Note: Building a SOCFPGA preloader will currently not produe a working
image if built in master, but that is due to issues in building SPL,
not in this signer.
common/image.c |1 +
include/crc32_alt.h | 17 +++
include/image.h |1 +
lib/Makefile |1 +
lib/crc32_alt.c | 94 +
spl/Makefile |5 +
tools/Makefile |2 +
tools/crc32_alt.c|1 +
tools/imagetool.c|2 +
tools/imagetool.h|1 +
tools/socfpgaimage.c | 278 ++
11 files changed, 403 insertions(+)
create mode 100644 include/crc32_alt.h
create mode 100644 lib/crc32_alt.c
create mode 100644 tools/crc32_alt.c
create mode 100644 tools/socfpgaimage.c
diff --git a/common/image.c b/common/image.c
index 9c6bec5..e7dc8cc 100644
--- a/common/image.c
+++ b/common/image.c
@@ -135,6 +135,7 @@ static const table_entry_t uimage_type[] = {
{ IH_TYPE_PBLIMAGE, pblimage, Freescale PBL Boot Image,},
{ IH_TYPE_RAMDISK,ramdisk,RAMDisk Image, },
{ IH_TYPE_SCRIPT, script, Script, },
+ { IH_TYPE_SOCFPGAIMAGE, socfpgaimage, Altera SOCFPGA
preloader,},
{ IH_TYPE_STANDALONE, standalone, Standalone Program, },
{ IH_TYPE_UBLIMAGE, ublimage, Davinci UBL image,},
{ IH_TYPE_MXSIMAGE, mxsimage, Freescale MXS Boot Image,},
diff --git a/include/crc32_alt.h b/include/crc32_alt.h
new file mode 100644
index 000..813d55d
--- /dev/null
+++ b/include/crc32_alt.h
@@ -0,0 +1,17 @@
+/*
+ * Copyright (C) 2014 Charles Manning cdhmann...@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.
+ */
+
+#ifndef __CRC32_ALT_H__
+#define __CRC32_ALT_H__
+
+#include stdint.h
+
+uint32_t crc32_alt(uint32_t crc, const void *_buf, int length);
+
+#endif
diff --git a/include/image.h b/include/image.h
index 6afd57b..bde31d9 100644
--- a/include/image.h
+++ b/include/image.h
@@ -215,6 +215,7 @@ struct lmb;
#define IH_TYPE_KERNEL_NOLOAD 14 /* OS Kernel Image, can run from any
load address */
#define IH_TYPE_PBLIMAGE 15 /* Freescale PBL Boot Image */
#define IH_TYPE_MXSIMAGE 16 /* Freescale MXSBoot Image */
+#define IH_TYPE_SOCFPGAIMAGE 17 /* Altera SOCFPGA Preloader */
/*
* Compression Types
diff --git a/lib/Makefile b/lib/Makefile
index 8c483c9..7ee07a5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -52,6 +52,7 @@ obj-y += errno.o
obj-y += display_options.o
obj-$(CONFIG_BCH) += bch.o
obj-y += crc32.o
+obj-y += crc32_alt.o
obj-y += ctype.o
obj-y += div64.o
obj-y += hang.o
diff --git a/lib/crc32_alt.c b/lib/crc32_alt.c
new file mode 100644
index 000..e0db335
--- /dev/null
+++ b/lib/crc32_alt.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2014 Charles Manning cdhmann...@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.
+ */
+
+#include crc32_alt.h
+#include stdint.h
+
+static uint32_t crc_table[256] = {
+ 0x, 0x04c11db7, 0x09823b6e, 0x0d4326d9,
+ 0x130476dc, 0x17c56b6b, 0x1a864db2, 0x1e475005,
+ 0x2608edb8, 0x22c9f00f, 0x2f8ad6d6, 0x2b4bcb61,
+ 0x350c9b64, 0x31cd86d3, 0x3c8ea00a, 0x384fbdbd,
+ 0x4c11db70, 0x48d0c6c7, 0x4593e01e, 0x4152fda9,
+ 0x5f15adac, 0x5bd4b01b, 0x569796c2, 0x52568b75,
+ 0x6a1936c8, 0x6ed82b7f, 0x639b0da6, 0x675a1011,
+
