[U-Boot] [PATCH] mkimage: fit: spl: Add an optional static offset for external data

* [U-Boot] [PATCH] mkimage: fit: spl: Add an optional static offset for external data
@ 2016-05-02  8:40 Teddy Reed
  2016-05-19  3:59 ` Simon Glass
  0 siblings, 1 reply; 4+ messages in thread
From: Teddy Reed @ 2016-05-02  8:40 UTC (permalink / raw)
  To: u-boot

Hey Simon!

On Sun, May 1, 2016 at 12:32 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Teddy,
>
> On 1 May 2016 at 11:10, 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.
>>
>> It is considered an error if the requested absolute position overlaps with the
>> initial data required for the compact FIT.
>
> Can you explain why this is useful? I'd like to understand that
> clearly for your use case.

Sure! I'm working with a board (not in upstream :/ yet), and trying to
morph the boot to use a verified-boot from SPL. The *only* thing I
need the SPL to do is read a FIT, check signatures of hashes, then
jump without arguments to the U-Boot-- no dtb selection, DRAM
initialization, or anything fancy is needed. Furthermore, I'd like the
U-Boot build not to depend on the SPL boot, this makes the transition
from pure-U-Boot-boot to SPL much easier. At the end of the day,
U-Boot is essentially booting from memory, built that way too, where
the TEXT_BASE during build is the location I'm placing it outside of
the FIT. A static 32kB offset on the media.

>
> I have considered this as a general feature, but I was thinking of
> being more explicit, e.g. add a property like:
>
> data-source
>     - source of data, valid values are:
>         - "internal" - internal to the FIT, with data-offset providing
> the offset from the start of the FIT to the data
>         - "device" - a separate device, details in property TBD
>         - "address" - a memory address
>
> What do you think?
>

I like this a lot! In fact, for my board I don't need "data-position",
since the concurrent SPL/U-Boot build know the configured 32kB offset.
I added the mutex on "data-offset" and "data-position" for some sanity
when reading back the generated FIT and hopefully so that others could
benefit from parse-time discovery of static positional offsets.

Having internal, device, address, (maybe offset too) works for me!

> Also can you please update the docs? See uImage.FIT and the mkimage man page.
>

If you'd like to continue moving forward in trying to land this patch
I can make revisions and include docs + stderr help.

Let me know!

>>
>> Signed-off-by: Teddy Reed <teddy.reed@gmail.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>  tools/fit_image.c | 19 ++++++++++++++++++-
>>  tools/imagetool.h |  1 +
>>  tools/mkimage.c   |  9 ++++++++-
>>  3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/fit_image.c b/tools/fit_image.c
>> index ddefa72..98610bf 100644
>> --- a/tools/fit_image.c
>> +++ b/tools/fit_image.c
>> @@ -417,7 +417,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;
>> @@ -438,6 +444,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 24f8f4b..7862fa3 100644
>> --- a/tools/imagetool.h
>> +++ b/tools/imagetool.h
>> @@ -73,6 +73,7 @@ struct image_tool_params {
>>         struct content_info *content_head;      /* List of files to include */
>>         struct content_info *content_tail;
>>         bool external_data;     /* Store data outside the FIT */
>> +       unsigned int external_offset;   /* Add padding to external data */
>>  };
>>
>>  /*
>> diff --git a/tools/mkimage.c b/tools/mkimage.c
>> index 2931783..85e4781 100644
>> --- a/tools/mkimage.c
>> +++ b/tools/mkimage.c
>> @@ -138,7 +138,7 @@ static void process_args(int argc, char **argv)
>>
>>         expecting = IH_TYPE_COUNT;      /* Unknown */
>>         while ((opt = getopt(argc, argv,
>> -                            "-a:A:bcC:d:D:e:Ef:Fk:K:ln:O:rR:sT:vVx")) != -1) {
>> +                            "-a:A:bcC:d:D:e:Ef:Fk:K:ln:p:O:rR:sT:vVx")) != -1) {
>>                 switch (opt) {
>>                 case 'a':
>>                         params.addr = strtoull(optarg, &ptr, 16);
>> @@ -213,6 +213,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 'r':
>>                         params.require_keys = 1;
>>                         break;
>> --
>> 1.9.1
>

Thanks!
-- 
Teddy Reed V

^ permalink raw reply	[flat|nested] 4+ messages in thread