[PATCH v2] mkimage: fit_image: Make fit header and data align to 512

[Punit Agrawal]

Kever Yang <kever.yang at rock-chips.com> writes:

> The image is usually stored in block device like emmc, SD card, make the
> offset of image data aligned to block(512 byte) can avoid data copy
> during boot process.
> eg. SPL boot from FIT image with external data:
> - SPL read the first block of FIT image, and then parse the header;
> - SPL read image data separately;
> - The first image offset is the base_offset which is the header size;
> - The second image offset is just after the first image;
> - If the offset of imge does not aligned, SPL will do memcpy;
> The header size is a ramdon number, which is very possible not aligned, so
> add '-B' to specify the align size in hex for better performance.
>
> example usage:
>   ./tools/mkimage -E -f u-boot.its -B 200 u-boot.itb
>
> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
> ---
>
> Changes in v2:
> - use '-B' to take a argument as a align block lenth;
> - address commens from Heinrich, Rasmus, Tom, Punit;
>
>  doc/uImage.FIT/source_file_format.txt |  5 +++++
>  tools/fit_image.c                     | 26 ++++++++++++++++++++------
>  tools/imagetool.h                     |  1 +
>  tools/mkimage.c                       | 14 ++++++++++++--
>  4 files changed, 38 insertions(+), 8 deletions(-)
>

[...]

> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index 6aa4b1c733..6fa2ce328a 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -23,6 +23,11 @@
>  
>  static image_header_t header;
>  
> +static int get_aligned_size(int size, int aligned_size)
> +{
> +	return ((size + aligned_size - 1) & ~(aligned_size - 1));
> +}
> +

Instead of adding another copy of this code (versions of it already
exist in imx8mimage.c, ifwitool.c, aisiamge.c), it would be better to
move the below snippet to imagetool.h.

#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
#define ALIGN(x, a)		__ALIGN_MASK((x), (typeof(x))(a) - 1)

With this, you can drop the version in imx8mimage.c which seems to
introduce an unnecessary multiplication / division.

>  static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
>  			     const char *tmpfile)
>  {
> @@ -430,8 +435,11 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname)
>  		return -EIO;
>  	fit_size = fdt_totalsize(fdt);
>  
> -	/* Allocate space to hold the image data we will extract */
> -	buf = malloc(fit_size);
> +	/*
> +	 * Allocate space to hold the image data we will extract,
> +	 * 4096 byte extral space allocate for image alignment.
> +	 */
> +	buf = malloc(fit_size + 4096);

I might be missing something but shouldn't the extra allocation depend
on -

* block size
* number of data objects in the FIT

>  	if (!buf) {
>  		ret = -ENOMEM;
>  		goto err_munmap;
> @@ -471,17 +479,23 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname)
>  					buf_ptr);
>  		}
>  		fdt_setprop_u32(fdt, node, FIT_DATA_SIZE_PROP, len);
> -
> -		buf_ptr += (len + 3) & ~3;
> +		if (params->bl_len > 0)
> +			buf_ptr += get_aligned_size(len, params->bl_len);
> +		else
> +			buf_ptr += get_aligned_size(len, 4);
>  	}
>  
>  	/* Pack the FDT and place the data after it */
>  	fdt_pack(fdt);
>  
> +	new_size = fdt_totalsize(fdt);
> +	if (params->bl_len > 0)
> +		new_size = get_aligned_size(new_size, params->bl_len);
> +	else
> +		new_size = get_aligned_size(new_size, 4);

This condition gets duplicated here and in the loop above. It would be
better to introduce a variable to track alignment requirement (4 or
bl_len) at the start of the function and use them in both places.

What do you think?

Thanks,
Punit

> +	fdt_set_totalsize(fdt, new_size);
>  	debug("Size reduced from %x to %x\n", fit_size, fdt_totalsize(fdt));
>  	debug("External data size %x\n", buf_ptr);
> -	new_size = fdt_totalsize(fdt);
> -	new_size = (new_size + 3) & ~3;
>  	munmap(fdt, sbuf.st_size);
>  
>  	if (ftruncate(fd, new_size)) {
> diff --git a/tools/imagetool.h b/tools/imagetool.h
> index e1c778b0df..fd5e4b246c 100644
> --- a/tools/imagetool.h
> +++ b/tools/imagetool.h
> @@ -76,6 +76,7 @@ struct image_tool_params {
>  	bool external_data;	/* Store data outside the FIT */
>  	bool quiet;		/* Don't output text in normal operation */
>  	unsigned int external_offset;	/* Add padding to external data */
> +	int bl_len;		/* Block length in byte for external data */
>  	const char *engine_id;	/* Engine to use for signing */
>  };
>  
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index 5f51d2cc89..584aeff907 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -97,8 +97,9 @@ static void usage(const char *msg)
>  		"          -i => input filename for ramdisk file\n");
>  #ifdef CONFIG_FIT_SIGNATURE
>  	fprintf(stderr,
> -		"Signing / verified boot options: [-E] [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r] [-N engine]\n"
> +		"Signing / verified boot options: [-E] [-B] [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r] [-N engine]\n"
>  		"          -E => place data outside of the FIT structure\n"
> +		"          -B => align FIT structure and header to block\n"
>  		"          -k => set directory containing private keys\n"
>  		"          -K => write public keys to this .dtb file\n"
>  		"          -c => add comment in signature node\n"
> @@ -143,7 +144,7 @@ static void process_args(int argc, char **argv)
>  	int opt;
>  
>  	while ((opt = getopt(argc, argv,
> -			     "a:A:b:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qsT:vVx")) != -1) {
> +			     "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qsT:vVx")) != -1) {
>  		switch (opt) {
>  		case 'a':
>  			params.addr = strtoull(optarg, &ptr, 16);
> @@ -167,6 +168,15 @@ static void process_args(int argc, char **argv)
>  					params.cmdname, optarg);
>  				exit(EXIT_FAILURE);
>  			}
> +			break;
> +		case 'B':
> +			params.bl_len = strtoull(optarg, &ptr, 16);
> +			if (*ptr) {
> +				fprintf(stderr, "%s: invalid block length %s\n",
> +					params.cmdname, optarg);
> +				exit(EXIT_FAILURE);
> +			}
> +
>  			break;
>  		case 'c':
>  			params.comment = optarg;