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

[Punit Agrawal]

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

> Hi Punit,
>
> On 2020/3/16 下午3:28, Punit Agrawal wrote:
>> 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(-)
>>>

[...]

>>> @@ -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
>
> We can get block size here, I will use it in next version, but not
> able to get the number of data
>
> objects before go over the dtb later, so add 8 block may OK for most
> case.

It wouldn't be much effort to get the child count - see
fdtdec_get_child_count() (lib/fdtdec.c) for how it's done.

>>
>>>   	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.
>
>
> Introduce a new variable to replace bl_len may not help much for
> logical or code size because
>
> the compare to '0' already very like the use of a tag.

The suggestion was to make the code more readable. I agree it won't have
any significant impact on code size or execution.

Thanks,
Punit

>
>
> Thanks,
>
> - Kever
>
>>
>> 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;