Re: [PATCH 1/4] mkimage: also honour -B even without external data

[Sean Anderson <seanga2@gmail.com>]

On 9/19/23 07:37, Rasmus Villemoes wrote:
> In some cases, using the "external data" feature is impossible or
> undesirable, but one may still want (or need) the FIT image to have a
> certain alignment. Also, given the current 'mkimage -h' output,
> 
>    -B => align size in hex for FIT structure and header
> 
> it is quite unexpected for -B to be effectively ignored without -E.

FWIW, this behavior is documented in doc/mkimage.1 (which should also be
updated if this behavior is implemented):

| The alignment, in hexadecimal, that external data will be aligned to.
| This option only has an effect when -E is specified.

And, for additional context, the documentation for -E is

| After processing, move the image data outside the FIT and store a data
| offset in the FIT. Images will be placed one after the other
| immediately after the FIT, with each one aligned to a 4-byte boundary.
| The existing ‘data’ property in each image will be replaced with
| ‘data-offset’ and ‘data-size’ properties. A ‘data-offset’ of 0
| indicates that it starts in the first (4-byte-aligned) byte after the
| FIT.

Based on this documentation and my understanding of the code as-is, -B
controls the alignment of the images themselves, not the size multiple
of the FIT. However, from what I can tell, this patch does not actually
affect the alignment of the images, but rather adjusts the size of the
overall FIT to a certain alignment. I find this rather unexpected.

--Sean

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   tools/fit_image.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index 9fe69ea0d9..2f5b25098a 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -712,6 +712,42 @@ err:
>   	return ret;
>   }
>   
> +/**
> + * fit_align() - Ensure FIT image has certain alignment
> + *
> + * This takes a normal FIT file (with embedded data) and increases its
> + * size so that it is a multiple of params->bl_len.
> + */
> +static int fit_align(struct image_tool_params *params, const char *fname)
> +{
> +	int fit_size, new_size;
> +	int fd;
> +	struct stat sbuf;
> +	void *fdt;
> +	int ret = 0;
> +	int align_size;
> +
> +	align_size = params->bl_len;
> +	fd = mmap_fdt(params->cmdname, fname, 0, &fdt, &sbuf, false, false);
> +	if (fd < 0)
> +		return -EIO;
> +
> +	fit_size = fdt_totalsize(fdt);
> +	new_size = ALIGN(fit_size, align_size);
> +	fdt_set_totalsize(fdt, new_size);
> +	debug("Size extended from from %x to %x\n", fit_size, new_size);
> +	munmap(fdt, sbuf.st_size);
> +
> +	if (ftruncate(fd, new_size)) {
> +		debug("%s: Failed to truncate file: %s\n", __func__,
> +		      strerror(errno));
> +		ret = -EIO;
> +	}
> +
> +	close(fd);
> +	return ret;
> +}
> +
>   /**
>    * fit_handle_file - main FIT file processing function
>    *
> @@ -817,6 +853,10 @@ static int fit_handle_file(struct image_tool_params *params)
>   		ret = fit_extract_data(params, tmpfile);
>   		if (ret)
>   			goto err_system;
> +	} else if (params->bl_len) {
> +		ret = fit_align(params, tmpfile);
> +		if (ret)
> +			goto err_system;
>   	}
>   
>   	if (rename (tmpfile, params->imagefile) == -1) {