| Message ID | 20160605151802.248f0184beb7f986629dab67@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Teddy, On 5 June 2016 at 15:18, Teddy Reed <teddy.reed@gmail.com> wrote: > > When building a FIT with external data (-E), an SPL may require absolute > positioning for executing the external firmware. To acheive this use the > (-p) switch, which will replace the amended 'data-offset' with 'data-position' > indicating the absolute position of external data. Why does SPL require that? Is it because it is at an external address, not inside the FIT? > > It is considered an error if the requested absolute position overlaps with the > initial data required for the compact FIT. > > Cc: Simon Glass <sjg@chromium.org> > --- > > Changes in v2: > - Add -p argument to mkimage.1 > - Add -E, -p arguments to mkimage usage text > - Add notes for static position within uImage.FIT docs > - Rebased onto master > > doc/mkimage.1 | 6 ++++++ > doc/uImage.FIT/source_file_format.txt | 4 ++++ > tools/fit_image.c | 19 ++++++++++++++++++- > tools/imagetool.h | 1 + > tools/mkimage.c | 13 +++++++++++-- > 5 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/doc/mkimage.1 b/doc/mkimage.1 > index 4b3a255..ffa7d60 100644 > --- a/doc/mkimage.1 > +++ b/doc/mkimage.1 > @@ -152,6 +152,12 @@ verification. Typically the file here is the device tree binary used by > CONFIG_OF_CONTROL in U-Boot. > > .TP > +.BI "\-p [" "external position" "]" > +Place external data at a static external position. See \-E. Instead of writing > +a 'data-offset' property defining the offset from the end of the FIT, \-p will > +use 'data-position' as the absolute position from the base of the FIT. > + > +.TP > .BI "\-r > Specifies that keys used to sign the FIT are required. This means that they > must be verified for the image to boot. Without this option, the verification > diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt > index 3f54180..ee72740 100644 > --- a/doc/uImage.FIT/source_file_format.txt > +++ b/doc/uImage.FIT/source_file_format.txt > @@ -282,6 +282,10 @@ In this case the 'data' property is omitted. Instead you can use: > aligned to a 4-byte boundary. > - data-size : size of the data in bytes > > +The 'data-offset' property can be substituted with 'data-position', which > +defines a static position or address from the base of the FIT as the offset. > +A static position is helpful when booting U-Boot proper before performing > +relocation. This confuses me since it mentions a static address, but then references the base of the FIT, suggesting it is relative. Can you use the word 'absolute' instead of 'static'? > > 9) Examples > ----------- > diff --git a/tools/fit_image.c b/tools/fit_image.c > index 0551572..76a6de4 100644 > --- a/tools/fit_image.c > +++ b/tools/fit_image.c > @@ -416,7 +416,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) > ret = -EPERM; > goto err_munmap; > } > - fdt_setprop_u32(fdt, node, "data-offset", buf_ptr); > + if (params->external_offset > 0) { > + /* An external offset positions the data absolutely. */ > + fdt_setprop_u32(fdt, node, "data-position", > + params->external_offset + buf_ptr); > + } else { > + fdt_setprop_u32(fdt, node, "data-offset", buf_ptr); > + } > fdt_setprop_u32(fdt, node, "data-size", len); > > buf_ptr += (len + 3) & ~3; > @@ -437,6 +443,17 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) > ret = -EIO; > goto err; > } > + > + /* Check if an offset for the external data was set. */ > + if (params->external_offset > 0) { > + if (params->external_offset < new_size) { > + debug("External offset %x overlaps FIT length %x", > + params->external_offset, new_size); > + ret = -EINVAL; > + goto err; > + } > + new_size = params->external_offset; > + } > if (lseek(fd, new_size, SEEK_SET) < 0) { > debug("%s: Failed to seek to end of file: %s\n", __func__, > strerror(errno)); > diff --git a/tools/imagetool.h b/tools/imagetool.h > index a3ed0f4..1f2161c 100644 > --- a/tools/imagetool.h > +++ b/tools/imagetool.h > @@ -74,6 +74,7 @@ struct image_tool_params { > struct content_info *content_tail; > 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 */ > }; > > /* > diff --git a/tools/mkimage.c b/tools/mkimage.c > index aefe22f..ff3024a 100644 > --- a/tools/mkimage.c > +++ b/tools/mkimage.c > @@ -93,11 +93,13 @@ static void usage(const char *msg) > " -f => input filename for FIT source\n"); > #ifdef CONFIG_FIT_SIGNATURE > fprintf(stderr, > - "Signing / verified boot options: [-k keydir] [-K dtb] [ -c <comment>] [-r]\n" > + "Signing / verified boot options: [-E] [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r]\n" > + " -E => place data outside of the FIT structure\n" > " -k => set directory containing private keys\n" > " -K => write public keys to this .dtb file\n" > " -c => add comment in signature node\n" > " -F => re-sign existing FIT image\n" > + " -p => place external data at a static position\n" > " -r => mark keys used as 'required' in dtb\n"); > #else > fprintf(stderr, > @@ -136,7 +138,7 @@ static void process_args(int argc, char **argv) > int opt; > > while ((opt = getopt(argc, argv, > - "a:A:b:cC:d:D:e:Ef:Fk:K:ln:O:rR:qsT:vVx")) != -1) { > + "a:A:b:cC:d:D:e:Ef:Fk:K:ln:p:O:rR:qsT:vVx")) != -1) { > switch (opt) { > case 'a': > params.addr = strtoull(optarg, &ptr, 16); > @@ -216,6 +218,13 @@ static void process_args(int argc, char **argv) > if (params.os < 0) > usage("Invalid operating system"); > break; > + case 'p': > + params.external_offset = strtoull(optarg, &ptr, 16); > + if (*ptr) { > + fprintf(stderr, "%s: invalid offset size %s\n", > + params.cmdname, optarg); > + exit(EXIT_FAILURE); > + } > case 'q': > params.quiet = 1; > break; > -- > 2.7.4 Regards, Simon
Thanks for the review Simon! On Thu, Jun 9, 2016 at 6:03 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Teddy, > > On 5 June 2016 at 15:18, Teddy Reed <teddy.reed@gmail.com> wrote: >> >> When building a FIT with external data (-E), an SPL may require absolute >> positioning for executing the external firmware. To acheive this use the >> (-p) switch, which will replace the amended 'data-offset' with 'data-position' >> indicating the absolute position of external data. > > Why does SPL require that? Is it because it is at an external address, > not inside the FIT? > Ah, it's more so the U-Boot proper that expects to have been built with a TEXT_BASE known to it and the SPL. I'm trying to accommodate a scenario where U-Boot proper executes after being verified by the SPL, without needing arguments or relocation. Given an execute-in-place without arguments U-Boot proper must know TEXT_BASE at build time. This is clobbered when the external FIT is generated, if the firmware is placed at a relative position. With a -p your U_BOOT_TEXT_BASE remains constant between build and external FIT generation. >> >> It is considered an error if the requested absolute position overlaps with the >> initial data required for the compact FIT. >> >> Cc: Simon Glass <sjg@chromium.org> >> --- >> >> Changes in v2: >> - Add -p argument to mkimage.1 >> - Add -E, -p arguments to mkimage usage text >> - Add notes for static position within uImage.FIT docs >> - Rebased onto master >> >> doc/mkimage.1 | 6 ++++++ >> doc/uImage.FIT/source_file_format.txt | 4 ++++ >> tools/fit_image.c | 19 ++++++++++++++++++- >> tools/imagetool.h | 1 + >> tools/mkimage.c | 13 +++++++++++-- >> 5 files changed, 40 insertions(+), 3 deletions(-) >> >> diff --git a/doc/mkimage.1 b/doc/mkimage.1 >> index 4b3a255..ffa7d60 100644 >> --- a/doc/mkimage.1 >> +++ b/doc/mkimage.1 >> @@ -152,6 +152,12 @@ verification. Typically the file here is the device tree binary used by >> CONFIG_OF_CONTROL in U-Boot. >> >> .TP >> +.BI "\-p [" "external position" "]" >> +Place external data at a static external position. See \-E. Instead of writing >> +a 'data-offset' property defining the offset from the end of the FIT, \-p will >> +use 'data-position' as the absolute position from the base of the FIT. >> + >> +.TP >> .BI "\-r >> Specifies that keys used to sign the FIT are required. This means that they >> must be verified for the image to boot. Without this option, the verification >> diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt >> index 3f54180..ee72740 100644 >> --- a/doc/uImage.FIT/source_file_format.txt >> +++ b/doc/uImage.FIT/source_file_format.txt >> @@ -282,6 +282,10 @@ In this case the 'data' property is omitted. Instead you can use: >> aligned to a 4-byte boundary. >> - data-size : size of the data in bytes >> >> +The 'data-offset' property can be substituted with 'data-position', which >> +defines a static position or address from the base of the FIT as the offset. >> +A static position is helpful when booting U-Boot proper before performing >> +relocation. > > This confuses me since it mentions a static address, but then > references the base of the FIT, suggesting it is relative. > > Can you use the word 'absolute' instead of 'static'? > Absolutely! ;) >> >> 9) Examples >> ----------- >> diff --git a/tools/fit_image.c b/tools/fit_image.c >> index 0551572..76a6de4 100644 >> --- a/tools/fit_image.c >> +++ b/tools/fit_image.c >> @@ -416,7 +416,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) >> ret = -EPERM; >> goto err_munmap; >> } >> - fdt_setprop_u32(fdt, node, "data-offset", buf_ptr); >> + if (params->external_offset > 0) { >> + /* An external offset positions the data absolutely. */ >> + fdt_setprop_u32(fdt, node, "data-position", >> + params->external_offset + buf_ptr); >> + } else { >> + fdt_setprop_u32(fdt, node, "data-offset", buf_ptr); >> + } >> fdt_setprop_u32(fdt, node, "data-size", len); >> >> buf_ptr += (len + 3) & ~3; >> @@ -437,6 +443,17 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) >> ret = -EIO; >> goto err; >> } >> + >> + /* Check if an offset for the external data was set. */ >> + if (params->external_offset > 0) { >> + if (params->external_offset < new_size) { >> + debug("External offset %x overlaps FIT length %x", >> + params->external_offset, new_size); >> + ret = -EINVAL; >> + goto err; >> + } >> + new_size = params->external_offset; >> + } >> if (lseek(fd, new_size, SEEK_SET) < 0) { >> debug("%s: Failed to seek to end of file: %s\n", __func__, >> strerror(errno)); >> diff --git a/tools/imagetool.h b/tools/imagetool.h >> index a3ed0f4..1f2161c 100644 >> --- a/tools/imagetool.h >> +++ b/tools/imagetool.h >> @@ -74,6 +74,7 @@ struct image_tool_params { >> struct content_info *content_tail; >> 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 */ >> }; >> >> /* >> diff --git a/tools/mkimage.c b/tools/mkimage.c >> index aefe22f..ff3024a 100644 >> --- a/tools/mkimage.c >> +++ b/tools/mkimage.c >> @@ -93,11 +93,13 @@ static void usage(const char *msg) >> " -f => input filename for FIT source\n"); >> #ifdef CONFIG_FIT_SIGNATURE >> fprintf(stderr, >> - "Signing / verified boot options: [-k keydir] [-K dtb] [ -c <comment>] [-r]\n" >> + "Signing / verified boot options: [-E] [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r]\n" >> + " -E => place data outside of the FIT structure\n" >> " -k => set directory containing private keys\n" >> " -K => write public keys to this .dtb file\n" >> " -c => add comment in signature node\n" >> " -F => re-sign existing FIT image\n" >> + " -p => place external data at a static position\n" >> " -r => mark keys used as 'required' in dtb\n"); >> #else >> fprintf(stderr, >> @@ -136,7 +138,7 @@ static void process_args(int argc, char **argv) >> int opt; >> >> while ((opt = getopt(argc, argv, >> - "a:A:b:cC:d:D:e:Ef:Fk:K:ln:O:rR:qsT:vVx")) != -1) { >> + "a:A:b:cC:d:D:e:Ef:Fk:K:ln:p:O:rR:qsT:vVx")) != -1) { >> switch (opt) { >> case 'a': >> params.addr = strtoull(optarg, &ptr, 16); >> @@ -216,6 +218,13 @@ static void process_args(int argc, char **argv) >> if (params.os < 0) >> usage("Invalid operating system"); >> break; >> + case 'p': >> + params.external_offset = strtoull(optarg, &ptr, 16); >> + if (*ptr) { >> + fprintf(stderr, "%s: invalid offset size %s\n", >> + params.cmdname, optarg); >> + exit(EXIT_FAILURE); >> + } >> case 'q': >> params.quiet = 1; >> break; >> -- >> 2.7.4
Hi Teddy, On 9 June 2016 at 19:02, Teddy Reed <teddy.reed@gmail.com> wrote: > Thanks for the review Simon! > > On Thu, Jun 9, 2016 at 6:03 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Teddy, >> >> On 5 June 2016 at 15:18, Teddy Reed <teddy.reed@gmail.com> wrote: >>> >>> When building a FIT with external data (-E), an SPL may require absolute >>> positioning for executing the external firmware. To acheive this use the >>> (-p) switch, which will replace the amended 'data-offset' with 'data-position' >>> indicating the absolute position of external data. >> >> Why does SPL require that? Is it because it is at an external address, >> not inside the FIT? >> > > Ah, it's more so the U-Boot proper that expects to have been built > with a TEXT_BASE known to it and the SPL. I'm trying to accommodate a > scenario where U-Boot proper executes after being verified by the SPL, > without needing arguments or relocation. > > Given an execute-in-place without arguments U-Boot proper must know > TEXT_BASE at build time. This is clobbered when the external FIT is > generated, if the firmware is placed at a relative position. With a -p > your U_BOOT_TEXT_BASE remains constant between build and external FIT > generation. I'm stil not quite clear on this. SPL has to know where to load U-Boot. The existing FIT support uses CONFIG_SYS_TEXT_BASE for this. spl_load_simple_fit() should load U-Boot proper to that address. The position of U-Boot in the FIT itself should not be relevant. What sort of relocation are you trrying to avoid? > >>> >>> It is considered an error if the requested absolute position overlaps with the >>> initial data required for the compact FIT. >>> >>> Cc: Simon Glass <sjg@chromium.org> >>> --- >>> >>> Changes in v2: >>> - Add -p argument to mkimage.1 >>> - Add -E, -p arguments to mkimage usage text >>> - Add notes for static position within uImage.FIT docs >>> - Rebased onto master >>> >>> doc/mkimage.1 | 6 ++++++ >>> doc/uImage.FIT/source_file_format.txt | 4 ++++ >>> tools/fit_image.c | 19 ++++++++++++++++++- >>> tools/imagetool.h | 1 + >>> tools/mkimage.c | 13 +++++++++++-- >>> 5 files changed, 40 insertions(+), 3 deletions(-) >>> >>> diff --git a/doc/mkimage.1 b/doc/mkimage.1 >>> index 4b3a255..ffa7d60 100644 >>> --- a/doc/mkimage.1 >>> +++ b/doc/mkimage.1 >>> @@ -152,6 +152,12 @@ verification. Typically the file here is the device tree binary used by >>> CONFIG_OF_CONTROL in U-Boot. >>> >>> .TP >>> +.BI "\-p [" "external position" "]" >>> +Place external data at a static external position. See \-E. Instead of writing >>> +a 'data-offset' property defining the offset from the end of the FIT, \-p will >>> +use 'data-position' as the absolute position from the base of the FIT. >>> + >>> +.TP >>> .BI "\-r >>> Specifies that keys used to sign the FIT are required. This means that they >>> must be verified for the image to boot. Without this option, the verification >>> diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt >>> index 3f54180..ee72740 100644 >>> --- a/doc/uImage.FIT/source_file_format.txt >>> +++ b/doc/uImage.FIT/source_file_format.txt >>> @@ -282,6 +282,10 @@ In this case the 'data' property is omitted. Instead you can use: >>> aligned to a 4-byte boundary. >>> - data-size : size of the data in bytes >>> >>> +The 'data-offset' property can be substituted with 'data-position', which >>> +defines a static position or address from the base of the FIT as the offset. >>> +A static position is helpful when booting U-Boot proper before performing >>> +relocation. >> >> This confuses me since it mentions a static address, but then >> references the base of the FIT, suggesting it is relative. >> >> Can you use the word 'absolute' instead of 'static'? >> > > Absolutely! ;) Regards, Simon
diff --git a/doc/mkimage.1 b/doc/mkimage.1 index 4b3a255..ffa7d60 100644 --- a/doc/mkimage.1 +++ b/doc/mkimage.1 @@ -152,6 +152,12 @@ verification. Typically the file here is the device tree binary used by CONFIG_OF_CONTROL in U-Boot. .TP +.BI "\-p [" "external position" "]" +Place external data at a static external position. See \-E. Instead of writing +a 'data-offset' property defining the offset from the end of the FIT, \-p will +use 'data-position' as the absolute position from the base of the FIT. + +.TP .BI "\-r Specifies that keys used to sign the FIT are required. This means that they must be verified for the image to boot. Without this option, the verification diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt index 3f54180..ee72740 100644 --- a/doc/uImage.FIT/source_file_format.txt +++ b/doc/uImage.FIT/source_file_format.txt @@ -282,6 +282,10 @@ In this case the 'data' property is omitted. Instead you can use: aligned to a 4-byte boundary. - data-size : size of the data in bytes +The 'data-offset' property can be substituted with 'data-position', which +defines a static position or address from the base of the FIT as the offset. +A static position is helpful when booting U-Boot proper before performing +relocation. 9) Examples ----------- diff --git a/tools/fit_image.c b/tools/fit_image.c index 0551572..76a6de4 100644 --- a/tools/fit_image.c +++ b/tools/fit_image.c @@ -416,7 +416,13 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) ret = -EPERM; goto err_munmap; } - fdt_setprop_u32(fdt, node, "data-offset", buf_ptr); + if (params->external_offset > 0) { + /* An external offset positions the data absolutely. */ + fdt_setprop_u32(fdt, node, "data-position", + params->external_offset + buf_ptr); + } else { + fdt_setprop_u32(fdt, node, "data-offset", buf_ptr); + } fdt_setprop_u32(fdt, node, "data-size", len); buf_ptr += (len + 3) & ~3; @@ -437,6 +443,17 @@ static int fit_extract_data(struct image_tool_params *params, const char *fname) ret = -EIO; goto err; } + + /* Check if an offset for the external data was set. */ + if (params->external_offset > 0) { + if (params->external_offset < new_size) { + debug("External offset %x overlaps FIT length %x", + params->external_offset, new_size); + ret = -EINVAL; + goto err; + } + new_size = params->external_offset; + } if (lseek(fd, new_size, SEEK_SET) < 0) { debug("%s: Failed to seek to end of file: %s\n", __func__, strerror(errno)); diff --git a/tools/imagetool.h b/tools/imagetool.h index a3ed0f4..1f2161c 100644 --- a/tools/imagetool.h +++ b/tools/imagetool.h @@ -74,6 +74,7 @@ struct image_tool_params { struct content_info *content_tail; 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 */ }; /* diff --git a/tools/mkimage.c b/tools/mkimage.c index aefe22f..ff3024a 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -93,11 +93,13 @@ static void usage(const char *msg) " -f => input filename for FIT source\n"); #ifdef CONFIG_FIT_SIGNATURE fprintf(stderr, - "Signing / verified boot options: [-k keydir] [-K dtb] [ -c <comment>] [-r]\n" + "Signing / verified boot options: [-E] [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r]\n" + " -E => place data outside of the FIT structure\n" " -k => set directory containing private keys\n" " -K => write public keys to this .dtb file\n" " -c => add comment in signature node\n" " -F => re-sign existing FIT image\n" + " -p => place external data at a static position\n" " -r => mark keys used as 'required' in dtb\n"); #else fprintf(stderr, @@ -136,7 +138,7 @@ static void process_args(int argc, char **argv) int opt; while ((opt = getopt(argc, argv, - "a:A:b:cC:d:D:e:Ef:Fk:K:ln:O:rR:qsT:vVx")) != -1) { + "a:A:b:cC:d:D:e:Ef:Fk:K:ln:p:O:rR:qsT:vVx")) != -1) { switch (opt) { case 'a': params.addr = strtoull(optarg, &ptr, 16); @@ -216,6 +218,13 @@ static void process_args(int argc, char **argv) if (params.os < 0) usage("Invalid operating system"); break; + case 'p': + params.external_offset = strtoull(optarg, &ptr, 16); + if (*ptr) { + fprintf(stderr, "%s: invalid offset size %s\n", + params.cmdname, optarg); + exit(EXIT_FAILURE); + } case 'q': params.quiet = 1; break;