[U-Boot] [PATCH] socfpga: Add a signing tool that automatically signs the preloader.

[Gerhard Sittig]

On Thu, Feb 20, 2014 at 12:01 +1300, Charles Manning wrote:
> 
> No need to use the altera tool any more...

I like the idea of removing this external dependency.  But there
are some issues with your submission.

The commit message could use some more helpful text for those who
are not familiar with the details of how the preloader gets
mangled before it becomes acceptable to the ROM loader.

The patch does not apply to current U-Boot source code, can you
re-check please?  Although this might have been because master
has moved between your sending the patch and my trying to apply
it.

Then there are whitespace issues, make sure to check the coding
style before submission.  Other comments are further down below.

> --- a/spl/Makefile
> +++ b/spl/Makefile
> @@ -144,8 +144,12 @@ $(OBJTREE)/MLO:	$(obj)u-boot-spl.bin
>  
>  $(OBJTREE)/MLO.byteswap: $(obj)u-boot-spl.bin
>  	$(OBJTREE)/tools/mkimage -T omapimage -n byteswap \
> +
>  		-a $(CONFIG_SPL_TEXT_BASE) -d $< $@

this looks suspicious: there has been a continued line, which you
now continue with nothing, and have the remainder stand alone?
Does it even work?

> --- /dev/null
> +++ b/tools/socfpga-signer.c
> @@ -0,0 +1,294 @@
> +
> +[ ... ]
> +
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <string.h>

alpha-sort those, makes checking for duplicates easier and
reduces conflicts during maintenance

> +
> +#define HEADER_OFFSET	0x40
> +#define HEADER_SIZE	0x0C
> +#define BUFFER_SIZE	(0x10000)
> +#define MAX_IMAGE_SIZE 0xFF00
> +#define VALIDATION_WORD	0x31305341
> +#define FILL_BYTE	0x00

there's no point in the parentheses around a single value

isn't the maximum preloader size said to be 60KiB?  aka 64KiB
minus 4KiB for internal use by the ROM loader?  that would be
0x1000 less than 0x10000, not just 0x100

> +static uint16_t hdr_checksum(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;
> +}

the cast and masking should be unnecessary, you are dereferencing
a pointer to a byte after all

> +static void le16(uint8_t *buf, uint16_t v)
> +{
> +	buf[0] = (v >> 0) & 0xff;
> +	buf[1] = (v >> 8) & 0xff;
> +}
> +
> +static void 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;
> +}

can you come up with more descriptive names?  it took a while
before I noticed (at call sites) that "le16()" not only did
conversion, but manipulated a buffer as well

does "put_le16()" sound better to you?

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

isn't there some ROUNDUP() macro already?  (it is in U-Boot, but
I'm not sure for user space)

> +static void build_header(uint8_t *buf,
> +			  uint8_t version,
> +			  uint8_t flags,
> +			  uint16_t length_bytes)
> +{
> +	memset(buf, 0, HEADER_SIZE);
> +
> +	le32(buf + 0, VALIDATION_WORD);
> +	buf[4] = version;
> +	buf[5] = flags;
> +	le16(buf + 6, length_bytes/4);
> +	le16(buf + 10, hdr_checksum(buf, 10));
> +}
> +
> +static uint32_t crc_table[256] = {
> +
> +	0x00000000, 0x04c11db7, 0x09823b6e, 0x0d4326d9,
> +	0x130476dc, 0x17c56b6b, 0x1a864db2, 0x1e475005,
> +	[ ... ]
> +	0xafb010b1, 0xab710d06, 0xa6322bdf, 0xa2f33668,
> +	0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4
> +};
> +
> +uint32_t crc32(uint32_t crc, void *_buf, int length)
> +{
> +	uint8_t *buf = _buf;
> +
> +	crc ^= ~0;
> +
> +	while (length--) {
> +		crc = (crc << 8) ^ crc_table[((crc >> 24) ^ *buf) & 0xff];
> +		buf++;
> +	}
> +
> +	crc ^= ~0;
> +
> +	return crc;
> +}

are you re-inventing CRC32 here?  is there nothing that you can
re-use?

> +static int massage_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(0, buf, len);
> +	//crcval = crc_calc ((uint32_t *)buf, len/4);
> +	printf("crc %x\n", crcval);

dead code? remainders from during development? should go

> +
> +	le32(buf + len, crcval);
> +
> +	if(!pad_64k)
> +		return len + 4;
> +
> +	return (1<<16);
> +}

is this not BUFFER_SIZE?  phrasing the same condition differently
obfuscates what happens, and makes you miss stuff during
maintenance


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de