[U-Boot] [PATCH v4] 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.
Signed-off-by: Charles Manning cdhmanning@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).
Note: Building a SOCFPGA preloader will currently not produe a working image, but that is due to issues in building SPL, not in this signer.
common/image.c | 1 + include/image.h | 1 + spl/Makefile | 8 ++ tools/Makefile | 1 + tools/imagetool.c | 2 + tools/imagetool.h | 1 + tools/socfpgaimage.c | 365 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 379 insertions(+) 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/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/spl/Makefile b/spl/Makefile index bf98024..90faaa6 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,14 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin + $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@ + + +ifdef CONFIG_SOCFPGA +ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin +endif + ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..59ff8d3 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -85,6 +85,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \ + socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y) diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type(); + /* Init Altera SOCFPGA support */ + init_socfpga_image_type(); }
/* diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..ca4369c --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,365 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@gmail.com + * + * Use as you see fit. + * + * Reference doc http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate. + * + * "Header" is a structure of the following format. + * this is positioned at 0x40. + * + * Endian is LSB. + * + * Offset Length Usage + * ----------------------- + * 0x40 4 Validation word 0x31305341 + * 0x44 1 Version (whatever, zero is fine) + * 0x45 1 Flags (unused, zero is fine) + * 0x46 2 Length (in units of u32, including the end checksum). + * 0x48 2 Zero + * 0x0A 2 Checksum over the heder. NB Not CRC32 + * + * At the end of the code we have a 32-bit CRC checksum over whole binary + * excluding the CRC. + * + * The image is typically padded out to 64k, because that is what is used + * to write the image to the boot medium. + * + * This uses the CRC32 calc out of the well known Apple + * crc32.c code. CRC32 algorithms do not produce the same results. + * We need this one. Sorry about the coade bloat. + * + * Copyright for the CRC code: + * + * COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or + * code or tables extracted from it, as desired without restriction. + * + */ + +#include "imagetool.h" +#include <image.h> + +#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0x0C +#define MAX_IMAGE_SIZE 0xFF00 +#define VALIDATION_WORD 0x31305341 +#define FILL_BYTE 0x00 +#define PADDED_SIZE 0x10000 + + +static uint8_t buffer[PADDED_SIZE]; + +/* + * The header checksum is just a very simple checksum over + * the header area. + * There is still a crc32 over the whole lot. + */ +static uint16_t hdr_checksum(const uint8_t *buf, int len) +{ + uint16_t ret = 0; + int i; + + for (i = 0; i < len; i++) { + ret += (((uint16_t) *buf) & 0xff); + buf++; + } + return ret; +} + +/* + * Some byte marshalling functions... + * + */ + +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; +} + +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; +} + +static int align4(int v) +{ + return ((v + 3) / 4) * 4; +} + +static void build_header(uint8_t *buf, + uint8_t version, + uint8_t flags, + uint16_t length_bytes) +{ + memset(buf, 0, HEADER_SIZE); + + load_le32(buf + 0, VALIDATION_WORD); + buf[4] = version; + buf[5] = flags; + load_le16(buf + 6, length_bytes/4); + load_le16(buf + 10, hdr_checksum(buf, 10)); +} + +/* Verify the header and return size of image */ +static int verify_header(const uint8_t *buf) +{ + if (get_le32(buf) != VALIDATION_WORD) + return -1; + if (get_le16(buf + 10) != hdr_checksum(buf, 10)) + return -1; + + return get_le16(buf+6) * 4; +} + +/* Local CRC32 function. Lifted from Apple CRC32. */ +static uint32_t crc_table[256] = { + 0x00000000, 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, + 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd, + 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039, + 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5, + 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81, + 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d, + 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49, + 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95, + 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1, + 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d, + 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae, + 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072, + 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16, + 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca, + 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde, + 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02, + 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066, + 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba, + 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e, + 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692, + 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6, + 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a, + 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e, + 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2, + 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686, + 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a, + 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637, + 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb, + 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f, + 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53, + 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47, + 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b, + 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff, + 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623, + 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7, + 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b, + 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f, + 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3, + 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7, + 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b, + 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f, + 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3, + 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640, + 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c, + 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8, + 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24, + 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30, + 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec, + 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088, + 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654, + 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0, + 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c, + 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18, + 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4, + 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0, + 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c, + 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668, + 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4 +}; + +static uint32_t local_crc32(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; +} + +/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf, + uint8_t version, uint8_t flags, + int len, int pad_64k) +{ + uint32_t crcval; + + /* Align the length up */ + len = align4(len); + + /* Build header, adding 4 bytes to length to hold the CRC32. */ + build_header(buf + HEADER_OFFSET, version, flags, len + 4); + + crcval = local_crc32(0, buf, len); + + load_le32(buf + len, crcval); + + if (!pad_64k) + return len + 4; + + return PADDED_SIZE; +} + +/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{ + int len; /* Including 32bit CRC */ + uint32_t crccalc; + + len = verify_header(buf + HEADER_OFFSET); + if (len < 0) + return -1; + if (len < HEADER_OFFSET || len > PADDED_SIZE) + return -1; + + /* Adjust length, removing CRC */ + len -= 4; + + crccalc = local_crc32(0, buf, len); + + if (get_le32(buf + len) != crccalc) + return -1; + + return 0; +} + +/* mkimage glue functions */ + +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size, + struct image_tool_params *params) +{ + if (image_size != PADDED_SIZE) + return -1; + + return verify_buffer(ptr); +} + +static void socfpgaimage_print_header(const void *ptr) +{ + if (verify_buffer(ptr) == 0) + printf("Looks like a sane SOCFPGA preloader\n"); + else + printf("Not a sane SOCFPGA preloader\n"); +} + +static int socfpgaimage_check_params(struct image_tool_params *params) +{ + /* Not sure if we should be accepting fflags */ + return (params->dflag && (params->fflag || params->lflag)) || + (params->fflag && (params->dflag || params->lflag)) || + (params->lflag && (params->dflag || params->fflag)); +} + +static int socfpgaimage_check_image_types(uint8_t type) +{ + if (type == IH_TYPE_SOCFPGAIMAGE) + return EXIT_SUCCESS; + else + return EXIT_FAILURE; +} + +/* To work in with the mkimage framework, we do some ugly stuff... + * + * First we prepend a fake header big enough to make the file 64k. + * When set_header is called, we fix this up by moving the image + * around in the buffer. + */ + +static int data_size; +#define fake_header_size (PADDED_SIZE - data_size) + +/* Use vrec_header to set up the fake header */ + +static int socfpgaimage_vrec_header(struct image_tool_params *params, + struct image_type_params *tparams) +{ + struct stat sbuf; + + if (!params->datafile || stat(params->datafile, &sbuf) < 0) + return 0; + + data_size = sbuf.st_size; + tparams->header_size = fake_header_size; + + return 0; +} + +static void socfpgaimage_set_header(void *ptr, struct stat *sbuf, int ifd, + struct image_tool_params *params) +{ + uint8_t *buf = (uint8_t *)ptr; + + /* At this stage we have the header_size dummy bytes followed by + * PADDED_SIZE-header_size image bytes. + * We need to fix the buffer by moving the image bytes back to + * the beginning of the buffer, then actually do the signing stuff... + */ + memmove(buf, buf + fake_header_size, data_size); + memset(buf + data_size, 0, fake_header_size); + + sign_buffer(buf, 0, 0, data_size, 0); +} + +/* + * socfpgaimage parameters + */ +static struct image_type_params socfpgaimage_params = { + .name = "Altera SOCFPGA preloader support", + .vrec_header = socfpgaimage_vrec_header, + .header_size = 0, + .hdr = (void *)buffer, + .check_image_type = socfpgaimage_check_image_types, + .verify_header = socfpgaimage_verify_header, + .print_header = socfpgaimage_print_header, + .set_header = socfpgaimage_set_header, + .check_params = socfpgaimage_check_params, +}; + +void init_socfpga_image_type(void) +{ + register_image_type(&socfpgaimage_params); +} +
Dear Charles,
In message 1393194939-29786-1-git-send-email-cdhmanning@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.
This change automatically creates an appropriately signed preloader from an SPL image.
Signed-off-by: Charles Manning cdhmanning@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).
There may be perfectly valid reasons why you might decide to ingore a review comments - sch comments may be wrong, too, after all. But in such a case it is always a good idea to provide feedback to the reviewer why you decided not to follow his advice. Otherwise he might think you just missed or ignored the comment.
And this is what is happeneing (again) in your patch...
diff --git a/spl/Makefile b/spl/Makefile index bf98024..90faaa6 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,14 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
One blank line is sufficient.
I asked before: "socfpga-signed-preloader.bin" is a terribly long name. Can we find a better one?
+ifdef CONFIG_SOCFPGA +ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin +endif
I asked before: Can we not use "ALL-$(CONFIG_SOCFPGA)" and avoid the ifdef ?
- Note this doc is not entirely accurate.
I aseked before: Would you care to explain the errors in the document that might cause problems to the reader?
Noting that something contains errors without mentioning what these are is really not helpful.
- This uses the CRC32 calc out of the well known Apple
- crc32.c code. CRC32 algorithms do not produce the same results.
- We need this one. Sorry about the coade bloat.
Both Gerhard and me asked before: Why exactly do we need another implementation of CRC32. We already have some - why cannot we use these?
- Copyright for the CRC code:
- COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or
- code or tables extracted from it, as desired without restriction.
I asked before: Please provide _exact_ reference. See http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign for instructions how to do this.
I also commented before: If you really need this copied code (i. e. you canot use one of the already existing implementations of CRC32 - which I seriously doubt), then better move this into a separate file, and assign a separate license ID tag for it.
I also asked this before: I cannot find a license ID tag in your new code. Please add.
+/* To work in with the mkimage framework, we do some ugly stuff...
- First we prepend a fake header big enough to make the file 64k.
- When set_header is called, we fix this up by moving the image
- around in the buffer.
- */
Also asked before: Incorrect multiline comment style. Please fix globally.
It turns out that you basically ignored nearly ALL of trhe review comments which I made before, twice.
It is an exhausting experience to deal with your patches :-(
Wolfgang Denk
Hello Wolfgang On Monday 24 February 2014 19:48:36 Wolfgang Denk wrote:
Dear Charles,
In message 1393194939-29786-1-git-send-email-cdhmanning@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.
This change automatically creates an appropriately signed preloader from an SPL image.
Signed-off-by: Charles Manning cdhmanning@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).
There may be perfectly valid reasons why you might decide to ingore a review comments - sch comments may be wrong, too, after all. But in such a case it is always a good idea to provide feedback to the reviewer why you decided not to follow his advice. Otherwise he might think you just missed or ignored the comment.
I am sorry, I must have missed some of the comments. I have intended to implement them all, except one by Gerhard.
And this is what is happeneing (again) in your patch...
diff --git a/spl/Makefile b/spl/Makefile index bf98024..90faaa6 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,14 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
One blank line is sufficient.
Ok, a blank line. I can delete that.
I asked before: "socfpga-signed-preloader.bin" is a terribly long name. Can we find a better one?
+ifdef CONFIG_SOCFPGA +ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin +endif
I asked before: Can we not use "ALL-$(CONFIG_SOCFPGA)" and avoid the ifdef ?
I am sorry, I had developed this code in a different (older) branch where socfpga actually works. It is broken in master.
This I shall fix.
- Reference doc
http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate.
I aseked before: Would you care to explain the errors in the document that might cause problems to the reader?
Noting that something contains errors without mentioning what these are is really not helpful.
Ok, I shall mention the errors pertinent to the code. Other errors I shall ignore.
- This uses the CRC32 calc out of the well known Apple
- crc32.c code. CRC32 algorithms do not produce the same results.
- We need this one. Sorry about the coade bloat.
Both Gerhard and me asked before: Why exactly do we need another implementation of CRC32. We already have some - why cannot we use these?
Well I would have thought that obvious from the comment I put in the code, but email is an imperfect communications medium, so I shall explain in further detail here.
As I am sure you are aware, there are many different crc32 calculations. Indeed Wikipedia lists 5 and there are most likely others.
Even when they use the same polynomial, they give differences when you stuff the bits through the shift register lsb-first or msb-first.
Now from what I see looking through the u-boot lib/ directory u-boot provides just one crc32 version - Adler (and a bit flipped version thereof for use by jffs2). If there are others I did not see them.
Now as I expect you are aware, the purpose of a signer is to massage the code so that it is in a form that the boot ROM accepts. One of those criteria is that the crc in the code matches the crc the boot ROM is expecting. If not, the boot ROM refuses to execute the code.
It would be nice to use the Adler code. Indeed this is my favourite crc. However, this is not what the boot ROM wants.
The boot ROM does not enter into rational discussions, so we must, unfortunately, bow to its whims. If it wants a different CRC calculation then we must supply it.
I did much experimentation to find the crc that worked. I tried the zlib crc - no luck. I tried different many things until I found something that worked.
- Copyright for the CRC code:
- COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or
- code or tables extracted from it, as desired without restriction.
I asked before: Please provide _exact_ reference. See http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sig n for instructions how to do this.
As it was, I had attributed this incorrectly. This is a file I generated myself, though I had used this as a reference during my research. The values will look the same as some other code floating out there, but that is because that is the way things have to be.
I shall fix this wen I make another file.
I also commented before: If you really need this copied code (i. e. you canot use one of the already existing implementations of CRC32 - which I seriously doubt), then better move this into a separate file, and assign a separate license ID tag for it.
You say "already exiting implementations". I see only one: lib/crc32.c. Perhaps I am not looking in the right place?
I shall split this code out and call it alt_crc32 and put it in lib with one of those proxy files in tools.
I also asked this before: I cannot find a license ID tag in your new code. Please add.
Ok.
+/* To work in with the mkimage framework, we do some ugly stuff...
- First we prepend a fake header big enough to make the file 64k.
- When set_header is called, we fix this up by moving the image
- around in the buffer.
- */
Also asked before: Incorrect multiline comment style. Please fix globally.
Ok.
I am sorry I missed some of your comments earlier.
Regards
Charles
Hello Wolfgang
I have some further observations to my last email...
Your input would be vastly appreciated.
Please see below.
On Tue, Feb 25, 2014 at 8:18 AM, Charles Manning cdhmanning@gmail.comwrote:
Hello Wolfgang On Monday 24 February 2014 19:48:36 Wolfgang Denk wrote:
Dear Charles,
In message 1393194939-29786-1-git-send-email-cdhmanning@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.
This change automatically creates an appropriately signed preloader from an SPL image.
Signed-off-by: Charles Manning cdhmanning@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).
There may be perfectly valid reasons why you might decide to ingore a review comments - sch comments may be wrong, too, after all. But in such a case it is always a good idea to provide feedback to the reviewer why you decided not to follow his advice. Otherwise he might think you just missed or ignored the comment.
I am sorry, I must have missed some of the comments. I have intended to implement them all, except one by Gerhard.
And this is what is happeneing (again) in your patch...
diff --git a/spl/Makefile b/spl/Makefile index bf98024..90faaa6 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,14 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
One blank line is sufficient.
Ok, a blank line. I can delete that.
I asked before: "socfpga-signed-preloader.bin" is a terribly long name. Can we find a better one?
+ifdef CONFIG_SOCFPGA +ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin +endif
I asked before: Can we not use "ALL-$(CONFIG_SOCFPGA)" and avoid the ifdef ?
I am sorry, I had developed this code in a different (older) branch where socfpga actually works. It is broken in master.
This I shall fix.
I can certainly avoid the ifdef, but there is already another one three lines down for the SAMSUNG case:
ifdef CONFIG_SOCFPGA ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin endif
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
It seems odd to me that you would want different conventions in the same Makefile, but if that is what you want then that is what you will get.
- Reference doc
http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate.
I aseked before: Would you care to explain the errors in the document that might cause problems to the reader?
Noting that something contains errors without mentioning what these are is really not helpful.
Ok, I shall mention the errors pertinent to the code. Other errors I shall ignore.
- This uses the CRC32 calc out of the well known Apple
- crc32.c code. CRC32 algorithms do not produce the same results.
- We need this one. Sorry about the coade bloat.
Both Gerhard and me asked before: Why exactly do we need another implementation of CRC32. We already have some - why cannot we use these?
Well I would have thought that obvious from the comment I put in the code, but email is an imperfect communications medium, so I shall explain in further detail here.
As I am sure you are aware, there are many different crc32 calculations. Indeed Wikipedia lists 5 and there are most likely others.
Even when they use the same polynomial, they give differences when you stuff the bits through the shift register lsb-first or msb-first.
Now from what I see looking through the u-boot lib/ directory u-boot provides just one crc32 version - Adler (and a bit flipped version thereof for use by jffs2). If there are others I did not see them.
Now as I expect you are aware, the purpose of a signer is to massage the code so that it is in a form that the boot ROM accepts. One of those criteria is that the crc in the code matches the crc the boot ROM is expecting. If not, the boot ROM refuses to execute the code.
It would be nice to use the Adler code. Indeed this is my favourite crc. However, this is not what the boot ROM wants.
The boot ROM does not enter into rational discussions, so we must, unfortunately, bow to its whims. If it wants a different CRC calculation then we must supply it.
I did much experimentation to find the crc that worked. I tried the zlib crc - no luck. I tried different many things until I found something that worked.
The actual table values I am using are also found in lib/bzlib_crctable.c
This is, however, not exposed but is a private thing within bzlib.
The best choice would possibly be to expose this with a new crc function, but that means dragging bzlib (or parts thereof) into mkimage or at least putting "hooks" into bzlib that were never intended to be there.
The alternative is to maybe just add a new alt_crc.c to lib.
- Copyright for the CRC code:
- COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or
- code or tables extracted from it, as desired without restriction.
I asked before: Please provide _exact_ reference. See
http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sig
n for instructions how to do this.
As it was, I had attributed this incorrectly. This is a file I generated myself, though I had used this as a reference during my research. The values will look the same as some other code floating out there, but that is because that is the way things have to be.
I shall fix this wen I make another file.
I also commented before: If you really need this copied code (i. e. you canot use one of the already existing implementations of CRC32 - which I seriously doubt), then better move this into a separate file, and assign a separate license ID tag for it.
You say "already exiting implementations". I see only one: lib/crc32.c. Perhaps I am not looking in the right place?
I shall split this code out and call it alt_crc32 and put it in lib with one of those proxy files in tools.
I also asked this before: I cannot find a license ID tag in your new code. Please add.
Ok.
+/* To work in with the mkimage framework, we do some ugly stuff...
- First we prepend a fake header big enough to make the file 64k.
- When set_header is called, we fix this up by moving the image
- around in the buffer.
- */
Also asked before: Incorrect multiline comment style. Please fix globally.
Ok.
I am sorry I missed some of your comments earlier.
Regards
Charles
Dear Charles,
In message CAE21AQp2GSPedrDakio1wPa3vGTwjd-3D1wSQM0WhG9r-8Bzcg@mail.gmail.com you wrote:
I asked before: Can we not use "ALL-$(CONFIG_SOCFPGA)" and avoid the ifdef ?
...
I can certainly avoid the ifdef, but there is already another one three lines down for the SAMSUNG case:
ifdef CONFIG_SOCFPGA ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin endif
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
It seems odd to me that you would want different conventions in the same Makefile, but if that is what you want then that is what you will get.
Existing poor code should not be used as reference for adding new bad code. If possible, it should be imprived -patches are welcome. In any case, please avoid the issues in new code.
Best regards,
Wolfgang Denk
I can certainly avoid the ifdef, but there is already another one three lines down for the SAMSUNG case:
ifdef CONFIG_SOCFPGA ALL-y += $(OBJTREE)/socfpga-signed-preloader.bin endif
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
It seems odd to me that you would want different conventions in the same Makefile, but if that is what you want then that is what you will get.
Existing poor code should not be used as reference for adding new bad code. If possible, it should be imprived -patches are welcome. In any case, please avoid the issues in new code.
Thank you for that, though it is not obvious which is poor code and which is good code and sometimes one must follow what exists for guidance.
I have cleared this up in the latest version that I posted.
-- Charles
Dear Charles,
sorry, I only send part of the message. Here is the rest.
In message CAE21AQp2GSPedrDakio1wPa3vGTwjd-3D1wSQM0WhG9r-8Bzcg@mail.gmail.com you wrote:
Both Gerhard and me asked before: Why exactly do we need another implementation of CRC32. We already have some - why cannot we use these?
...
The actual table values I am using are also found in lib/bzlib_crctable.c
So could you use the bzlib implementation for your purposes?
This is, however, not exposed but is a private thing within bzlib.
The best choice would possibly be to expose this with a new crc function, but that means dragging bzlib (or parts thereof) into mkimage or at least putting "hooks" into bzlib that were never intended to be there.
The alternative is to maybe just add a new alt_crc.c to lib.
Code duplication is always bad. Please factor out common code parts.
- Copyright for the CRC code:
- COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or
- code or tables extracted from it, as desired without restriction.
I asked before: Please provide _exact_ reference. See
...
Exact reference includes some URL plus version id, etc.
I also commented before: If you really need this copied code (i. e. you canot use one of the already existing implementations of CRC32 - which I seriously doubt), then better move this into a separate file, and assign a separate license ID tag for it.
You say "already exiting implementations". I see only one: lib/crc32.c. Perhaps I am not looking in the right place?
I shall split this code out and call it alt_crc32 and put it in lib with one of those proxy files in tools.
"alt" is a really bad name - you mentioned yourself that ther eis not only one alternative, so please rather use a descriptive name.
Best regards,
Wolfgang Denk
On Friday 28 February 2014 10:23:11 Wolfgang Denk wrote:
Dear Charles,
sorry, I only send part of the message. Here is the rest.
In message
CAE21AQp2GSPedrDakio1wPa3vGTwjd-3D1wSQM0WhG9r-8Bzcg@mail.gmail.com you wrote:
Both Gerhard and me asked before: Why exactly do we need another implementation of CRC32. We already have some - why cannot we use these?
...
The actual table values I am using are also found in lib/bzlib_crctable.c
So could you use the bzlib implementation for your purposes?
This is, however, not exposed but is a private thing within bzlib.
The best choice would possibly be to expose this with a new crc function, but that means dragging bzlib (or parts thereof) into mkimage or at least putting "hooks" into bzlib that were never intended to be there.
The alternative is to maybe just add a new alt_crc.c to lib.
Code duplication is always bad. Please factor out common code parts.
Ok, I will write a small wrapper thing that accesses the bzlib table.
That table will now be compiled in if you use either bzlib or the alternative crc32.
Does that sound OK?
- Copyright for the CRC code:
- COPYRIGHT (C) 1986 Gary S. Brown. You may use this program, or
- code or tables extracted from it, as desired without
restriction.
I asked before: Please provide _exact_ reference. See
...
Exact reference includes some URL plus version id, etc.
That stuff is now gone
I also commented before: If you really need this copied code (i. e. you canot use one of the already existing implementations of CRC32 - which I seriously doubt), then better move this into a separate file, and assign a separate license ID tag for it.
You say "already exiting implementations". I see only one: lib/crc32.c. Perhaps I am not looking in the right place?
I shall split this code out and call it alt_crc32 and put it in lib with one of those proxy files in tools.
"alt" is a really bad name - you mentioned yourself that ther eis not only one alternative, so please rather use a descriptive name.
Is bzlib_crc32 Ok?
Thank you for your feedback.
-- CHarles
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 cdhmanning@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.
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 | 276 ++++++++++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 401 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 0000000..813d55d --- /dev/null +++ b/include/crc32_alt.h @@ -0,0 +1,17 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@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 0000000..e0db335 --- /dev/null +++ b/lib/crc32_alt.c @@ -0,0 +1,94 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@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] = { + 0x00000000, 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, + 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd, + 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039, + 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5, + 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81, + 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d, + 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49, + 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95, + 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1, + 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d, + 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae, + 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072, + 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16, + 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca, + 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde, + 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02, + 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066, + 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba, + 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e, + 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692, + 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6, + 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a, + 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e, + 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2, + 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686, + 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a, + 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637, + 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb, + 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f, + 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53, + 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47, + 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b, + 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff, + 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623, + 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7, + 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b, + 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f, + 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3, + 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7, + 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b, + 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f, + 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3, + 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640, + 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c, + 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8, + 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24, + 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30, + 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec, + 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088, + 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654, + 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0, + 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c, + 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18, + 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4, + 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0, + 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c, + 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668, + 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4, +}; + +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; +} diff --git a/spl/Makefile b/spl/Makefile index bf98024..bce9055 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,11 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin + $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@ + +ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin + ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..46af0b1 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -71,6 +71,7 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \ crc32.o \ + crc32_alt.o \ default_image.o \ fit_image.o \ image-fit.o \ @@ -85,6 +86,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \ + socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y) diff --git a/tools/crc32_alt.c b/tools/crc32_alt.c new file mode 100644 index 0000000..3faa222 --- /dev/null +++ b/tools/crc32_alt.c @@ -0,0 +1 @@ +#include "../lib/crc32_alt.c" diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type(); + /* Init Altera SOCFPGA support */ + init_socfpga_image_type(); }
/* diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..f9df9ae --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,276 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@gmail.com + * + * SPDX-License-Identifier: GPL-2.0+ + * + * Reference doc http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate. Of particular interest to us is the + * "header" length field being in U32s and not bytes. + * + * "Header" is a structure of the following format. + * this is positioned at 0x40. + * + * Endian is LSB. + * + * Offset Length Usage + * ----------------------- + * 0x40 4 Validation word 0x31305341 + * 0x44 1 Version (whatever, zero is fine) + * 0x45 1 Flags (unused, zero is fine) + * 0x46 2 Length (in units of u32, including the end checksum). + * 0x48 2 Zero + * 0x0A 2 Checksum over the heder. NB Not CRC32 + * + * At the end of the code we have a 32-bit CRC checksum over whole binary + * excluding the CRC. + * + * Note that the CRC used here is **not** the zlib/Adler crc32. It is the + * CRC-32 used in bzip2, ethernet and elsewhere. + * + * The image is padded out to 64k, because that is what is + * typically used to write the image to the boot medium. + */ + +#include <crc32_alt.h> +#include "imagetool.h" +#include <image.h> + +#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0x0C +#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000 + +static uint8_t buffer[PADDED_SIZE]; + +/* + * The header checksum is just a very simple checksum over + * the header area. + * There is still a crc32 over the whole lot. + */ +static uint16_t hdr_checksum(const uint8_t *buf, int len) +{ + uint16_t ret = 0; + int i; + + for (i = 0; i < len; i++) { + ret += (((uint16_t) *buf) & 0xff); + buf++; + } + return ret; +} + +/* + * 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; +} + +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; +} + +static int align4(int v) +{ + return ((v + 3) / 4) * 4; +} + +static void build_header(uint8_t *buf, + uint8_t version, + uint8_t flags, + uint16_t length_bytes) +{ + memset(buf, 0, HEADER_SIZE); + + load_le32(buf + 0, VALIDATION_WORD); + buf[4] = version; + buf[5] = flags; + load_le16(buf + 6, length_bytes/4); + load_le16(buf + 10, hdr_checksum(buf, 10)); +} + +/* + * Perform a rudimentary verification of header and return + * size of image. + */ +static int verify_header(const uint8_t *buf) +{ + if (get_le32(buf) != VALIDATION_WORD) + return -1; + if (get_le16(buf + 10) != hdr_checksum(buf, 10)) + return -1; + + return get_le16(buf+6) * 4; +} + +/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf, + uint8_t version, uint8_t flags, + int len, int pad_64k) +{ + uint32_t crcval; + + /* Align the length up */ + len = align4(len); + + /* Build header, adding 4 bytes to length to hold the CRC32. */ + build_header(buf + HEADER_OFFSET, version, flags, len + 4); + + crcval = crc32_alt(0, buf, len); + + load_le32(buf + len, crcval); + + if (!pad_64k) + return len + 4; + + return PADDED_SIZE; +} + +/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{ + int len; /* Including 32bit CRC */ + uint32_t crccalc; + + len = verify_header(buf + HEADER_OFFSET); + if (len < 0) + return -1; + if (len < HEADER_OFFSET || len > PADDED_SIZE) + return -1; + + /* Adjust length, removing CRC */ + len -= 4; + + crccalc = crc32_alt(0, buf, len); + + if (get_le32(buf + len) != crccalc) + return -1; + + return 0; +} + +/* mkimage glue functions */ +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size, + struct image_tool_params *params) +{ + if (image_size != PADDED_SIZE) + return -1; + + return verify_buffer(ptr); +} + +static void socfpgaimage_print_header(const void *ptr) +{ + if (verify_buffer(ptr) == 0) + printf("Looks like a sane SOCFPGA preloader\n"); + else + printf("Not a sane SOCFPGA preloader\n"); +} + +static int socfpgaimage_check_params(struct image_tool_params *params) +{ + /* Not sure if we should be accepting fflags */ + return (params->dflag && (params->fflag || params->lflag)) || + (params->fflag && (params->dflag || params->lflag)) || + (params->lflag && (params->dflag || params->fflag)); +} + +static int socfpgaimage_check_image_types(uint8_t type) +{ + if (type == IH_TYPE_SOCFPGAIMAGE) + return EXIT_SUCCESS; + else + return EXIT_FAILURE; +} + +/* + * To work in with the mkimage framework, we do some ugly stuff... + * + * First, socfpgaimage_vrec_header() is called. + * We prepend a fake header big enough to make the file PADDED_SIZE. + * This gives us enough space to do what we want later. + * + * Next, socfpgaimage_set_header() is called. + * We fix up the buffer by moving the image to the start of the buffer. + * We now have some room to do what we need (add CRC and padding). + */ + +static int data_size; +#define FAKE_HEADER_SIZE (PADDED_SIZE - data_size) + +static int socfpgaimage_vrec_header(struct image_tool_params *params, + struct image_type_params *tparams) +{ + struct stat sbuf; + + if (!params->datafile || stat(params->datafile, &sbuf) < 0) + return 0; + + data_size = sbuf.st_size; + tparams->header_size = FAKE_HEADER_SIZE; + + return 0; +} + +static void socfpgaimage_set_header(void *ptr, struct stat *sbuf, int ifd, + struct image_tool_params *params) +{ + uint8_t *buf = (uint8_t *)ptr; + + /* + * This function is called after vrec_header() has been called. + * At this stage we have the FAKE_HEADER_SIZE dummy bytes followed by + * data_size image bytes. Total = PADDED_SIZE. + * We need to fix the buffer by moving the image bytes back to + * the beginning of the buffer, then actually do the signing stuff... + */ + memmove(buf, buf + FAKE_HEADER_SIZE, data_size); + memset(buf + data_size, 0, FAKE_HEADER_SIZE); + + sign_buffer(buf, 0, 0, data_size, 0); +} + +static struct image_type_params socfpgaimage_params = { + .name = "Altera SOCFPGA preloader support", + .vrec_header = socfpgaimage_vrec_header, + .header_size = 0, /* This will be modified by vrec_header() */ + .hdr = (void *)buffer, + .check_image_type = socfpgaimage_check_image_types, + .verify_header = socfpgaimage_verify_header, + .print_header = socfpgaimage_print_header, + .set_header = socfpgaimage_set_header, + .check_params = socfpgaimage_check_params, +}; + +void init_socfpga_image_type(void) +{ + register_image_type(&socfpgaimage_params); +}
On 02/26/2014 02:17 AM, 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 cdhmanning@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.
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 | 276 ++++++++++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 401 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 0000000..813d55d --- /dev/null +++ b/include/crc32_alt.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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 0000000..e0db335 --- /dev/null +++ b/lib/crc32_alt.c @@ -0,0 +1,94 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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] = {
- 0x00000000, 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,
- 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd,
- 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039,
- 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5,
- 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81,
- 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d,
- 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49,
- 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95,
- 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1,
- 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d,
- 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae,
- 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072,
- 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16,
- 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca,
- 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde,
- 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02,
- 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066,
- 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba,
- 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e,
- 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692,
- 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6,
- 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a,
- 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e,
- 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2,
- 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686,
- 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a,
- 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637,
- 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb,
- 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f,
- 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53,
- 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47,
- 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b,
- 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff,
- 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623,
- 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7,
- 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b,
- 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f,
- 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3,
- 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7,
- 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b,
- 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f,
- 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3,
- 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640,
- 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c,
- 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8,
- 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24,
- 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30,
- 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec,
- 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088,
- 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654,
- 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0,
- 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c,
- 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18,
- 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4,
- 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0,
- 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c,
- 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668,
- 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4,
+};
+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;
+} diff --git a/spl/Makefile b/spl/Makefile index bf98024..bce9055 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,11 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..46af0b1 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -71,6 +71,7 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \ crc32.o \
crc32_alt.o \ default_image.o \ fit_image.o \ image-fit.o \@@ -85,6 +86,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/crc32_alt.c b/tools/crc32_alt.c new file mode 100644 index 0000000..3faa222 --- /dev/null +++ b/tools/crc32_alt.c @@ -0,0 +1 @@ +#include "../lib/crc32_alt.c" diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type();
- /* Init Altera SOCFPGA support */
- init_socfpga_image_type();
}
/* diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..f9df9ae --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,276 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- Note this doc is not entirely accurate. Of particular interest to us is the
- "header" length field being in U32s and not bytes.
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end checksum).
- 0x48 2 Zero
- 0x0A 2 Checksum over the heder. NB Not CRC32
- At the end of the code we have a 32-bit CRC checksum over whole binary
- excluding the CRC.
- Note that the CRC used here is **not** the zlib/Adler crc32. It is the
- CRC-32 used in bzip2, ethernet and elsewhere.
- The image is padded out to 64k, because that is what is
- typically used to write the image to the boot medium.
- */
+#include <crc32_alt.h> +#include "imagetool.h" +#include <image.h>
+#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0x0C
just 0xC here
+#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000
+static uint8_t buffer[PADDED_SIZE];
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
+/*
- 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;
+}
+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;
+}
+static int align4(int v) +{
- return ((v + 3) / 4) * 4;
+}
+static void build_header(uint8_t *buf,
uint8_t version,uint8_t flags,uint16_t length_bytes)+{
- memset(buf, 0, HEADER_SIZE);
- load_le32(buf + 0, VALIDATION_WORD);
- buf[4] = version;
- buf[5] = flags;
- load_le16(buf + 6, length_bytes/4);
- load_le16(buf + 10, hdr_checksum(buf, 10));
+}
+/*
- Perform a rudimentary verification of header and return
- size of image.
- */
+static int verify_header(const uint8_t *buf) +{
- if (get_le32(buf) != VALIDATION_WORD)
return -1;- if (get_le16(buf + 10) != hdr_checksum(buf, 10))
return -1;- return get_le16(buf+6) * 4;
buf + 6
+}
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t crcval;
- /* Align the length up */
- len = align4(len);
- /* Build header, adding 4 bytes to length to hold the CRC32. */
- build_header(buf + HEADER_OFFSET, version, flags, len + 4);
- crcval = crc32_alt(0, buf, len);
- load_le32(buf + len, crcval);
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{
- int len; /* Including 32bit CRC */
- uint32_t crccalc;
- len = verify_header(buf + HEADER_OFFSET);
- if (len < 0)
return -1;- if (len < HEADER_OFFSET || len > PADDED_SIZE)
return -1;- /* Adjust length, removing CRC */
- len -= 4;
- crccalc = crc32_alt(0, buf, len);
- if (get_le32(buf + len) != crccalc)
return -1;- return 0;
+}
+/* mkimage glue functions */ +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size,
struct image_tool_params *params)+{
- if (image_size != PADDED_SIZE)
return -1;- return verify_buffer(ptr);
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
+static int socfpgaimage_check_params(struct image_tool_params *params) +{
- /* Not sure if we should be accepting fflags */
- return (params->dflag && (params->fflag || params->lflag)) ||
(params->fflag && (params->dflag || params->lflag)) ||(params->lflag && (params->dflag || params->fflag));+}
+static int socfpgaimage_check_image_types(uint8_t type) +{
- if (type == IH_TYPE_SOCFPGAIMAGE)
return EXIT_SUCCESS;- else
this else is not needed here.
return EXIT_FAILURE;+}
+/*
- To work in with the mkimage framework, we do some ugly stuff...
- First, socfpgaimage_vrec_header() is called.
on space here.
- We prepend a fake header big enough to make the file PADDED_SIZE.
- This gives us enough space to do what we want later.
- Next, socfpgaimage_set_header() is called.
- We fix up the buffer by moving the image to the start of the buffer.
- We now have some room to do what we need (add CRC and padding).
- */
+static int data_size; +#define FAKE_HEADER_SIZE (PADDED_SIZE - data_size)
+static int socfpgaimage_vrec_header(struct image_tool_params *params,
struct image_type_params *tparams)+{
- struct stat sbuf;
- if (!params->datafile || stat(params->datafile, &sbuf) < 0)
return 0;- data_size = sbuf.st_size;
- tparams->header_size = FAKE_HEADER_SIZE;
- return 0;
That's quite weird that you are returning 0 for both cases.
Thanks, Michal
On Wednesday 26 February 2014 19:16:37 Michal Simek wrote:
On 02/26/2014 02:17 AM, 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 cdhmanning@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.
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 | 276 ++++++++++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 401 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 0000000..813d55d --- /dev/null +++ b/include/crc32_alt.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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 0000000..e0db335 --- /dev/null +++ b/lib/crc32_alt.c @@ -0,0 +1,94 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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] = {
- 0x00000000, 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,
- 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd,
- 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039,
- 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5,
- 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81,
- 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d,
- 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49,
- 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95,
- 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1,
- 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d,
- 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae,
- 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072,
- 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16,
- 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca,
- 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde,
- 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02,
- 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066,
- 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba,
- 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e,
- 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692,
- 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6,
- 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a,
- 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e,
- 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2,
- 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686,
- 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a,
- 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637,
- 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb,
- 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f,
- 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53,
- 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47,
- 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b,
- 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff,
- 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623,
- 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7,
- 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b,
- 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f,
- 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3,
- 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7,
- 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b,
- 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f,
- 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3,
- 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640,
- 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c,
- 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8,
- 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24,
- 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30,
- 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec,
- 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088,
- 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654,
- 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0,
- 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c,
- 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18,
- 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4,
- 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0,
- 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c,
- 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668,
- 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4,
+};
+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;
+} diff --git a/spl/Makefile b/spl/Makefile index bf98024..bce9055 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,11 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..46af0b1 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -71,6 +71,7 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \ crc32.o \
crc32_alt.o \ default_image.o \ fit_image.o \ image-fit.o \@@ -85,6 +86,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/crc32_alt.c b/tools/crc32_alt.c new file mode 100644 index 0000000..3faa222 --- /dev/null +++ b/tools/crc32_alt.c @@ -0,0 +1 @@ +#include "../lib/crc32_alt.c" diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type();
- /* Init Altera SOCFPGA support */
- init_socfpga_image_type();
}
/* diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..f9df9ae --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,276 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- Reference doc
http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate. Of particular interest to us is the +
- "header" length field being in U32s and not bytes.
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end
checksum). + * 0x48 2 Zero
- 0x0A 2 Checksum over the heder. NB Not CRC32
- At the end of the code we have a 32-bit CRC checksum over whole
binary + * excluding the CRC.
- Note that the CRC used here is **not** the zlib/Adler crc32. It is
the + * CRC-32 used in bzip2, ethernet and elsewhere.
- The image is padded out to 64k, because that is what is
- typically used to write the image to the boot medium.
- */
+#include <crc32_alt.h> +#include "imagetool.h" +#include <image.h>
+#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0x0C
just 0xC here
+#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000
+static uint8_t buffer[PADDED_SIZE];
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
+/*
- 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;
+}
+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;
+}
+static int align4(int v) +{
- return ((v + 3) / 4) * 4;
+}
+static void build_header(uint8_t *buf,
uint8_t version,uint8_t flags,uint16_t length_bytes)+{
- memset(buf, 0, HEADER_SIZE);
- load_le32(buf + 0, VALIDATION_WORD);
- buf[4] = version;
- buf[5] = flags;
- load_le16(buf + 6, length_bytes/4);
- load_le16(buf + 10, hdr_checksum(buf, 10));
+}
+/*
- Perform a rudimentary verification of header and return
- size of image.
- */
+static int verify_header(const uint8_t *buf) +{
- if (get_le32(buf) != VALIDATION_WORD)
return -1;- if (get_le16(buf + 10) != hdr_checksum(buf, 10))
return -1;- return get_le16(buf+6) * 4;
buf + 6
+}
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t crcval;
- /* Align the length up */
- len = align4(len);
- /* Build header, adding 4 bytes to length to hold the CRC32. */
- build_header(buf + HEADER_OFFSET, version, flags, len + 4);
- crcval = crc32_alt(0, buf, len);
- load_le32(buf + len, crcval);
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{
- int len; /* Including 32bit CRC */
- uint32_t crccalc;
- len = verify_header(buf + HEADER_OFFSET);
- if (len < 0)
return -1;- if (len < HEADER_OFFSET || len > PADDED_SIZE)
return -1;- /* Adjust length, removing CRC */
- len -= 4;
- crccalc = crc32_alt(0, buf, len);
- if (get_le32(buf + len) != crccalc)
return -1;- return 0;
+}
+/* mkimage glue functions */ +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size, + struct image_tool_params *params) +{
- if (image_size != PADDED_SIZE)
return -1;- return verify_buffer(ptr);
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
+static int socfpgaimage_check_params(struct image_tool_params *params) +{
- /* Not sure if we should be accepting fflags */
- return (params->dflag && (params->fflag || params->lflag)) ||
(params->fflag && (params->dflag || params->lflag)) ||(params->lflag && (params->dflag || params->fflag));+}
+static int socfpgaimage_check_image_types(uint8_t type) +{
- if (type == IH_TYPE_SOCFPGAIMAGE)
return EXIT_SUCCESS;- else
this else is not needed here.
return EXIT_FAILURE;+}
+/*
- To work in with the mkimage framework, we do some ugly stuff...
- First, socfpgaimage_vrec_header() is called.
on space here.
- We prepend a fake header big enough to make the file PADDED_SIZE.
- This gives us enough space to do what we want later.
- Next, socfpgaimage_set_header() is called.
- We fix up the buffer by moving the image to the start of the buffer.
- We now have some room to do what we need (add CRC and padding).
- */
+static int data_size; +#define FAKE_HEADER_SIZE (PADDED_SIZE - data_size)
+static int socfpgaimage_vrec_header(struct image_tool_params *params,
struct image_type_params *tparams)+{
- struct stat sbuf;
- if (!params->datafile || stat(params->datafile, &sbuf) < 0)
return 0;- data_size = sbuf.st_size;
- tparams->header_size = FAKE_HEADER_SIZE;
- return 0;
That's quite weird that you are returning 0 for both cases.
The history for that is that the usage of vrec_header by mkimage.c changed.
In the branch where this code was developed, I returned FAKE_HEADER_SIZE, but mkimage.c was ignoring the returned value.
On master, the returned value is used for the post padding size or something, but is limited to using a 4k. mkimage.c cannot tolerate return values of more than 4k and will abort the mkimage
I agree the code does look weird and I will rework that to something like:
if (...) { data_size = .. tparams->header_size ... } return 0;
Thank you for your feedback, most appreciated.
-- CHarles
Hi Charles,
On 02/26/2014 01:42 AM, Charles Manning wrote:
On Wednesday 26 February 2014 19:16:37 Michal Simek wrote:
On 02/26/2014 02:17 AM, 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 cdhmanning@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.
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 | 276 ++++++++++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 401 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 0000000..813d55d --- /dev/null +++ b/include/crc32_alt.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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 0000000..e0db335 --- /dev/null +++ b/lib/crc32_alt.c @@ -0,0 +1,94 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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] = {
- 0x00000000, 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,
- 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd,
- 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039,
- 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5,
- 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81,
- 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d,
- 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49,
- 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95,
- 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1,
- 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d,
- 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae,
- 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072,
- 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16,
- 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca,
- 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde,
- 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02,
- 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066,
- 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba,
- 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e,
- 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692,
- 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6,
- 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a,
- 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e,
- 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2,
- 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686,
- 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a,
- 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637,
- 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb,
- 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f,
- 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53,
- 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47,
- 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b,
- 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff,
- 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623,
- 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7,
- 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b,
- 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f,
- 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3,
- 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7,
- 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b,
- 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f,
- 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3,
- 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640,
- 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c,
- 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8,
- 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24,
- 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30,
- 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec,
- 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088,
- 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654,
- 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0,
- 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c,
- 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18,
- 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4,
- 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0,
- 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c,
- 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668,
- 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4,
+};
+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;
+} diff --git a/spl/Makefile b/spl/Makefile index bf98024..bce9055 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,11 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin
- ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..46af0b1 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -71,6 +71,7 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \ crc32.o \
crc32_alt.o \ default_image.o \ fit_image.o \ image-fit.o \@@ -85,6 +86,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/crc32_alt.c b/tools/crc32_alt.c new file mode 100644 index 0000000..3faa222 --- /dev/null +++ b/tools/crc32_alt.c @@ -0,0 +1 @@ +#include "../lib/crc32_alt.c" diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type();
/* Init Altera SOCFPGA support */
init_socfpga_image_type(); }
/*
diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..f9df9ae --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,276 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- Reference doc
http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate. Of particular interest to us is the +
- "header" length field being in U32s and not bytes.
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end
checksum). + * 0x48 2 Zero
- 0x0A 2 Checksum over the heder. NB Not CRC32
- At the end of the code we have a 32-bit CRC checksum over whole
binary + * excluding the CRC.
- Note that the CRC used here is **not** the zlib/Adler crc32. It is
the + * CRC-32 used in bzip2, ethernet and elsewhere.
- The image is padded out to 64k, because that is what is
- typically used to write the image to the boot medium.
- */
+#include <crc32_alt.h> +#include "imagetool.h" +#include <image.h>
+#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0x0C
just 0xC here
+#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000
+static uint8_t buffer[PADDED_SIZE];
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
+/*
- 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;
+}
+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;
+}
+static int align4(int v) +{
- return ((v + 3) / 4) * 4;
+}
+static void build_header(uint8_t *buf,
uint8_t version,uint8_t flags,uint16_t length_bytes)+{
- memset(buf, 0, HEADER_SIZE);
- load_le32(buf + 0, VALIDATION_WORD);
- buf[4] = version;
- buf[5] = flags;
- load_le16(buf + 6, length_bytes/4);
- load_le16(buf + 10, hdr_checksum(buf, 10));
+}
+/*
- Perform a rudimentary verification of header and return
- size of image.
- */
+static int verify_header(const uint8_t *buf) +{
- if (get_le32(buf) != VALIDATION_WORD)
return -1;- if (get_le16(buf + 10) != hdr_checksum(buf, 10))
return -1;- return get_le16(buf+6) * 4;
buf + 6
+}
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t crcval;
- /* Align the length up */
- len = align4(len);
- /* Build header, adding 4 bytes to length to hold the CRC32. */
- build_header(buf + HEADER_OFFSET, version, flags, len + 4);
- crcval = crc32_alt(0, buf, len);
- load_le32(buf + len, crcval);
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{
- int len; /* Including 32bit CRC */
- uint32_t crccalc;
- len = verify_header(buf + HEADER_OFFSET);
- if (len < 0)
return -1;- if (len < HEADER_OFFSET || len > PADDED_SIZE)
return -1;- /* Adjust length, removing CRC */
- len -= 4;
- crccalc = crc32_alt(0, buf, len);
- if (get_le32(buf + len) != crccalc)
return -1;- return 0;
+}
+/* mkimage glue functions */ +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size, + struct image_tool_params *params) +{
- if (image_size != PADDED_SIZE)
return -1;- return verify_buffer(ptr);
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
+static int socfpgaimage_check_params(struct image_tool_params *params) +{
- /* Not sure if we should be accepting fflags */
- return (params->dflag && (params->fflag || params->lflag)) ||
(params->fflag && (params->dflag || params->lflag)) ||(params->lflag && (params->dflag || params->fflag));+}
+static int socfpgaimage_check_image_types(uint8_t type) +{
- if (type == IH_TYPE_SOCFPGAIMAGE)
return EXIT_SUCCESS;- else
this else is not needed here.
return EXIT_FAILURE;+}
+/*
- To work in with the mkimage framework, we do some ugly stuff...
- First, socfpgaimage_vrec_header() is called.
on space here.
- We prepend a fake header big enough to make the file PADDED_SIZE.
- This gives us enough space to do what we want later.
- Next, socfpgaimage_set_header() is called.
- We fix up the buffer by moving the image to the start of the buffer.
- We now have some room to do what we need (add CRC and padding).
- */
+static int data_size; +#define FAKE_HEADER_SIZE (PADDED_SIZE - data_size)
+static int socfpgaimage_vrec_header(struct image_tool_params *params,
struct image_type_params *tparams)+{
- struct stat sbuf;
- if (!params->datafile || stat(params->datafile, &sbuf) < 0)
return 0;- data_size = sbuf.st_size;
- tparams->header_size = FAKE_HEADER_SIZE;
- return 0;
That's quite weird that you are returning 0 for both cases.
The history for that is that the usage of vrec_header by mkimage.c changed.
In the branch where this code was developed, I returned FAKE_HEADER_SIZE, but mkimage.c was ignoring the returned value.
On master, the returned value is used for the post padding size or something, but is limited to using a 4k. mkimage.c cannot tolerate return values of more than 4k and will abort the mkimage
I agree the code does look weird and I will rework that to something like:
Thanks for this. Can you please CC myself and Chin Liang on your next series?
clee@altera.com dinguyen@altera.com
Thanks, Dinh
if (...) { data_size = .. tparams->header_size ... } return 0;
Thank you for your feedback, most appreciated.
-- CHarles
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 cdhmanning@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 0000000..813d55d --- /dev/null +++ b/include/crc32_alt.h @@ -0,0 +1,17 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@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 0000000..e0db335 --- /dev/null +++ b/lib/crc32_alt.c @@ -0,0 +1,94 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@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] = { + 0x00000000, 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, + 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd, + 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039, + 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5, + 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81, + 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d, + 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49, + 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95, + 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1, + 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d, + 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae, + 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072, + 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16, + 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca, + 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde, + 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02, + 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066, + 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba, + 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e, + 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692, + 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6, + 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a, + 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e, + 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2, + 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686, + 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a, + 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637, + 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb, + 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f, + 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53, + 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47, + 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b, + 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff, + 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623, + 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7, + 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b, + 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f, + 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3, + 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7, + 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b, + 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f, + 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3, + 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640, + 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c, + 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8, + 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24, + 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30, + 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec, + 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088, + 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654, + 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0, + 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c, + 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18, + 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4, + 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0, + 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c, + 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668, + 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4, +}; + +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; +} diff --git a/spl/Makefile b/spl/Makefile index bf98024..bce9055 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,11 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin + $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@ + +ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin + ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..46af0b1 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -71,6 +71,7 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \ crc32.o \ + crc32_alt.o \ default_image.o \ fit_image.o \ image-fit.o \ @@ -85,6 +86,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \ + socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y) diff --git a/tools/crc32_alt.c b/tools/crc32_alt.c new file mode 100644 index 0000000..3faa222 --- /dev/null +++ b/tools/crc32_alt.c @@ -0,0 +1 @@ +#include "../lib/crc32_alt.c" diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type(); + /* Init Altera SOCFPGA support */ + init_socfpga_image_type(); }
/* diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..842c1b0 --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,278 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@gmail.com + * + * SPDX-License-Identifier: GPL-2.0+ + * + * Reference doc http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate. Of particular interest to us is the + * "header" length field being in U32s and not bytes. + * + * "Header" is a structure of the following format. + * this is positioned at 0x40. + * + * Endian is LSB. + * + * Offset Length Usage + * ----------------------- + * 0x40 4 Validation word 0x31305341 + * 0x44 1 Version (whatever, zero is fine) + * 0x45 1 Flags (unused, zero is fine) + * 0x46 2 Length (in units of u32, including the end checksum). + * 0x48 2 Zero + * 0x4A 2 Checksum over the heder. NB Not CRC32 + * + * At the end of the code we have a 32-bit CRC checksum over whole binary + * excluding the CRC. + * + * Note that the CRC used here is **not** the zlib/Adler crc32. It is the + * CRC-32 used in bzip2, ethernet and elsewhere. + * + * The image is padded out to 64k, because that is what is + * typically used to write the image to the boot medium. + */ + +#include <crc32_alt.h> +#include "imagetool.h" +#include <image.h> + +#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0xC +#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000 + +/* To allow for adding CRC, the max input size is a bit smaller. */ +#define MAX_INPUT_SIZE (PADDED_SIZE - sizeof(uint32_t)) + +static uint8_t buffer[PADDED_SIZE]; + +/* + * The header checksum is just a very simple checksum over + * the header area. + * There is still a crc32 over the whole lot. + */ +static uint16_t hdr_checksum(const uint8_t *buf, int len) +{ + uint16_t ret = 0; + int i; + + for (i = 0; i < len; i++) { + ret += (((uint16_t) *buf) & 0xff); + buf++; + } + return ret; +} + +/* + * 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; +} + +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; +} + +static int align4(int v) +{ + return ((v + 3) / 4) * 4; +} + +static void build_header(uint8_t *buf, + uint8_t version, + uint8_t flags, + uint16_t length_bytes) +{ + memset(buf, 0, HEADER_SIZE); + + load_le32(buf + 0, VALIDATION_WORD); + buf[4] = version; + buf[5] = flags; + load_le16(buf + 6, length_bytes/4); + load_le16(buf + 10, hdr_checksum(buf, 10)); +} + +/* + * Perform a rudimentary verification of header and return + * size of image. + */ +static int verify_header(const uint8_t *buf) +{ + if (get_le32(buf) != VALIDATION_WORD) + return -1; + if (get_le16(buf + 10) != hdr_checksum(buf, 10)) + return -1; + + return get_le16(buf + 6) * 4; +} + +/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf, + uint8_t version, uint8_t flags, + int len, int pad_64k) +{ + uint32_t crcval; + + /* Align the length up */ + len = align4(len); + + /* Build header, adding 4 bytes to length to hold the CRC32. */ + build_header(buf + HEADER_OFFSET, version, flags, len + 4); + + crcval = crc32_alt(0, buf, len); + + load_le32(buf + len, crcval); + + if (!pad_64k) + return len + 4; + + return PADDED_SIZE; +} + +/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{ + int len; /* Including 32bit CRC */ + uint32_t crccalc; + + len = verify_header(buf + HEADER_OFFSET); + if (len < 0) + return -1; + if (len < HEADER_OFFSET || len > PADDED_SIZE) + return -1; + + /* Adjust length, removing CRC */ + len -= 4; + + crccalc = crc32_alt(0, buf, len); + + if (get_le32(buf + len) != crccalc) + return -1; + + return 0; +} + +/* mkimage glue functions */ +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size, + struct image_tool_params *params) +{ + if (image_size != PADDED_SIZE) + return -1; + + return verify_buffer(ptr); +} + +static void socfpgaimage_print_header(const void *ptr) +{ + if (verify_buffer(ptr) == 0) + printf("Looks like a sane SOCFPGA preloader\n"); + else + printf("Not a sane SOCFPGA preloader\n"); +} + +static int socfpgaimage_check_params(struct image_tool_params *params) +{ + /* Not sure if we should be accepting fflags */ + return (params->dflag && (params->fflag || params->lflag)) || + (params->fflag && (params->dflag || params->lflag)) || + (params->lflag && (params->dflag || params->fflag)); +} + +static int socfpgaimage_check_image_types(uint8_t type) +{ + if (type == IH_TYPE_SOCFPGAIMAGE) + return EXIT_SUCCESS; + return EXIT_FAILURE; +} + +/* + * To work in with the mkimage framework, we do some ugly stuff... + * + * First, socfpgaimage_vrec_header() is called. + * We prepend a fake header big enough to make the file PADDED_SIZE. + * This gives us enough space to do what we want later. + * + * Next, socfpgaimage_set_header() is called. + * We fix up the buffer by moving the image to the start of the buffer. + * We now have some room to do what we need (add CRC and padding). + */ + +static int data_size; +#define FAKE_HEADER_SIZE (PADDED_SIZE - data_size) + +static int socfpgaimage_vrec_header(struct image_tool_params *params, + struct image_type_params *tparams) +{ + struct stat sbuf; + + if (params->datafile && + stat(params->datafile, &sbuf) == 0 && + sbuf.st_size <= MAX_INPUT_SIZE) { + data_size = sbuf.st_size; + tparams->header_size = FAKE_HEADER_SIZE; + } + return 0; +} + +static void socfpgaimage_set_header(void *ptr, struct stat *sbuf, int ifd, + struct image_tool_params *params) +{ + uint8_t *buf = (uint8_t *)ptr; + + /* + * This function is called after vrec_header() has been called. + * At this stage we have the FAKE_HEADER_SIZE dummy bytes followed by + * data_size image bytes. Total = PADDED_SIZE. + * We need to fix the buffer by moving the image bytes back to + * the beginning of the buffer, then actually do the signing stuff... + */ + memmove(buf, buf + FAKE_HEADER_SIZE, data_size); + memset(buf + data_size, 0, FAKE_HEADER_SIZE); + + sign_buffer(buf, 0, 0, data_size, 0); +} + +static struct image_type_params socfpgaimage_params = { + .name = "Altera SOCFPGA preloader support", + .vrec_header = socfpgaimage_vrec_header, + .header_size = 0, /* This will be modified by vrec_header() */ + .hdr = (void *)buffer, + .check_image_type = socfpgaimage_check_image_types, + .verify_header = socfpgaimage_verify_header, + .print_header = socfpgaimage_print_header, + .set_header = socfpgaimage_set_header, + .check_params = socfpgaimage_check_params, +}; + +void init_socfpga_image_type(void) +{ + register_image_type(&socfpgaimage_params); +}
Dear Charles,
In message 1393472979-7522-1-git-send-email-cdhmanning@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@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
On Friday 28 February 2014 10:57:21 Wolfgang Denk wrote:
Dear Charles,
In message 1393472979-7522-1-git-send-email-cdhmanning@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@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
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
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 cdhmanning@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 0000000..813d55d --- /dev/null +++ b/include/crc32_alt.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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 0000000..e0db335 --- /dev/null +++ b/lib/crc32_alt.c @@ -0,0 +1,94 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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] = {
- 0x00000000, 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,
- 0x791d4014, 0x7ddc5da3, 0x709f7b7a, 0x745e66cd,
- 0x9823b6e0, 0x9ce2ab57, 0x91a18d8e, 0x95609039,
- 0x8b27c03c, 0x8fe6dd8b, 0x82a5fb52, 0x8664e6e5,
- 0xbe2b5b58, 0xbaea46ef, 0xb7a96036, 0xb3687d81,
- 0xad2f2d84, 0xa9ee3033, 0xa4ad16ea, 0xa06c0b5d,
- 0xd4326d90, 0xd0f37027, 0xddb056fe, 0xd9714b49,
- 0xc7361b4c, 0xc3f706fb, 0xceb42022, 0xca753d95,
- 0xf23a8028, 0xf6fb9d9f, 0xfbb8bb46, 0xff79a6f1,
- 0xe13ef6f4, 0xe5ffeb43, 0xe8bccd9a, 0xec7dd02d,
- 0x34867077, 0x30476dc0, 0x3d044b19, 0x39c556ae,
- 0x278206ab, 0x23431b1c, 0x2e003dc5, 0x2ac12072,
- 0x128e9dcf, 0x164f8078, 0x1b0ca6a1, 0x1fcdbb16,
- 0x018aeb13, 0x054bf6a4, 0x0808d07d, 0x0cc9cdca,
- 0x7897ab07, 0x7c56b6b0, 0x71159069, 0x75d48dde,
- 0x6b93dddb, 0x6f52c06c, 0x6211e6b5, 0x66d0fb02,
- 0x5e9f46bf, 0x5a5e5b08, 0x571d7dd1, 0x53dc6066,
- 0x4d9b3063, 0x495a2dd4, 0x44190b0d, 0x40d816ba,
- 0xaca5c697, 0xa864db20, 0xa527fdf9, 0xa1e6e04e,
- 0xbfa1b04b, 0xbb60adfc, 0xb6238b25, 0xb2e29692,
- 0x8aad2b2f, 0x8e6c3698, 0x832f1041, 0x87ee0df6,
- 0x99a95df3, 0x9d684044, 0x902b669d, 0x94ea7b2a,
- 0xe0b41de7, 0xe4750050, 0xe9362689, 0xedf73b3e,
- 0xf3b06b3b, 0xf771768c, 0xfa325055, 0xfef34de2,
- 0xc6bcf05f, 0xc27dede8, 0xcf3ecb31, 0xcbffd686,
- 0xd5b88683, 0xd1799b34, 0xdc3abded, 0xd8fba05a,
- 0x690ce0ee, 0x6dcdfd59, 0x608edb80, 0x644fc637,
- 0x7a089632, 0x7ec98b85, 0x738aad5c, 0x774bb0eb,
- 0x4f040d56, 0x4bc510e1, 0x46863638, 0x42472b8f,
- 0x5c007b8a, 0x58c1663d, 0x558240e4, 0x51435d53,
- 0x251d3b9e, 0x21dc2629, 0x2c9f00f0, 0x285e1d47,
- 0x36194d42, 0x32d850f5, 0x3f9b762c, 0x3b5a6b9b,
- 0x0315d626, 0x07d4cb91, 0x0a97ed48, 0x0e56f0ff,
- 0x1011a0fa, 0x14d0bd4d, 0x19939b94, 0x1d528623,
- 0xf12f560e, 0xf5ee4bb9, 0xf8ad6d60, 0xfc6c70d7,
- 0xe22b20d2, 0xe6ea3d65, 0xeba91bbc, 0xef68060b,
- 0xd727bbb6, 0xd3e6a601, 0xdea580d8, 0xda649d6f,
- 0xc423cd6a, 0xc0e2d0dd, 0xcda1f604, 0xc960ebb3,
- 0xbd3e8d7e, 0xb9ff90c9, 0xb4bcb610, 0xb07daba7,
- 0xae3afba2, 0xaafbe615, 0xa7b8c0cc, 0xa379dd7b,
- 0x9b3660c6, 0x9ff77d71, 0x92b45ba8, 0x9675461f,
- 0x8832161a, 0x8cf30bad, 0x81b02d74, 0x857130c3,
- 0x5d8a9099, 0x594b8d2e, 0x5408abf7, 0x50c9b640,
- 0x4e8ee645, 0x4a4ffbf2, 0x470cdd2b, 0x43cdc09c,
- 0x7b827d21, 0x7f436096, 0x7200464f, 0x76c15bf8,
- 0x68860bfd, 0x6c47164a, 0x61043093, 0x65c52d24,
- 0x119b4be9, 0x155a565e, 0x18197087, 0x1cd86d30,
- 0x029f3d35, 0x065e2082, 0x0b1d065b, 0x0fdc1bec,
- 0x3793a651, 0x3352bbe6, 0x3e119d3f, 0x3ad08088,
- 0x2497d08d, 0x2056cd3a, 0x2d15ebe3, 0x29d4f654,
- 0xc5a92679, 0xc1683bce, 0xcc2b1d17, 0xc8ea00a0,
- 0xd6ad50a5, 0xd26c4d12, 0xdf2f6bcb, 0xdbee767c,
- 0xe3a1cbc1, 0xe760d676, 0xea23f0af, 0xeee2ed18,
- 0xf0a5bd1d, 0xf464a0aa, 0xf9278673, 0xfde69bc4,
- 0x89b8fd09, 0x8d79e0be, 0x803ac667, 0x84fbdbd0,
- 0x9abc8bd5, 0x9e7d9662, 0x933eb0bb, 0x97ffad0c,
- 0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668,
- 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4,
+};
+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;
+}
This function is the same as BZ_UPDATE_CRC within lib/bzlib_private.h.
diff --git a/spl/Makefile b/spl/Makefile index bf98024..bce9055 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -181,6 +181,11 @@ $(objtree)/SPL : $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..46af0b1 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -71,6 +71,7 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \ crc32.o \
crc32_alt.o \ default_image.o \ fit_image.o \ image-fit.o \@@ -85,6 +86,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/crc32_alt.c b/tools/crc32_alt.c new file mode 100644 index 0000000..3faa222 --- /dev/null +++ b/tools/crc32_alt.c @@ -0,0 +1 @@ +#include "../lib/crc32_alt.c" diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type();
- /* Init Altera SOCFPGA support */
- init_socfpga_image_type();
}
/* diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..842c1b0 --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,278 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- Note this doc is not entirely accurate. Of particular interest to us is the
- "header" length field being in U32s and not bytes.
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end checksum).
- 0x48 2 Zero
- 0x4A 2 Checksum over the heder. NB Not CRC32
- At the end of the code we have a 32-bit CRC checksum over whole binary
- excluding the CRC.
- Note that the CRC used here is **not** the zlib/Adler crc32. It is the
- CRC-32 used in bzip2, ethernet and elsewhere.
- The image is padded out to 64k, because that is what is
- typically used to write the image to the boot medium.
- */
+#include <crc32_alt.h> +#include "imagetool.h" +#include <image.h>
+#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0xC +#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000
+/* To allow for adding CRC, the max input size is a bit smaller. */ +#define MAX_INPUT_SIZE (PADDED_SIZE - sizeof(uint32_t))
+static uint8_t buffer[PADDED_SIZE];
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
+/*
- 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;
+}
+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;
+}
+static int align4(int v) +{
- return ((v + 3) / 4) * 4;
+}
+static void build_header(uint8_t *buf,
uint8_t version,uint8_t flags,uint16_t length_bytes)+{
- memset(buf, 0, HEADER_SIZE);
- load_le32(buf + 0, VALIDATION_WORD);
- buf[4] = version;
- buf[5] = flags;
- load_le16(buf + 6, length_bytes/4);
- load_le16(buf + 10, hdr_checksum(buf, 10));
+}
+/*
- Perform a rudimentary verification of header and return
- size of image.
- */
+static int verify_header(const uint8_t *buf) +{
- if (get_le32(buf) != VALIDATION_WORD)
return -1;- if (get_le16(buf + 10) != hdr_checksum(buf, 10))
return -1;- return get_le16(buf + 6) * 4;
+}
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t crcval;
- /* Align the length up */
- len = align4(len);
- /* Build header, adding 4 bytes to length to hold the CRC32. */
- build_header(buf + HEADER_OFFSET, version, flags, len + 4);
- crcval = crc32_alt(0, buf, len);
- load_le32(buf + len, crcval);
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{
- int len; /* Including 32bit CRC */
- uint32_t crccalc;
- len = verify_header(buf + HEADER_OFFSET);
- if (len < 0)
return -1;- if (len < HEADER_OFFSET || len > PADDED_SIZE)
return -1;- /* Adjust length, removing CRC */
- len -= 4;
- crccalc = crc32_alt(0, buf, len);
- if (get_le32(buf + len) != crccalc)
return -1;- return 0;
+}
+/* mkimage glue functions */ +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size,
struct image_tool_params *params)+{
- if (image_size != PADDED_SIZE)
return -1;- return verify_buffer(ptr);
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
+static int socfpgaimage_check_params(struct image_tool_params *params) +{
- /* Not sure if we should be accepting fflags */
- return (params->dflag && (params->fflag || params->lflag)) ||
(params->fflag && (params->dflag || params->lflag)) ||(params->lflag && (params->dflag || params->fflag));+}
+static int socfpgaimage_check_image_types(uint8_t type) +{
- if (type == IH_TYPE_SOCFPGAIMAGE)
return EXIT_SUCCESS;- return EXIT_FAILURE;
+}
+/*
- To work in with the mkimage framework, we do some ugly stuff...
- First, socfpgaimage_vrec_header() is called.
- We prepend a fake header big enough to make the file PADDED_SIZE.
- This gives us enough space to do what we want later.
- Next, socfpgaimage_set_header() is called.
- We fix up the buffer by moving the image to the start of the buffer.
- We now have some room to do what we need (add CRC and padding).
- */
+static int data_size; +#define FAKE_HEADER_SIZE (PADDED_SIZE - data_size)
+static int socfpgaimage_vrec_header(struct image_tool_params *params,
struct image_type_params *tparams)+{
- struct stat sbuf;
- if (params->datafile &&
stat(params->datafile, &sbuf) == 0 &&sbuf.st_size <= MAX_INPUT_SIZE) {data_size = sbuf.st_size;tparams->header_size = FAKE_HEADER_SIZE;- }
- return 0;
+}
+static void socfpgaimage_set_header(void *ptr, struct stat *sbuf, int ifd,
struct image_tool_params *params)+{
- uint8_t *buf = (uint8_t *)ptr;
- /*
* This function is called after vrec_header() has been called.* At this stage we have the FAKE_HEADER_SIZE dummy bytes followed by* data_size image bytes. Total = PADDED_SIZE.* We need to fix the buffer by moving the image bytes back to* the beginning of the buffer, then actually do the signing stuff...*/- memmove(buf, buf + FAKE_HEADER_SIZE, data_size);
- memset(buf + data_size, 0, FAKE_HEADER_SIZE);
- sign_buffer(buf, 0, 0, data_size, 0);
+}
+static struct image_type_params socfpgaimage_params = {
- .name = "Altera SOCFPGA preloader support",
- .vrec_header = socfpgaimage_vrec_header,
- .header_size = 0, /* This will be modified by vrec_header() */
- .hdr = (void *)buffer,
- .check_image_type = socfpgaimage_check_image_types,
- .verify_header = socfpgaimage_verify_header,
- .print_header = socfpgaimage_print_header,
- .set_header = socfpgaimage_set_header,
- .check_params = socfpgaimage_check_params,
+};
+void init_socfpga_image_type(void) +{
- register_image_type(&socfpgaimage_params);
+}
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 cdhmanning@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.
Changes for v7: - Use bzlib's crc table instead of adding another one. - Use existing code and a packed structure for header marshalling.
Note: Building a SOCFPGA preloader will currently not produce 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/bzlib_crc32.h | 17 ++++ include/image.h | 1 + lib/bzlib_crc32.c | 26 +++++ spl/Makefile | 5 + tools/Makefile | 3 + tools/bzlib_crc32.c | 1 + tools/bzlib_crctable.c | 1 + tools/bzlib_private.h | 1 + tools/imagetool.c | 2 + tools/imagetool.h | 1 + tools/socfpgaimage.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++ 12 files changed, 314 insertions(+) create mode 100644 include/bzlib_crc32.h create mode 100644 lib/bzlib_crc32.c create mode 100644 tools/bzlib_crc32.c create mode 100644 tools/bzlib_crctable.c create mode 100644 tools/bzlib_private.h 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/bzlib_crc32.h b/include/bzlib_crc32.h new file mode 100644 index 0000000..96d8124 --- /dev/null +++ b/include/bzlib_crc32.h @@ -0,0 +1,17 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@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 __BZLIB_CRC32_H__ +#define __BZLIB_CRC32_H__ + +#include <stdint.h> + +uint32_t bzlib_crc32(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/bzlib_crc32.c b/lib/bzlib_crc32.c new file mode 100644 index 0000000..cc1a8a0 --- /dev/null +++ b/lib/bzlib_crc32.c @@ -0,0 +1,26 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@gmail.com + * + * SPDX-License-Identifier: GPL-2.0+ + * + * This provides a CRC-32 using the tables in bzlib_crctable.c + */ + +#include <bzlib_crc32.h> +#include "bzlib_private.h" + +uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length) +{ + const uint8_t *buf = _buf; + + crc ^= ~0; + + while (length--) { + crc = (crc << 8) ^ BZ2_crc32Table[((crc >> 24) ^ *buf) & 0xff]; + buf++; + } + + crc ^= ~0; + + return crc; +} diff --git a/spl/Makefile b/spl/Makefile index 346d0aa..4e0f33f 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -182,6 +182,11 @@ MLO MLO.byteswap: $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin + $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@ + +ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin + ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif diff --git a/tools/Makefile b/tools/Makefile index dcd49f8..a912093 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \ + bzlib_crc32.o \ + bzlib_crctable.o \ crc32.o \ default_image.o \ fit_image.o \ @@ -85,6 +87,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \ + socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y) diff --git a/tools/bzlib_crc32.c b/tools/bzlib_crc32.c new file mode 100644 index 0000000..b7f7aa5 --- /dev/null +++ b/tools/bzlib_crc32.c @@ -0,0 +1 @@ +#include "../lib/bzlib_crc32.c" diff --git a/tools/bzlib_crctable.c b/tools/bzlib_crctable.c new file mode 100644 index 0000000..53d38ef --- /dev/null +++ b/tools/bzlib_crctable.c @@ -0,0 +1 @@ +#include "../bzlib_crctable.c" diff --git a/tools/bzlib_private.h b/tools/bzlib_private.h new file mode 100644 index 0000000..cfb74be --- /dev/null +++ b/tools/bzlib_private.h @@ -0,0 +1 @@ +#include "lib/bzlib_private.h" diff --git a/tools/imagetool.c b/tools/imagetool.c index 29d2189..1ef20b1 100644 --- a/tools/imagetool.c +++ b/tools/imagetool.c @@ -45,6 +45,8 @@ void register_image_tool(imagetool_register_t image_register) init_ubl_image_type(); /* Init Davinci AIS support */ init_ais_image_type(); + /* Init Altera SOCFPGA support */ + init_socfpga_image_type(); }
/* diff --git a/tools/imagetool.h b/tools/imagetool.h index c2c9aea..c4833b1 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -167,6 +167,7 @@ void init_mxs_image_type(void); void init_fit_image_type(void); void init_ubl_image_type(void); void init_omap_image_type(void); +void init_socfpga_image_type(void);
void pbl_load_uboot(int fd, struct image_tool_params *mparams);
diff --git a/tools/socfpgaimage.c b/tools/socfpgaimage.c new file mode 100644 index 0000000..c7ec4d1 --- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,255 @@ +/* + * Copyright (C) 2014 Charles Manning cdhmanning@gmail.com + * + * SPDX-License-Identifier: GPL-2.0+ + * + * Reference doc http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate. Of particular interest to us is the + * "header" length field being in U32s and not bytes. + * + * "Header" is a structure of the following format. + * this is positioned at 0x40. + * + * Endian is LSB. + * + * Offset Length Usage + * ----------------------- + * 0x40 4 Validation word 0x31305341 + * 0x44 1 Version (whatever, zero is fine) + * 0x45 1 Flags (unused, zero is fine) + * 0x46 2 Length (in units of u32, including the end checksum). + * 0x48 2 Zero + * 0x4A 2 Checksum over the heder. NB Not CRC32 + * + * At the end of the code we have a 32-bit CRC checksum over whole binary + * excluding the CRC. + * + * Note that the CRC used here is **not** the zlib/Adler crc32. It is the + * CRC-32 used in bzip2, ethernet and elsewhere. + * + * The image is padded out to 64k, because that is what is + * typically used to write the image to the boot medium. + */ + +#include <bzlib_crc32.h> +#include "imagetool.h" +#include <image.h> + +#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0xC +#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000 + +/* To allow for adding CRC, the max input size is a bit smaller. */ +#define MAX_INPUT_SIZE (PADDED_SIZE - sizeof(uint32_t)) + +static uint8_t buffer[PADDED_SIZE]; + +static struct { + uint32_t validation; + uint8_t version; + uint8_t flags; + uint16_t length_u32; + uint16_t zero; + uint16_t checksum; +} __attribute__((packed)) header; + +/* + * The header checksum is just a very simple checksum over + * the header area. + * There is still a crc32 over the whole lot. + */ +static uint16_t hdr_checksum(const uint8_t *buf, int len) +{ + uint16_t ret = 0; + int i; + + for (i = 0; i < len; i++) { + ret += (((uint16_t) *buf) & 0xff); + buf++; + } + return ret; +} + + +static void build_header(uint8_t *buf, + uint8_t version, + uint8_t flags, + uint16_t length_bytes) +{ + header.validation = htole32(VALIDATION_WORD); + header.version = version; + header.flags = flags; + header.length_u32 = htole16(length_bytes/4); + header.zero = 0; + header.checksum = htole16(hdr_checksum((const uint8_t *)&header, 10)); + + memcpy(buf, &header, sizeof(header)); +} + +/* + * Perform a rudimentary verification of header and return + * size of image. + */ +static int verify_header(const uint8_t *buf) +{ + memcpy(&header, buf, sizeof(header)); + + if (le32toh(header.validation) != VALIDATION_WORD) + return -1; + if (le16toh(header.checksum) != + hdr_checksum((const uint8_t *)&header, 10)) + return -1; + + return le16toh(header.length_u32) * 4; +} + +/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf, + uint8_t version, uint8_t flags, + int len, int pad_64k) +{ + uint32_t calc_crc; + + /* Align the length up */ + len = (len + 3) & (~3); + + /* Build header, adding 4 bytes to length to hold the CRC32. */ + build_header(buf + HEADER_OFFSET, version, flags, len + 4); + + /* Calculate and apply the CRC */ + calc_crc = bzlib_crc32(0, buf, len); + + *((uint32_t *)(buf + len)) = htole32(calc_crc); + + if (!pad_64k) + return len + 4; + + return PADDED_SIZE; +} + +/* Verify that the buffer looks sane */ +static int verify_buffer(const uint8_t *buf) +{ + int len; /* Including 32bit CRC */ + uint32_t calc_crc; + uint32_t buf_crc; + + len = verify_header(buf + HEADER_OFFSET); + if (len < 0) + return -1; + if (len < HEADER_OFFSET || len > PADDED_SIZE) + return -1; + + /* + * Adjust length to the base of the CRC. + * Check the CRC. + */ + len -= 4; + + calc_crc = bzlib_crc32(0, buf, len); + + buf_crc = le32toh(*((uint32_t *)(buf + len))); + + if (buf_crc != calc_crc) + return -1; + + return 0; +} + +/* mkimage glue functions */ +static int socfpgaimage_verify_header(unsigned char *ptr, int image_size, + struct image_tool_params *params) +{ + if (image_size != PADDED_SIZE) + return -1; + + return verify_buffer(ptr); +} + +static void socfpgaimage_print_header(const void *ptr) +{ + if (verify_buffer(ptr) == 0) + printf("Looks like a sane SOCFPGA preloader\n"); + else + printf("Not a sane SOCFPGA preloader\n"); +} + +static int socfpgaimage_check_params(struct image_tool_params *params) +{ + /* Not sure if we should be accepting fflags */ + return (params->dflag && (params->fflag || params->lflag)) || + (params->fflag && (params->dflag || params->lflag)) || + (params->lflag && (params->dflag || params->fflag)); +} + +static int socfpgaimage_check_image_types(uint8_t type) +{ + if (type == IH_TYPE_SOCFPGAIMAGE) + return EXIT_SUCCESS; + return EXIT_FAILURE; +} + +/* + * To work in with the mkimage framework, we do some ugly stuff... + * + * First, socfpgaimage_vrec_header() is called. + * We prepend a fake header big enough to make the file PADDED_SIZE. + * This gives us enough space to do what we want later. + * + * Next, socfpgaimage_set_header() is called. + * We fix up the buffer by moving the image to the start of the buffer. + * We now have some room to do what we need (add CRC and padding). + */ + +static int data_size; +#define FAKE_HEADER_SIZE (PADDED_SIZE - data_size) + +static int socfpgaimage_vrec_header(struct image_tool_params *params, + struct image_type_params *tparams) +{ + struct stat sbuf; + + if (params->datafile && + stat(params->datafile, &sbuf) == 0 && + sbuf.st_size <= MAX_INPUT_SIZE) { + data_size = sbuf.st_size; + tparams->header_size = FAKE_HEADER_SIZE; + } + return 0; +} + +static void socfpgaimage_set_header(void *ptr, struct stat *sbuf, int ifd, + struct image_tool_params *params) +{ + uint8_t *buf = (uint8_t *)ptr; + + /* + * This function is called after vrec_header() has been called. + * At this stage we have the FAKE_HEADER_SIZE dummy bytes followed by + * data_size image bytes. Total = PADDED_SIZE. + * We need to fix the buffer by moving the image bytes back to + * the beginning of the buffer, then actually do the signing stuff... + */ + memmove(buf, buf + FAKE_HEADER_SIZE, data_size); + memset(buf + data_size, 0, FAKE_HEADER_SIZE); + + sign_buffer(buf, 0, 0, data_size, 0); +} + +static struct image_type_params socfpgaimage_params = { + .name = "Altera SOCFPGA preloader support", + .vrec_header = socfpgaimage_vrec_header, + .header_size = 0, /* This will be modified by vrec_header() */ + .hdr = (void *)buffer, + .check_image_type = socfpgaimage_check_image_types, + .verify_header = socfpgaimage_verify_header, + .print_header = socfpgaimage_print_header, + .set_header = socfpgaimage_set_header, + .check_params = socfpgaimage_check_params, +}; + +void init_socfpga_image_type(void) +{ + register_image_type(&socfpgaimage_params); +}
On Thu, Mar 06, 2014 at 15:40 +1300, Charles Manning wrote:
[ ... ] 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.
I don't quite get why you say that the zlib implementation is not a CRC. IIUC all of the CRC-32 methods follow a common algorithm, and just happen to differ in their polynomials or initial and final masks. So they result in different CRC values for the same data stream, yet all of them are CRCs.
But strictly speaking this remark is not specific to this patch submission, and might be omitted. The only relevant thing is that the zlib CRC-32 approach does not result in the value that the SoC's ROM loader expects.
Thus, the appropriate CRC generator is added to lib/ as crc32_alt.c.
What does "alt" stand for, and can you provide an appropriate description with the commit log (or just use a better name)? As previous communication suggests, it's certainly not "alternative" -- as there just are too many alternatives available to mark one of them as _the_ alternative. It's neither "apple" where you appear to have found the matching polynomial first. If it's "Altera", I'd suggest to use either "altr" as this is what elsewhere is used for Altera (the stock ticker), or plain "altera" instead of something this short and ambiguous. Or "bzlib" since this is the most recent implementation that you are using.
Signed-off-by: Charles Manning cdhmanning@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.
Changes for v7:
- Use bzlib's crc table instead of adding another one.
- Use existing code and a packed structure for header marshalling.
One of the reasons I got tired of providing feedback is that this change log is rather terse. Several identical "fix coding style" phrases are another way of telling readers that they should figure out for themselves what might have changed (especially when newer versions have functional changes that are not announced as such), and whether feedback was considered or got ignored. Seeing several iterations which suffer from the same issues that have been identified multiple versions before isn't helpful either when asking yourself whether to spend more time and getting ignored another time.
--- /dev/null +++ b/include/bzlib_crc32.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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 __BZLIB_CRC32_H__ +#define __BZLIB_CRC32_H__
+#include <stdint.h>
+uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length);
+#endif
and
--- /dev/null +++ b/lib/bzlib_crc32.c @@ -0,0 +1,26 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- This provides a CRC-32 using the tables in bzlib_crctable.c
- */
+#include <bzlib_crc32.h> +#include "bzlib_private.h"
+uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length) +{
- const uint8_t *buf = _buf;
- crc ^= ~0;
- while (length--) {
crc = (crc << 8) ^ BZ2_crc32Table[((crc >> 24) ^ *buf) & 0xff];buf++;- }
- crc ^= ~0;
- return crc;
+}
Since you already include the private bzlib header for the BZ2_crc32Table[] declaration, you might as well use the BZ_INITIALISE_CRC(), BZ_FINALISE_CRC(), and BZ_UPDATE_CRC() macros declared from there (right next to the table's declaration), instead of re-inventing how the CRC gets calculated.
You probably should determine whether you want to provide one routine to do the complete calculate over a given byte stream, without any "previous" CRC value -- this is what your current use case is. Or whether you want to provide three primitives to initialize, update, and finalize a CRC (which is not used yet). Or whether you want to provide the three primitives plus one convenience wrapper.
--- a/tools/Makefile +++ b/tools/Makefile @@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \
bzlib_crc32.o \bzlib_crctable.o \ crc32.o \ default_image.o \ fit_image.o \@@ -85,6 +87,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/bzlib_crc32.c b/tools/bzlib_crc32.c new file mode 100644 index 0000000..b7f7aa5 --- /dev/null +++ b/tools/bzlib_crc32.c @@ -0,0 +1 @@ +#include "../lib/bzlib_crc32.c" diff --git a/tools/bzlib_crctable.c b/tools/bzlib_crctable.c new file mode 100644 index 0000000..53d38ef --- /dev/null +++ b/tools/bzlib_crctable.c @@ -0,0 +1 @@ +#include "../bzlib_crctable.c" diff --git a/tools/bzlib_private.h b/tools/bzlib_private.h new file mode 100644 index 0000000..cfb74be --- /dev/null +++ b/tools/bzlib_private.h @@ -0,0 +1 @@ +#include "lib/bzlib_private.h"
Now this is incredibly ugly. You are duplicating lib/ stuff in tools/ and build it there as well? Or not at all build it in lib/ where the code actually resides? It's only a side note that #include for "*.c" is problematic as some compilers do semi-intelligent stuff when the filename does not end in ".h" and then break compilation, so this hack should neither get spread nor suggested for use.
Is there any (real, technical) reason why the bzip stuff (the CRC-32 calculation that has been made available separately) cannot get built and used as a library, and the tools/ application just gets linked against it as one would expect?
--- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,255 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- Note this doc is not entirely accurate. Of particular interest to us is the
- "header" length field being in U32s and not bytes.
Can you rephrase this? The current form suggests that you cannot trust the Altera documentation, while you fail to state what exactly you have identified as being wrong (whether this one length spec is all the problems, or whether there are more).
Looking at "Loading the Preloader" on page A-7 I see that it says the "Program length" is in bytes, while the "CRC" description on page A-8 mentions "Program Length * 4". So the Handbook is inconsistent, and available data suggests that "length in units of 32bit words" is correct.
The polynomial cited there BTW translates to 0x04c11db7, which may be more specific than "what is used in ethernet and elsewhere", and might be useful to have in comments and commit logs. The initial value and final XOR and "no reflection" are the other attributes of this specific CRC-32 method.
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end checksum).
- 0x48 2 Zero
- 0x4A 2 Checksum over the heder. NB Not CRC32
This is wrong (and is not what the Handbook Appendix says). The length is 32bits in width. It just happens that in little endian representation and with a maximum of 60KB of payload data, the upper 16bits naturally remain zero. Still the documentation and implementation should work with a 32bit length.
I agree that the term "checksum" might be confusing, because it's a mere sum of the preceeding bytes, and "by coincidence" the sum is used to check plausibility of the header. I wouldn't either know how to best put this into a short comment. From personal experience I can tell that "sum" alone would have helped a lot.
- At the end of the code we have a 32-bit CRC checksum over whole binary
- excluding the CRC.
This probably should say "CRC over the all payload data from offset zero up to but not including the position of the CRC". Depending on whether the 64KB padding is considered part of the binary or some "afterwards thing", this might help.
Background information: The SPL is supposed to be up to 60KB in size (the upper 4KB are for the ROM loader's internal use, and for data shared among the preloader and the bootloader). The SPL memory image has a gap at 0x40 (after the vectors, and before the "useful code") where the preloader header/signature gets put into. After the signature application, a CRC-32 is calculated over the complete data and gets appended to the data (the length spec in the header includes the CRC location). Then the image gets padded to full 64KB and gets duplicated four times (to increase robustness by means of redundancy, assuming boot media might have read errors).
Your submission's motivation is to initially stamp a preloader that's the fresh result of a compilation. I can see that it would be useful and desirable to apply the tool to existing binaries as well, i.e. throw a 64KB binary at the tool to re-create the signature.
- Note that the CRC used here is **not** the zlib/Adler crc32. It is the
- CRC-32 used in bzip2, ethernet and elsewhere.
If you want to be most specific, cite the parameters of the CRC-32 used here. See the above comments and http://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/libcn/crc... and maybe http://www.ross.net/crc/download/crc_v3.txt
BTW I'm still under the impression that zlib uses the same polynomial as well as identical initial values and final XOR masks (see lib/crc32.c). So where is the difference between the zlib and the bzlib CRC-32 parameters?
$ perl -e 'use Compress::Zlib; printf "%x\n", crc32("123456789");' cbf43926
- The image is padded out to 64k, because that is what is
- typically used to write the image to the boot medium.
- */
+#include <bzlib_crc32.h> +#include "imagetool.h" +#include <image.h>
+#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0xC +#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000
+/* To allow for adding CRC, the max input size is a bit smaller. */ +#define MAX_INPUT_SIZE (PADDED_SIZE - sizeof(uint32_t))
See above, it's not a blocker for the initial motivation, but might be useful to run the tool on existing 64KB blobs as well. But then it's hard to determine the "right" length in reliable ways, and the benefit may not be as big as I think it is, hmm... So OK, forget this idea, running the tool on fresh compiler output of at most 60KB size is quite appropriate an assumption.
+static uint8_t buffer[PADDED_SIZE];
+static struct {
- uint32_t validation;
- uint8_t version;
- uint8_t flags;
- uint16_t length_u32;
- uint16_t zero;
- uint16_t checksum;
+} __attribute__((packed)) header;
You are aware of the "packed" and "alignment" discussion threads here, aren't you? I considered the "serializer" accessors of previous patch iterations most appropriate (except for their names), and feel this approach with a struct is inferior (especially because it doesn't cover the CRC later). But others need not agree.
Having a "length_u32" field which is of 'uint16_t' data type doesn't look right.
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
What is the typecast good for? Why are there still whitespace issues? Can you either fix them or tell why this is OK? I.e. can you _somehow_ respond to feedback you receive? Ignoring review comments makes you recive less feedback later.
The usual C language style comments apply: Please don't mix declarations and instructions. You can eliminate variables. Use names which better reflect what is happening. Something like this is more idiomatic:
static uint16_t header_sum(const uint8_t *buf, int len) { uint16_t sum;
sum = 0; while (len-- > 0) { sum += *buf++; } return sum; }
+static void build_header(uint8_t *buf,
uint8_t version,uint8_t flags,uint16_t length_bytes)+{
- header.validation = htole32(VALIDATION_WORD);
- header.version = version;
- header.flags = flags;
- header.length_u32 = htole16(length_bytes/4);
- header.zero = 0;
- header.checksum = htole16(hdr_checksum((const uint8_t *)&header, 10));
- memcpy(buf, &header, sizeof(header));
+}
I'd suggest to s/4/sizeof(uint32_t)/
Can you rephrase the "10" in terms of "header size minus the width of its sum"? Very similar to the CRC which covers "all of the data except the CRC field".
And there may be a problem with "direct" assignments to a "packed" struct. Since you fill in a local struct and memcpy to the image then, you might as well use "normal serializers".
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t calc_crc;
- /* Align the length up */
- len = (len + 3) & (~3);
Is there nothing you can re-use? If not can you introduce something generic to roundup() for user space tools to use? Can you rephrase the condition such that it becomes evident you are doing ROUNDUP(len, sizeof(uint32_t)) and avoid the magic "3" that has no such meaning for readers?
- /* Build header, adding 4 bytes to length to hold the CRC32. */
- build_header(buf + HEADER_OFFSET, version, flags, len + 4);
- /* Calculate and apply the CRC */
- calc_crc = bzlib_crc32(0, buf, len);
- *((uint32_t *)(buf + len)) = htole32(calc_crc);
This is what I was talking about above. It may be better to use a serializer to write both header fields as well as the CRC.
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
Would it be useful to print a little more information than "it's a preloader" without any further details? I'm not familiar with the context this routine gets called in, what do users expect?
virtually yours Gerhard Sittig
Hello Gerhard
Thank you for that feedback.
On Sunday 09 March 2014 05:51:23 Gerhard Sittig wrote:
On Thu, Mar 06, 2014 at 15:40 +1300, Charles Manning wrote:
[ ... ] 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.
I don't quite get why you say that the zlib implementation is not a CRC. IIUC all of the CRC-32 methods follow a common algorithm, and just happen to differ in their polynomials or initial and final masks. So they result in different CRC values for the same data stream, yet all of them are CRCs.
But strictly speaking this remark is not specific to this patch submission, and might be omitted. The only relevant thing is that the zlib CRC-32 approach does not result in the value that the SoC's ROM loader expects.
Ok I will change that comment
Thus, the appropriate CRC generator is added to lib/ as crc32_alt.c.
What does "alt" stand for, and can you provide an appropriate description with the commit log (or just use a better name)? As previous communication suggests, it's certainly not "alternative" -- as there just are too many alternatives available to mark one of them as _the_ alternative. It's neither "apple" where you appear to have found the matching polynomial first. If it's "Altera", I'd suggest to use either "altr" as this is what elsewhere is used for Altera (the stock ticker), or plain "altera" instead of something this short and ambiguous. Or "bzlib" since this is the most recent implementation that you are using.
I agree thank you for pointing out that error in my comment.
Signed-off-by: Charles Manning cdhmanning@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.
Changes for v7:
- Use bzlib's crc table instead of adding another one.
- Use existing code and a packed structure for header marshalling.
One of the reasons I got tired of providing feedback is that this change log is rather terse. Several identical "fix coding style" phrases are another way of telling readers that they should figure out for themselves what might have changed (especially when newer versions have functional changes that are not announced as such), and whether feedback was considered or got ignored. Seeing several iterations which suffer from the same issues that have been identified multiple versions before isn't helpful either when asking yourself whether to spend more time and getting ignored another time.
Ok, I will try to be less terse.
--- /dev/null +++ b/include/bzlib_crc32.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@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 __BZLIB_CRC32_H__ +#define __BZLIB_CRC32_H__
+#include <stdint.h>
+uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length);
+#endif
and
--- /dev/null +++ b/lib/bzlib_crc32.c @@ -0,0 +1,26 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- This provides a CRC-32 using the tables in bzlib_crctable.c
- */
+#include <bzlib_crc32.h> +#include "bzlib_private.h"
+uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length) +{
- const uint8_t *buf = _buf;
- crc ^= ~0;
- while (length--) {
crc = (crc << 8) ^ BZ2_crc32Table[((crc >> 24) ^ *buf) & 0xff];buf++;- }
- crc ^= ~0;
- return crc;
+}
Since you already include the private bzlib header for the BZ2_crc32Table[] declaration, you might as well use the BZ_INITIALISE_CRC(), BZ_FINALISE_CRC(), and BZ_UPDATE_CRC() macros declared from there (right next to the table's declaration), instead of re-inventing how the CRC gets calculated.
If you think that makes it more clear I shall do that. I don't think though that it really is any clearer.
You probably should determine whether you want to provide one routine to do the complete calculate over a given byte stream, without any "previous" CRC value -- this is what your current use case is. Or whether you want to provide three primitives to initialize, update, and finalize a CRC (which is not used yet). Or whether you want to provide the three primitives plus one convenience wrapper.
I try to use a single "familiar" function, like crc32() does.
You can start with 0, then use it with the results.
eg
crc = crc32(0, first_buff,...); crc = crc32(crc, second_bufer,...);
--- a/tools/Makefile +++ b/tools/Makefile @@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \
bzlib_crc32.o \bzlib_crctable.o \ crc32.o \ default_image.o \ fit_image.o \@@ -85,6 +87,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/bzlib_crc32.c b/tools/bzlib_crc32.c new file mode 100644 index 0000000..b7f7aa5 --- /dev/null +++ b/tools/bzlib_crc32.c @@ -0,0 +1 @@ +#include "../lib/bzlib_crc32.c" diff --git a/tools/bzlib_crctable.c b/tools/bzlib_crctable.c new file mode 100644 index 0000000..53d38ef --- /dev/null +++ b/tools/bzlib_crctable.c @@ -0,0 +1 @@ +#include "../bzlib_crctable.c" diff --git a/tools/bzlib_private.h b/tools/bzlib_private.h new file mode 100644 index 0000000..cfb74be --- /dev/null +++ b/tools/bzlib_private.h @@ -0,0 +1 @@ +#include "lib/bzlib_private.h"
Now this is incredibly ugly. You are duplicating lib/ stuff in tools/ and build it there as well? Or not at all build it in lib/ where the code actually resides?
It's only a side note that #include for "*.c" is problematic as some compilers do semi-intelligent stuff when the filename does not end in ".h" and then break compilation, so this hack should neither get spread nor suggested for use.
I absolutely agree this is very, very, ugly.
I was just following the current way of doing things. This is how crc32.c, sha1.c, md5.c, fdt.c and many others are handled.
I expect that this is worth a refactorisation effort, but I think that should be a separate exercise from doing this signer.
Is there any (real, technical) reason why the bzip stuff (the CRC-32 calculation that has been made available separately) cannot get built and used as a library, and the tools/ application just gets linked against it as one would expect?
From my limited understanding, lib/ is not built for the host. It is only built for the target. That is why we have all these ugly C files: crc32.c, sha1.c,... in tools/.
--- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,255 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- Reference doc
http://www.altera.com.cn/literature/hb/cyclone-v/cv_5400A.pdf + * Note this doc is not entirely accurate. Of particular interest to us is the +
- "header" length field being in U32s and not bytes.
Can you rephrase this? The current form suggests that you cannot trust the Altera documentation, while you fail to state what exactly you have identified as being wrong (whether this one length spec is all the problems, or whether there are more).
Looking at "Loading the Preloader" on page A-7 I see that it says the "Program length" is in bytes, while the "CRC" description on page A-8 mentions "Program Length * 4". So the Handbook is inconsistent, and available data suggests that "length in units of 32bit words" is correct.
The definitive data point is that one works and the other does not :-).
The polynomial cited there BTW translates to 0x04c11db7, which may be more specific than "what is used in ethernet and elsewhere", and might be useful to have in comments and commit logs. The initial value and final XOR and "no reflection" are the other attributes of this specific CRC-32 method.
Ok, I will use that.
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end
checksum). + * 0x48 2 Zero
- 0x4A 2 Checksum over the heder. NB Not CRC32
This is wrong (and is not what the Handbook Appendix says). The length is 32bits in width. It just happens that in little endian representation and with a maximum of 60KB of payload data, the upper 16bits naturally remain zero. Still the documentation and implementation should work with a 32bit length.
The implementation surely only has to work with the ROM. There is no point in it working for arbitrary cases.
I agree that the term "checksum" might be confusing, because it's a mere sum of the preceeding bytes, and "by coincidence" the sum is used to check plausibility of the header. I wouldn't either know how to best put this into a short comment. From personal experience I can tell that "sum" alone would have helped a lot.
What is a checksum but a sum that is used for checking? The term checksum on its own does not imply any particular algorithm. If you think sum is better, I shall use that.
- At the end of the code we have a 32-bit CRC checksum over whole
binary + * excluding the CRC.
This probably should say "CRC over the all payload data from offset zero up to but not including the position of the CRC". Depending on whether the 64KB padding is considered part of the binary or some "afterwards thing", this might help.
Ok.
Background information: The SPL is supposed to be up to 60KB in size (the upper 4KB are for the ROM loader's internal use, and for data shared among the preloader and the bootloader). The SPL memory image has a gap at 0x40 (after the vectors, and before the "useful code") where the preloader header/signature gets put into. After the signature application, a CRC-32 is calculated over the complete data and gets appended to the data (the length spec in the header includes the CRC location). Then the image gets padded to full 64KB and gets duplicated four times (to increase robustness by means of redundancy, assuming boot media might have read errors).
Is 60K a hard limitation for SPL? I know that people are considering SPLs of larger size. Yeah... I know... what happened to 4k bootloaders?
Your submission's motivation is to initially stamp a preloader that's the fresh result of a compilation. I can see that it would be useful and desirable to apply the tool to existing binaries as well, i.e. throw a 64KB binary at the tool to re-create the signature.
I am confused by what you are saying here. There is only one point to this: sign a preloader so that a SocFPGA boot ROM will use it. There are no other use cases that make any sense.
Or put another way... would I run the omap signer on an arbitrary binary? No. Same argument here.
By "signing existing binaries" do you mean existing preloaders? If so, people can to that with mkimage -T socfpgaimage ... (once this addition is in place).
- Note that the CRC used here is **not** the zlib/Adler crc32. It is
the + * CRC-32 used in bzip2, ethernet and elsewhere.
If you want to be most specific, cite the parameters of the CRC-32 used here. See the above comments and http://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/libcn/cr c32-bzip2.c and maybe http://www.ross.net/crc/download/crc_v3.txt
BTW I'm still under the impression that zlib uses the same polynomial as well as identical initial values and final XOR masks (see lib/crc32.c). So where is the difference between the zlib and the bzlib CRC-32 parameters?
$ perl -e 'use Compress::Zlib; printf "%x\n", crc32("123456789");' cbf43926
From my understanding zlib crc32() == Adler.
Please compare the tables in lib/crc32.c and lib/bzlib_crc32.c. They are not the same.
I have run many tests.
My original socfpga signer used crc32() and zlib. It did not give the values the ROM desired. As much as I would have liked to use zlib, my preferences are of no consequence. We must use the values the ROM will work with. If we do not, then the ROM will refuse to boot the code, which is not really the point of signing it...
I know Wikipedia isn't always accurate, but please also look at:
http://en.wikipedia.org/wiki/Cyclic_redundancy_check#Commonly_used_and_stand...
I have not investigated this fully, but I think the crc I use here is the same as that used in pblimage.c and mxsimage.c
What I propose is that once this socfpga signer is bedded down, it makes sense to refactor those. I am prepared to do this. However, it is probably a better exercise to make refactoring a separate step from committing new code.
- The image is padded out to 64k, because that is what is
- typically used to write the image to the boot medium.
- */
+#include <bzlib_crc32.h> +#include "imagetool.h" +#include <image.h>
+#define HEADER_OFFSET 0x40 +#define HEADER_SIZE 0xC +#define VALIDATION_WORD 0x31305341 +#define PADDED_SIZE 0x10000
+/* To allow for adding CRC, the max input size is a bit smaller. */ +#define MAX_INPUT_SIZE (PADDED_SIZE - sizeof(uint32_t))
See above, it's not a blocker for the initial motivation, but might be useful to run the tool on existing 64KB blobs as well. But then it's hard to determine the "right" length in reliable ways, and the benefit may not be as big as I think it is, hmm... So OK, forget this idea, running the tool on fresh compiler output of at most 60KB size is quite appropriate an assumption.
I think we can safely say that for all useful use cases, this is a one-trick-pony: signing socfpga images.
+static uint8_t buffer[PADDED_SIZE];
+static struct {
- uint32_t validation;
- uint8_t version;
- uint8_t flags;
- uint16_t length_u32;
- uint16_t zero;
- uint16_t checksum;
+} __attribute__((packed)) header;
You are aware of the "packed" and "alignment" discussion threads here, aren't you?
Sorry I am not aware of these discussions. I personally **hate** using packed structure and assuming binary alignment.
I considered the "serializer" accessors of previous patch iterations most appropriate (except for their names), and feel this approach with a struct is inferior (especially because it doesn't cover the CRC later). But others need not agree.
I too prefer the older serialisers but was told I could not use my own. Yes, there are some in target space: put_aligned_le16() etc, but from my understanding it is unhealthy to use target space functions from host space (mkimage etc).
Having a "length_u32" field which is of 'uint16_t' data type doesn't look right.
It means length in units of u32.
I will change that to length_in_u32 or something like that.
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
What is the typecast good for? Why are there still whitespace issues? Can you either fix them or tell why this is OK? I.e. can you _somehow_ respond to feedback you receive? Ignoring review comments makes you recive less feedback later.
Ok, I will do away with the cast. It does not serve any function.
I am unsure what whitespace issue do you mean? Does checkpatch not catch things like this?
The usual C language style comments apply: Please don't mix declarations and instructions.
What do you mean by "declaration and instructions"? Do you mean the initial value mixing?
You can eliminate variables. Use names which better reflect what is happening. Something like this is more idiomatic:
static uint16_t header_sum(const uint8_t *buf, int len) { uint16_t sum;
sum = 0; while (len-- > 0) { sum += *buf++; } return sum; }
Ok I will do that.
+static void build_header(uint8_t *buf,
uint8_t version,uint8_t flags,uint16_t length_bytes)+{
- header.validation = htole32(VALIDATION_WORD);
- header.version = version;
- header.flags = flags;
- header.length_u32 = htole16(length_bytes/4);
- header.zero = 0;
- header.checksum = htole16(hdr_checksum((const uint8_t *)&header, 10));
- memcpy(buf, &header, sizeof(header));
+}
I'd suggest to s/4/sizeof(uint32_t)/
Ok, I will do that.
Can you rephrase the "10" in terms of "header size minus the width of its sum"? Very similar to the CRC which covers "all of the data except the CRC field".
And there may be a problem with "direct" assignments to a "packed" struct. Since you fill in a local struct and memcpy to the image then, you might as well use "normal serializers".
As explained above, I would prefer to use serialisers, they are much clearer for this sort of application.
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t calc_crc;
- /* Align the length up */
- len = (len + 3) & (~3);
Is there nothing you can re-use? If not can you introduce something generic to roundup() for user space tools to use?
Sure I define something for reuse...
Can you suggest where I might put it?
Can you rephrase the condition such that it becomes evident you are doing ROUNDUP(len, sizeof(uint32_t)) and avoid the magic "3" that has no such meaning for readers?
- /* Build header, adding 4 bytes to length to hold the CRC32. */
- build_header(buf + HEADER_OFFSET, version, flags, len + 4);
- /* Calculate and apply the CRC */
- calc_crc = bzlib_crc32(0, buf, len);
- *((uint32_t *)(buf + len)) = htole32(calc_crc);
This is what I was talking about above. It may be better to use a serializer to write both header fields as well as the CRC.
I absolutely agree. I prefer a serializer, but I moved to packed structures (which I generally mistrust for various reasons including arcane aliassing rules) because I was told not to add my own.
Is it Ok if I add my own? If I can, where do I put them, if not, I cannot see where to find some.
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
Would it be useful to print a little more information than "it's a preloader" without any further details? I'm not familiar with the context this routine gets called in, what do users expect?
It gets called at the end of making an image.
Ok, I will print out some numbers too. They are probably of little use to anyone, but they look "technical" :-).
Thanks
Charles
[ Cc: to Masahiro for the tools/ "vs" lib/ build support part ]
On Mon, Mar 10, 2014 at 16:04 +1300, Charles Manning wrote:
On Sunday 09 March 2014 05:51:23 Gerhard Sittig wrote:
On Thu, Mar 06, 2014 at 15:40 +1300, Charles Manning wrote:
--- /dev/null +++ b/lib/bzlib_crc32.c @@ -0,0 +1,26 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- This provides a CRC-32 using the tables in bzlib_crctable.c
- */
+#include <bzlib_crc32.h> +#include "bzlib_private.h"
+uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length) +{
- const uint8_t *buf = _buf;
- crc ^= ~0;
- while (length--) {
crc = (crc << 8) ^ BZ2_crc32Table[((crc >> 24) ^ *buf) & 0xff];buf++;- }
- crc ^= ~0;
- return crc;
+}
Since you already include the private bzlib header for the BZ2_crc32Table[] declaration, you might as well use the BZ_INITIALISE_CRC(), BZ_FINALISE_CRC(), and BZ_UPDATE_CRC() macros declared from there (right next to the table's declaration), instead of re-inventing how the CRC gets calculated.
If you think that makes it more clear I shall do that. I don't think though that it really is any clearer.
Admittedly your wrapper is thin. Yet it duplicates what the bzlib header already provides. And does so in different phrases (i.e. textually does not match the original, so probably gets missed upon searches).
It's more a matter of consistency to re-use the code as well as the table of the bzip2 implementation, instead of re-inventing both or using parts of it only.
You probably should determine whether you want to provide one routine to do the complete calculate over a given byte stream, without any "previous" CRC value -- this is what your current use case is. Or whether you want to provide three primitives to initialize, update, and finalize a CRC (which is not used yet). Or whether you want to provide the three primitives plus one convenience wrapper.
I try to use a single "familiar" function, like crc32() does.
You can start with 0, then use it with the results.
eg
crc = crc32(0, first_buff,...); crc = crc32(crc, second_bufer,...);
This only works "by coincidence" because the final XOR and the initial value happen to be complimentary and only by chance the value of zero "works". So it's not a general pattern, but a specific feature of this very implementation.
Given that your current application only determines CRC values for complete buffers and nothing is done piecemeal, I'd suggest to provide the one convenience wrapper routine which does not take a previous partial result.
Should other use cases later require incremental calculation, they can introduce and use the three primitives to open, iterate and close the calculation. This better reflects what's happening. Or am I being overly picky?
--- a/tools/Makefile +++ b/tools/Makefile @@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \
bzlib_crc32.o \bzlib_crctable.o \ crc32.o \ default_image.o \ fit_image.o \@@ -85,6 +87,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/bzlib_crc32.c b/tools/bzlib_crc32.c new file mode 100644 index 0000000..b7f7aa5 --- /dev/null +++ b/tools/bzlib_crc32.c @@ -0,0 +1 @@ +#include "../lib/bzlib_crc32.c" diff --git a/tools/bzlib_crctable.c b/tools/bzlib_crctable.c new file mode 100644 index 0000000..53d38ef --- /dev/null +++ b/tools/bzlib_crctable.c @@ -0,0 +1 @@ +#include "../bzlib_crctable.c" diff --git a/tools/bzlib_private.h b/tools/bzlib_private.h new file mode 100644 index 0000000..cfb74be --- /dev/null +++ b/tools/bzlib_private.h @@ -0,0 +1 @@ +#include "lib/bzlib_private.h"
Now this is incredibly ugly. You are duplicating lib/ stuff in tools/ and build it there as well? Or not at all build it in lib/ where the code actually resides?
It's only a side note that #include for "*.c" is problematic as some compilers do semi-intelligent stuff when the filename does not end in ".h" and then break compilation, so this hack should neither get spread nor suggested for use.
I absolutely agree this is very, very, ugly.
I was just following the current way of doing things. This is how crc32.c, sha1.c, md5.c, fdt.c and many others are handled.
I expect that this is worth a refactorisation effort, but I think that should be a separate exercise from doing this signer.
Is there any (real, technical) reason why the bzip stuff (the CRC-32 calculation that has been made available separately) cannot get built and used as a library, and the tools/ application just gets linked against it as one would expect?
From my limited understanding, lib/ is not built for the host. It is only built for the target. That is why we have all these ugly C files: crc32.c, sha1.c,... in tools/.
With Kbuild, is the '#include "../otherdir/source.c" trick still needed? Would a list of object files with relative path specs work, or can object files in one output directory result from compile units taken out of several source directories?
Can the tools/ build for the host environment use a library that gets built from lib/ sources?
That an existing implementation uses similar hacks need not mean that it must be OK, or cannot get improved. :)
--- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,255 @@ [ ... ]
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end
checksum). + * 0x48 2 Zero
- 0x4A 2 Checksum over the heder. NB Not CRC32
This is wrong (and is not what the Handbook Appendix says). The length is 32bits in width. It just happens that in little endian representation and with a maximum of 60KB of payload data, the upper 16bits naturally remain zero. Still the documentation and implementation should work with a 32bit length.
The implementation surely only has to work with the ROM. There is no point in it working for arbitrary cases.
Still it's a deviation from the documentation and obfuscates what's happening. Think of the next person to read and manipulate the code, and find a "32bits length spec" in a 16bit field. That's unexpected, and not necessary. (or is it?)
I still feel that the "length" spec is both, a length in units of 32bit words, as well as a 32bit value. This is what Altera documents in Appendix A.
I agree that the term "checksum" might be confusing, because it's a mere sum of the preceeding bytes, and "by coincidence" the sum is used to check plausibility of the header. I wouldn't either know how to best put this into a short comment. From personal experience I can tell that "sum" alone would have helped a lot.
What is a checksum but a sum that is used for checking? The term checksum on its own does not imply any particular algorithm. If you think sum is better, I shall use that.
Well, the term "checksum" usually is associated with CRCs, in contrast to mere sums. At least that's what triggered in my head. But I'm happy to learn how others feel about it. I may be wrong, and am happy to improve where possible.
Background information: The SPL is supposed to be up to 60KB in size (the upper 4KB are for the ROM loader's internal use, and for data shared among the preloader and the bootloader). The SPL memory image has a gap at 0x40 (after the vectors, and before the "useful code") where the preloader header/signature gets put into. After the signature application, a CRC-32 is calculated over the complete data and gets appended to the data (the length spec in the header includes the CRC location). Then the image gets padded to full 64KB and gets duplicated four times (to increase robustness by means of redundancy, assuming boot media might have read errors).
Is 60K a hard limitation for SPL? I know that people are considering SPLs of larger size. Yeah... I know... what happened to 4k bootloaders?
The 60K value is neither a limitation in the U-Boot project nor specific to the SPL approach. It's the limit of the SoC's ROM loader (OCRAM minus reserved data area).
The above "the SPL is supposed to be up to 60KB in size" is a specific statement for the Altera SoCFPGA preloader case. I thought you should have recognized that number, having worked on the preloader for some time. :)
Your submission's motivation is to initially stamp a preloader that's the fresh result of a compilation. I can see that it would be useful and desirable to apply the tool to existing binaries as well, i.e. throw a 64KB binary at the tool to re-create the signature.
I am confused by what you are saying here. There is only one point to this: sign a preloader so that a SocFPGA boot ROM will use it. There are no other use cases that make any sense.
Or put another way... would I run the omap signer on an arbitrary binary? No. Same argument here.
By "signing existing binaries" do you mean existing preloaders? If so, people can to that with mkimage -T socfpgaimage ... (once this addition is in place).
Upon re-consideration the idea turned out to be not of much benefit. I said that, can't tell whether I should have gone and removed all remarks about it instead. Just forget I said something. :)
- Note that the CRC used here is **not** the zlib/Adler crc32. It is
the + * CRC-32 used in bzip2, ethernet and elsewhere.
If you want to be most specific, cite the parameters of the CRC-32 used here. See the above comments and http://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/libcn/cr c32-bzip2.c and maybe http://www.ross.net/crc/download/crc_v3.txt
BTW I'm still under the impression that zlib uses the same polynomial as well as identical initial values and final XOR masks (see lib/crc32.c). So where is the difference between the zlib and the bzlib CRC-32 parameters?
$ perl -e 'use Compress::Zlib; printf "%x\n", crc32("123456789");' cbf43926
From my understanding zlib crc32() == Adler.
Please compare the tables in lib/crc32.c and lib/bzlib_crc32.c. They are not the same.
I have run many tests.
I do believe that they are not the same. Yet available data suggests that both use identical polynomial, although resulting in different values for the same input. So something else must be different. That's what I said. No conflict here. I wasn't saying that zlib must be used, I was wondering why there would be a difference.
I considered the "serializer" accessors of previous patch iterations most appropriate (except for their names), and feel this approach with a struct is inferior (especially because it doesn't cover the CRC later). But others need not agree.
I too prefer the older serialisers but was told I could not use my own. Yes, there are some in target space: put_aligned_le16() etc, but from my understanding it is unhealthy to use target space functions from host space (mkimage etc).
There may be responses of different severity, there are several grades from questions to recommendations to strong suggestions to total NAKs.
I understood that you were asked to look into whether there is something you can re-use, while experience suggests that there should be something along those lines.
Still I guess that the suggestion was not a hard and fast rule. Investigating and telling "does not apply" is as valid a response to these requests as changing the submission -- if there is necessity or reason. We need not shoehorn code into a specific approach which does not apply or does not fit the specific purpose. This kind of feedback is mostly about trying to blend in and remaining aware of when something is a workaround (at least I got that much). Code should not only work, but it's as important that it's readable.
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
What is the typecast good for? Why are there still whitespace issues? Can you either fix them or tell why this is OK? I.e. can you _somehow_ respond to feedback you receive? Ignoring review comments makes you recive less feedback later.
Ok, I will do away with the cast. It does not serve any function.
I am unsure what whitespace issue do you mean? Does checkpatch not catch things like this?
You see the multiple empty lines after the function, don't you? Even if not strictly required, you might want to use "--strict" with checkpatch as well. It may sound picky, but this kind of stuff is not only cosmetics, but in addition suggests how much attention is put to details, and how much care is taken in iterations of a submission.
The usual C language style comments apply: Please don't mix declarations and instructions.
What do you mean by "declaration and instructions"? Do you mean the initial value mixing?
You can eliminate variables. Use names which better reflect what is happening. Something like this is more idiomatic:
static uint16_t header_sum(const uint8_t *buf, int len) { uint16_t sum;
sum = 0; while (len-- > 0) { sum += *buf++; } return sum; }
Ok I will do that.
Maybe this helps: "uint16_t sum;" is a declaration. "sum = 0;" is an instruction. "uint16_t sum = 0;" is a declaration that is combined with an instruction. It's useful to a certain amount, but can turn into obfuscation when used excessively. It saves almost nothing (just typing the variable name once), but can be costly in terms of bugs and maintenance. In the specific case above, pre-setting the sum and adding the items unnecessarily is several lines apart with other declarations between them. It's easy to miss that there is more code involved than one might perceive at first glance. The effect gets stronger with code that is not as trivial.
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t calc_crc;
- /* Align the length up */
- len = (len + 3) & (~3);
Is there nothing you can re-use? If not can you introduce something generic to roundup() for user space tools to use?
Sure I define something for reuse...
Can you suggest where I might put it?
Can you rephrase the condition such that it becomes evident you are doing ROUNDUP(len, sizeof(uint32_t)) and avoid the magic "3" that has no such meaning for readers?
U-Boot apparently has a roundup() macro, both in the bootloader as well as in user space (mxsimage.c uses it). Did you look it up? `git grep -i roundup`
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
Would it be useful to print a little more information than "it's a preloader" without any further details? I'm not familiar with the context this routine gets called in, what do users expect?
It gets called at the end of making an image.
Ok, I will print out some numbers too. They are probably of little use to anyone, but they look "technical" :-).
At least the size might be nice to have. Checksums less so. It's a judgement call. Haven't checked what other formats do (except having seen U-Boot's native format several hundred times). Just had to ask.
That the print routine gets called at the end of image creation does not mean that it's the only use, BTW. Somebody may want to run -l interactively or from tools/scripts.
virtually yours Gerhard Sittig
Dear Gerhard
Thank you for your further comments and clarifications, may I press you for a few more?
On Tuesday 11 March 2014 08:36:24 Gerhard Sittig wrote:
[ Cc: to Masahiro for the tools/ "vs" lib/ build support part ]
On Mon, Mar 10, 2014 at 16:04 +1300, Charles Manning wrote:
On Sunday 09 March 2014 05:51:23 Gerhard Sittig wrote:
On Thu, Mar 06, 2014 at 15:40 +1300, Charles Manning wrote:
--- /dev/null +++ b/lib/bzlib_crc32.c @@ -0,0 +1,26 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- This provides a CRC-32 using the tables in bzlib_crctable.c
- */
+#include <bzlib_crc32.h> +#include "bzlib_private.h"
+uint32_t bzlib_crc32(uint32_t crc, const void *_buf, int length) +{
- const uint8_t *buf = _buf;
- crc ^= ~0;
- while (length--) {
crc = (crc << 8) ^ BZ2_crc32Table[((crc >> 24) ^ *buf) & 0xff];buf++;- }
- crc ^= ~0;
- return crc;
+}
Since you already include the private bzlib header for the BZ2_crc32Table[] declaration, you might as well use the BZ_INITIALISE_CRC(), BZ_FINALISE_CRC(), and BZ_UPDATE_CRC() macros declared from there (right next to the table's declaration), instead of re-inventing how the CRC gets calculated.
If you think that makes it more clear I shall do that. I don't think though that it really is any clearer.
Admittedly your wrapper is thin. Yet it duplicates what the bzlib header already provides. And does so in different phrases (i.e. textually does not match the original, so probably gets missed upon searches).
It's more a matter of consistency to re-use the code as well as the table of the bzip2 implementation, instead of re-inventing both or using parts of it only.
It does not bug me to use either way. I shall use that.
You probably should determine whether you want to provide one routine to do the complete calculate over a given byte stream, without any "previous" CRC value -- this is what your current use case is. Or whether you want to provide three primitives to initialize, update, and finalize a CRC (which is not used yet). Or whether you want to provide the three primitives plus one convenience wrapper.
I try to use a single "familiar" function, like crc32() does.
You can start with 0, then use it with the results.
eg
crc = crc32(0, first_buff,...); crc = crc32(crc, second_bufer,...);
This only works "by coincidence" because the final XOR and the initial value happen to be complimentary and only by chance the value of zero "works". So it's not a general pattern, but a specific feature of this very implementation.
Given that your current application only determines CRC values for complete buffers and nothing is done piecemeal, I'd suggest to provide the one convenience wrapper routine which does not take a previous partial result.
Should other use cases later require incremental calculation, they can introduce and use the three primitives to open, iterate and close the calculation. This better reflects what's happening. Or am I being overly picky?
Perhaps picky, but I do not mind losing this.
The issue is that these functions are often used on long sequences one buffer at a time (eg. doing a crc on 100k, but getting the data in 1k blocks).
This implementation does not need the multi-buffer support, but when I refactor the crcs in mkimage (as I have undertaken to do - even though it is of no direct utility to me at all), there might be a need to use multi-buffer.
For now I will just provide a single buffer version, and will widen it up if need be later.
--- a/tools/Makefile +++ b/tools/Makefile @@ -70,6 +70,8 @@ RSA_OBJS-$(CONFIG_FIT_SIGNATURE) := rsa-sign.o # common objs for dumpimage and mkimage dumpimage-mkimage-objs := aisimage.o \ $(FIT_SIG_OBJS-y) \
bzlib_crc32.o \bzlib_crctable.o \ crc32.o \ default_image.o \ fit_image.o \@@ -85,6 +87,7 @@ dumpimage-mkimage-objs := aisimage.o \ os_support.o \ pblimage.o \ sha1.o \
socfpgaimage.o \ ublimage.o \ $(LIBFDT_OBJS) \ $(RSA_OBJS-y)diff --git a/tools/bzlib_crc32.c b/tools/bzlib_crc32.c new file mode 100644 index 0000000..b7f7aa5 --- /dev/null +++ b/tools/bzlib_crc32.c @@ -0,0 +1 @@ +#include "../lib/bzlib_crc32.c" diff --git a/tools/bzlib_crctable.c b/tools/bzlib_crctable.c new file mode 100644 index 0000000..53d38ef --- /dev/null +++ b/tools/bzlib_crctable.c @@ -0,0 +1 @@ +#include "../bzlib_crctable.c" diff --git a/tools/bzlib_private.h b/tools/bzlib_private.h new file mode 100644 index 0000000..cfb74be --- /dev/null +++ b/tools/bzlib_private.h @@ -0,0 +1 @@ +#include "lib/bzlib_private.h"
Now this is incredibly ugly. You are duplicating lib/ stuff in tools/ and build it there as well? Or not at all build it in lib/ where the code actually resides?
It's only a side note that #include for "*.c" is problematic as some compilers do semi-intelligent stuff when the filename does not end in ".h" and then break compilation, so this hack should neither get spread nor suggested for use.
I absolutely agree this is very, very, ugly.
I was just following the current way of doing things. This is how crc32.c, sha1.c, md5.c, fdt.c and many others are handled.
I expect that this is worth a refactorisation effort, but I think that should be a separate exercise from doing this signer.
Is there any (real, technical) reason why the bzip stuff (the CRC-32 calculation that has been made available separately) cannot get built and used as a library, and the tools/ application just gets linked against it as one would expect?
From my limited understanding, lib/ is not built for the host. It is only built for the target. That is why we have all these ugly C files: crc32.c, sha1.c,... in tools/.
With Kbuild, is the '#include "../otherdir/source.c" trick still needed? Would a list of object files with relative path specs work, or can object files in one output directory result from compile units taken out of several source directories?
I lack this level of knowledge of Kbuild so sorry I do not know.
Can the tools/ build for the host environment use a library that gets built from lib/ sources?
That an existing implementation uses similar hacks need not mean that it must be OK, or cannot get improved. :)
Absolutely agree. The current way is very, very ugly.
However it is surely better to do the refactoring as a separate step from adding a new function.
There is not much point in doing some lib/*c includes "the right way" and leaving the other stuff "the ugly way". Doing things two ways must surely be even worse than consistency.
--- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,255 @@ [ ... ]
- "Header" is a structure of the following format.
- this is positioned at 0x40.
- Endian is LSB.
- Offset Length Usage
- 0x40 4 Validation word 0x31305341
- 0x44 1 Version (whatever, zero is fine)
- 0x45 1 Flags (unused, zero is fine)
- 0x46 2 Length (in units of u32, including the end
checksum). + * 0x48 2 Zero
- 0x4A 2 Checksum over the heder. NB Not CRC32
This is wrong (and is not what the Handbook Appendix says). The length is 32bits in width. It just happens that in little endian representation and with a maximum of 60KB of payload data, the upper 16bits naturally remain zero. Still the documentation and implementation should work with a 32bit length.
The implementation surely only has to work with the ROM. There is no point in it working for arbitrary cases.
Still it's a deviation from the documentation and obfuscates what's happening. Think of the next person to read and manipulate the code, and find a "32bits length spec" in a 16bit field. That's unexpected, and not necessary. (or is it?)
I still feel that the "length" spec is both, a length in units of 32bit words, as well as a 32bit value. This is what Altera documents in Appendix A.
It does not really bother me either way. I assume the boot ROM will actually be reading this as a u32. Might as well just make it one.
Frequently what you get in the app notes is highly different from what the boot ROM actually does. In this case there is one certain error, but some devices, like the MX53 have all sorts of undocumented flags that need magic fields... Bottom line though is that when we're dealing with some hidden code, what matters most is getting the actual bytes right.
I will change this to a u32 wide field and drop the zero field.
I agree that the term "checksum" might be confusing, because it's a mere sum of the preceeding bytes, and "by coincidence" the sum is used to check plausibility of the header. I wouldn't either know how to best put this into a short comment. From personal experience I can tell that "sum" alone would have helped a lot.
What is a checksum but a sum that is used for checking? The term checksum on its own does not imply any particular algorithm. If you think sum is better, I shall use that.
Well, the term "checksum" usually is associated with CRCs, in contrast to mere sums. At least that's what triggered in my head. But I'm happy to learn how others feel about it. I may be wrong, and am happy to improve where possible.
[Academic sidebar: Both CRCs and checksums are used for checking. The terms are sometimes used interchangeably and can often be very confusing.
By my understanding: * A "true CRC" is something that you add to a the end of a bitstring so that doing the crc calculation over the original string + crc gives you zero. That is a very useful property when you are using hardware to do CRC validation (eg. a CAN controller) and you will often see them associated with telephony/networking specs * A "checksum" just gives you two numbers (received and calculated) that you compare.
You can, of course, use a CRC as a checksum (and that is often how software is written). You cannot, however, use any checksum as a CRC because they do not always have that property.
I might be wrong here, but it is my understanding that the crc32() function in zlib lacks the CRC property mentioned above. From my understanding, it can only be used as a checksum.
end of sidebar]
Background information: The SPL is supposed to be up to 60KB in size (the upper 4KB are for the ROM loader's internal use, and for data shared among the preloader and the bootloader). The SPL memory image has a gap at 0x40 (after the vectors, and before the "useful code") where the preloader header/signature gets put into. After the signature application, a CRC-32 is calculated over the complete data and gets appended to the data (the length spec in the header includes the CRC location). Then the image gets padded to full 64KB and gets duplicated four times (to increase robustness by means of redundancy, assuming boot media might have read errors).
Is 60K a hard limitation for SPL? I know that people are considering SPLs of larger size. Yeah... I know... what happened to 4k bootloaders?
The 60K value is neither a limitation in the U-Boot project nor specific to the SPL approach. It's the limit of the SoC's ROM loader (OCRAM minus reserved data area).
The above "the SPL is supposed to be up to 60KB in size" is a specific statement for the Altera SoCFPGA preloader case. I thought you should have recognized that number, having worked on the preloader for some time. :)
Ok, sorry, I had misunderstood what you were saying.
Your submission's motivation is to initially stamp a preloader that's the fresh result of a compilation. I can see that it would be useful and desirable to apply the tool to existing binaries as well, i.e. throw a 64KB binary at the tool to re-create the signature.
I am confused by what you are saying here. There is only one point to this: sign a preloader so that a SocFPGA boot ROM will use it. There are no other use cases that make any sense.
Or put another way... would I run the omap signer on an arbitrary binary? No. Same argument here.
By "signing existing binaries" do you mean existing preloaders? If so, people can to that with mkimage -T socfpgaimage ... (once this addition is in place).
Upon re-consideration the idea turned out to be not of much benefit. I said that, can't tell whether I should have gone and removed all remarks about it instead. Just forget I said something. :)
- Note that the CRC used here is **not** the zlib/Adler crc32. It
is the + * CRC-32 used in bzip2, ethernet and elsewhere.
If you want to be most specific, cite the parameters of the CRC-32 used here. See the above comments and http://opensource.apple.com/source/CommonCrypto/CommonCrypto-60049/libc n/cr c32-bzip2.c and maybe http://www.ross.net/crc/download/crc_v3.txt
BTW I'm still under the impression that zlib uses the same polynomial as well as identical initial values and final XOR masks (see lib/crc32.c). So where is the difference between the zlib and the bzlib CRC-32 parameters?
$ perl -e 'use Compress::Zlib; printf "%x\n", crc32("123456789");' cbf43926
From my understanding zlib crc32() == Adler.
Please compare the tables in lib/crc32.c and lib/bzlib_crc32.c. They are not the same.
I have run many tests.
I do believe that they are not the same. Yet available data suggests that both use identical polynomial, although resulting in different values for the same input. So something else must be different. That's what I said. No conflict here. I wasn't saying that zlib must be used, I was wondering why there would be a difference.
Even with the same polynomial, and almost the identical implementation, you can get different results if the bytes are shifted through lsb or msb first.
I considered the "serializer" accessors of previous patch iterations most appropriate (except for their names), and feel this approach with a struct is inferior (especially because it doesn't cover the CRC later). But others need not agree.
I too prefer the older serialisers but was told I could not use my own. Yes, there are some in target space: put_aligned_le16() etc, but from my understanding it is unhealthy to use target space functions from host space (mkimage etc).
There may be responses of different severity, there are several grades from questions to recommendations to strong suggestions to total NAKs.
I understood that you were asked to look into whether there is something you can re-use, while experience suggests that there should be something along those lines.
Still I guess that the suggestion was not a hard and fast rule. Investigating and telling "does not apply" is as valid a response to these requests as changing the submission -- if there is necessity or reason. We need not shoehorn code into a specific approach which does not apply or does not fit the specific purpose. This kind of feedback is mostly about trying to blend in and remaining aware of when something is a workaround (at least I got that much). Code should not only work, but it's as important that it's readable.
Ok, what I suggest is that I go back to serialisers. For now I will just have my own private ones. In a future refactoring exercise these can be pulled into a file to stare with others.
Does that sound reasonable?
+/*
- The header checksum is just a very simple checksum over
- the header area.
- There is still a crc32 over the whole lot.
- */
+static uint16_t hdr_checksum(const uint8_t *buf, int len) +{
- uint16_t ret = 0;
- int i;
- for (i = 0; i < len; i++) {
ret += (((uint16_t) *buf) & 0xff);buf++;- }
- return ret;
+}
What is the typecast good for? Why are there still whitespace issues? Can you either fix them or tell why this is OK? I.e. can you _somehow_ respond to feedback you receive? Ignoring review comments makes you recive less feedback later.
Ok, I will do away with the cast. It does not serve any function.
I am unsure what whitespace issue do you mean? Does checkpatch not catch things like this?
You see the multiple empty lines after the function, don't you? Even if not strictly required, you might want to use "--strict" with checkpatch as well. It may sound picky, but this kind of stuff is not only cosmetics, but in addition suggests how much attention is put to details, and how much care is taken in iterations of a submission.
Thank you for that clarification as well as teaching me that checkpatch has a --strict option.
The usual C language style comments apply: Please don't mix declarations and instructions.
What do you mean by "declaration and instructions"? Do you mean the initial value mixing?
You can eliminate variables. Use names which better reflect what is happening. Something like this is more idiomatic:
static uint16_t header_sum(const uint8_t *buf, int len) { uint16_t sum;
sum = 0; while (len-- > 0) { sum += *buf++; } return sum; }
Ok I will do that.
Maybe this helps: "uint16_t sum;" is a declaration. "sum = 0;" is an instruction. "uint16_t sum = 0;" is a declaration that is combined with an instruction. It's useful to a certain amount, but can turn into obfuscation when used excessively. It saves almost nothing (just typing the variable name once), but can be costly in terms of bugs and maintenance. In the specific case above, pre-setting the sum and adding the items unnecessarily is several lines apart with other declarations between them. It's easy to miss that there is more code involved than one might perceive at first glance. The effect gets stronger with code that is not as trivial.
Thanks for that clarification. I will simplify.
+/* Sign the buffer and return the signed buffer size */ +static int sign_buffer(uint8_t *buf,
uint8_t version, uint8_t flags,int len, int pad_64k)+{
- uint32_t calc_crc;
- /* Align the length up */
- len = (len + 3) & (~3);
Is there nothing you can re-use? If not can you introduce something generic to roundup() for user space tools to use?
Sure I define something for reuse...
Can you suggest where I might put it?
Can you rephrase the condition such that it becomes evident you are doing ROUNDUP(len, sizeof(uint32_t)) and avoid the magic "3" that has no such meaning for readers?
U-Boot apparently has a roundup() macro, both in the bootloader as well as in user space (mxsimage.c uses it). Did you look it up? `git grep -i roundup`
roundup() is a macro in msximage.h which is a private header file for msximage.c.
It is surely an ugly dependency for socfpgaimage.c to include msximage.h just for a simple one-line macro.
That would make more sense to refactor into some common header. As I said before, refactoring should probably be a separate exercise.
There is a lot that can be refactored in mkimage: * some macros. * serialisation. * crc calcs (there appear to be 3 used in socfpgaimage.c mxsimage.c and pblimage.c that are the same or just differ by an inversion at the end or the beginning). * fix up that lib/*c include mess. * fix the Makefile which is a mix of Kbuild and ifdef.
The danger of refactoring is that it is easy to break things. It was my intention to add this socfpga signer in a way that just "clips on" and does not fiddle with other code. I think, as a general principle, it is a good idea to do refactoring and feature adding as seperate exercises.
- if (!pad_64k)
return len + 4;- return PADDED_SIZE;
+}
+static void socfpgaimage_print_header(const void *ptr) +{
- if (verify_buffer(ptr) == 0)
printf("Looks like a sane SOCFPGA preloader\n");- else
printf("Not a sane SOCFPGA preloader\n");+}
Would it be useful to print a little more information than "it's a preloader" without any further details? I'm not familiar with the context this routine gets called in, what do users expect?
It gets called at the end of making an image.
Ok, I will print out some numbers too. They are probably of little use to anyone, but they look "technical" :-).
At least the size might be nice to have. Checksums less so. It's a judgement call. Haven't checked what other formats do (except having seen U-Boot's native format several hundred times). Just had to ask.
That the print routine gets called at the end of image creation does not mean that it's the only use, BTW. Somebody may want to run -l interactively or from tools/scripts.
Thanks, I will do that.
Regards
Charles
Hello Charles, Gerhard,
Is there any (real, technical) reason why the bzip stuff (the CRC-32 calculation that has been made available separately) cannot get built and used as a library, and the tools/ application just gets linked against it as one would expect?
From my limited understanding, lib/ is not built for the host. It is only built for the target. That is why we have all these ugly C files: crc32.c, sha1.c,... in tools/.
With Kbuild, is the '#include "../otherdir/source.c" trick still needed? Would a list of object files with relative path specs work, or can object files in one output directory result from compile units taken out of several source directories?
In this case, object files with relative path such as "../lib/foo.o" does not work.
If we write ../lib/foo.o in tools/Makefile, $(HOSTCC) will compile foo.o into lib/ directory. And lator it will be overwritten with the one compiled with $(CC).
I thought this is a simple way to generate two objects from one source file, one is for hostprogs in tools/ and the other for the target in lib/.
BTW, I sometimes see #include for "*.c" for example, drivers/usb/host/ehci-hcd.c of Linux Kernel, although I admit it is ugly.
I agree we need to do something with it, but it's beyond the scope of this patch.
diff --git a/spl/Makefile b/spl/Makefile index 346d0aa..4e0f33f 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -182,6 +182,11 @@ MLO MLO.byteswap: $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
Could you re-write this part as follows, please?
diff --git a/spl/Makefile b/spl/Makefile index bb3d349..9f3893e 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -184,6 +184,12 @@ MLO MLO.byteswap: $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+MKIMAGEFLAGS_socfpga-signed-preloader.bin := -T socfpgaimage +socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin + $(call cmd,mkimage) + +ALL-$(CONFIG_SOCFPGA) += socfpga-signed-preloader.bin + ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
Best Regards Masahiro Yamada
On Tuesday 11 March 2014 22:13:26 you wrote:
Hello Charles, Gerhard,
Is there any (real, technical) reason why the bzip stuff (the CRC-32 calculation that has been made available separately) cannot get built and used as a library, and the tools/ application just gets linked against it as one would expect?
From my limited understanding, lib/ is not built for the host. It is only built for the target. That is why we have all these ugly C files: crc32.c, sha1.c,... in tools/.
With Kbuild, is the '#include "../otherdir/source.c" trick still needed? Would a list of object files with relative path specs work, or can object files in one output directory result from compile units taken out of several source directories?
In this case, object files with relative path such as "../lib/foo.o" does not work.
If we write ../lib/foo.o in tools/Makefile, $(HOSTCC) will compile foo.o into lib/ directory. And lator it will be overwritten with the one compiled with $(CC).
I thought this is a simple way to generate two objects from one source file, one is for hostprogs in tools/ and the other for the target in lib/.
In the long run, I guess splitting the objects something like how SPL is handled in parallel to main u-boot is the right thing, but I absolutely agree with you that this should be a separate exercise. Mixing adding new features and refactoring in one patch is best avoided.
It is a similar issue that makes most of the target side code unavailable for the host tools. For example, anything requiring <asm/*> will clearly NOT work since the host side processor environment need not be, and indeed seldom is, the same as target. Perhaps a split compile could fix that, but it is beyond the scope of this exercise.
BTW, I sometimes see #include for "*.c" for example, drivers/usb/host/ehci-hcd.c of Linux Kernel, although I admit it is ugly.
I agree we need to do something with it, but it's beyond the scope of this patch.
diff --git a/spl/Makefile b/spl/Makefile index 346d0aa..4e0f33f 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -182,6 +182,11 @@ MLO MLO.byteswap: $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+$(OBJTREE)/socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
- $(OBJTREE)/tools/mkimage -T socfpgaimage -d $< $@
+ALL-$(CONFIG_SOCFPGA) += $(OBJTREE)/socfpga-signed-preloader.bin
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
Could you re-write this part as follows, please?
diff --git a/spl/Makefile b/spl/Makefile index bb3d349..9f3893e 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -184,6 +184,12 @@ MLO MLO.byteswap: $(obj)/u-boot-spl.bin
ALL-y += $(obj)/$(SPL_BIN).bin
+MKIMAGEFLAGS_socfpga-signed-preloader.bin := -T socfpgaimage +socfpga-signed-preloader.bin: $(obj)/u-boot-spl.bin
$(call cmd,mkimage)+ALL-$(CONFIG_SOCFPGA) += socfpga-signed-preloader.bin
ifdef CONFIG_SAMSUNG ALL-y += $(obj)/$(BOARD)-spl.bin endif
Thank you. I shall try that.
Thank you for that valuable feedback.
Regards.
-- Charles
participants (7)
-
Charles Manning -
Chin Liang See -
Dinh Nguyen -
Gerhard Sittig -
Masahiro Yamada -
Michal Simek -
Wolfgang Denk