| Message ID | 20200313015041.27070-1-kever.yang@rock-chips.com |
|---|---|
| State | New |
| Headers | show |
| Series | mkimage: fit_image: Make fit header and data align to 512 | expand |
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;
On 13/03/2020 03.07, Heinrich Schuchardt wrote: > 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. Why not let -B take an integer argument, so the same flag can be used the day someone needs stuff to be aligned on a 4096 byte boundary? Rasmus
On Fri, Mar 13, 2020 at 02:09:32PM +0100, Rasmus Villemoes wrote: > On 13/03/2020 03.07, Heinrich Schuchardt wrote: > > 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. > > Why not let -B take an integer argument, so the same flag can be used > the day someone needs stuff to be aligned on a 4096 byte boundary? Agreed, I'm not happy with the 512 byte assumption either. Thanks!
Hi Kever, 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 align for FIT header and image data for better performance. > > SPI device do not need to enable this feature. This seems like a nice improvement. I had a few minor comments to help readability. > > 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/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; To improve readability of code, please consider naming the newly added flag to something more descriptive, e.g. "block_align_flag" or "align_flag", in future versions of the patch. > 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" "align" > " -k => set directory containing private keys\n" > " -K => write public keys to this .dtb file\n" > " -c => add comment in signature node\n" Thanks, Punit [...]
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); 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; + 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; + 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;
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(-)