[U-Boot] [PATCH v2] mkimage: fit: spl: Add an optional static offset for external data
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.
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.
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;
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'?
- 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! ;)
- 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
participants (2)
-
Simon Glass -
Teddy Reed