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! ;)
- 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