| Message ID | 20200316063053.26189-1-kever.yang@rock-chips.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mkimage: fit_image: Make fit header and data align to 512 | expand |
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;
Punit Agrawal <punit1.agrawal at toshiba.co.jp> writes: > 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> I hit send to soon - please don't forget to update the commit subject in the next posting. Thanks, Punit [...]
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;
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. The definition of ALIGN is already at include/linux/kernel.h, is it better to use that directly? Thanks, - Kever > >> 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;
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;
Kever Yang <kever.yang at rock-chips.com> writes: > On 2020/3/16 ??3:28, Punit Agrawal wrote: >> Kever Yang <kever.yang at rock-chips.com> writes: [...] >> 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. > > > The definition of ALIGN is already at include/linux/kernel.h, is it > better to use that directly? If there are no restrictions to include that header, please use it. It'll be better than making another copy. :) Cheers, Punit [...]
On Mon, Mar 16, 2020 at 07:11:54PM +0900, Punit Agrawal wrote: > Kever Yang <kever.yang at rock-chips.com> writes: > > > On 2020/3/16 ??3:28, Punit Agrawal wrote: > >> Kever Yang <kever.yang at rock-chips.com> writes: > > [...] > > >> 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. > > > > > > The definition of ALIGN is already at include/linux/kernel.h, is it > > better to use that directly? > > If there are no restrictions to include that header, please use > it. It'll be better than making another copy. :) We can't use that header in tools/ user-space code, no. Using imagetool.h by the ARRAY_SIZE macro is a good spot. Thanks!
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..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)); +} + 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); 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); + 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;
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(-)