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

[Kever Yang]

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(-)
>>
> [...]
>
>> 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.


This is a good idea to clean up the code, I didn't notice this because I 
don't have a chance to touch

other files, I will have a try for next version patch.

>
>>   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

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.

>
>>   	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.


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;