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

[Heinrich Schuchardt]

Am March 13, 2020 1:50:41 AM UTC schrieb Kever Yang <kever.yang at rock-chips.com>:
>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 align for FIT header and image data for better performance.
>
>SPI device do not need to enable this feature.
>
>Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>---
>
> doc/uImage.FIT/source_file_format.txt |  5 +++++
> tools/fit_image.c                     | 16 +++++++++++-----
> tools/imagetool.h                     |  1 +
> tools/mkimage.c                       |  8 ++++++--
> 4 files changed, 23 insertions(+), 7 deletions(-)
>
>diff --git a/doc/uImage.FIT/source_file_format.txt
>b/doc/uImage.FIT/source_file_format.txt
>index 18d2aedcb7..50679c4b24 100644
>--- a/doc/uImage.FIT/source_file_format.txt
>+++ b/doc/uImage.FIT/source_file_format.txt
>@@ -304,6 +304,11 @@ Normal kernel FIT image has data embedded within
>FIT structure. U-Boot image
>for SPL boot has external data. Existence of 'data-offset' can be used
>to
> identify which format is used.
> 
>+For FIT image with external data, it would be better to align each
>blob of data
>+to block(512 byte) for block device, so that we don't need to do the
>copy when
>+read the image data in SPL. Pass '-B' to mkimage to align the FIT
>structure and
>+data to 512 byte.
>+
> 9) Examples
> -----------
> 
>diff --git a/tools/fit_image.c b/tools/fit_image.c
>index 6aa4b1c733..4aeaf39536 100644
>--- a/tools/fit_image.c
>+++ b/tools/fit_image.c
>@@ -431,7 +431,7 @@ static int fit_extract_data(struct
>image_tool_params *params, const char *fname)
> 	fit_size = fdt_totalsize(fdt);
> 
> 	/* Allocate space to hold the image data we will extract */
>-	buf = malloc(fit_size);
>+	buf = malloc(fit_size + 2048);

Please, provide a comment in the code explaining why extra space for 4 times the block size is needed.

> 	if (!buf) {
> 		ret = -ENOMEM;
> 		goto err_munmap;
>@@ -471,17 +471,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->bflag)
>+			buf_ptr += (len + 511) & ~511;

Please, use a constant defining the block size.

Isn't there an ALIGN macro for rounding up?

>+		else
>+			buf_ptr += (len + 3) & ~3;
> 	}
> 
> 	/* Pack the FDT and place the data after it */
> 	fdt_pack(fdt);
> 
>+	new_size = fdt_totalsize(fdt);
>+	if (params->bflag)
>+		new_size = (new_size + 511) & ~511;

Same here.

Best regards

Heinrich


>+	else
>+		new_size = (new_size + 3) & ~3;
>+	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..e098cb41cc 100644
>--- a/tools/imagetool.h
>+++ b/tools/imagetool.h
>@@ -40,6 +40,7 @@ struct content_info {
>  * type specific functions
>  */
> struct image_tool_params {
>+	int bflag;
> 	int dflag;
> 	int eflag;
> 	int fflag;
>diff --git a/tools/mkimage.c b/tools/mkimage.c
>index 5f51d2cc89..03b357f869 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 => truncate 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);
>@@ -168,6 +169,9 @@ static void process_args(int argc, char **argv)
> 				exit(EXIT_FAILURE);
> 			}
> 			break;
>+		case 'B':
>+			params.bflag = optarg;
>+			break;
> 		case 'c':
> 			params.comment = optarg;
> 			break;