[U-Boot] [PATCH v2] mkimage: fit: spl: Add an optional static offset for external data
Teddy Reed
teddy.reed at gmail.com
Fri Jun 10 04:02:47 CEST 2016
Thanks for the review Simon!
On Thu, Jun 9, 2016 at 6:03 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi Teddy,
>
> On 5 June 2016 at 15:18, Teddy Reed <teddy.reed at 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 at 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
--
Teddy Reed V
More information about the U-Boot
mailing list