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

Hi Teddy,

On 2 May 2016 at 02:40, Teddy Reed teddy.reed@gmail.com wrote:

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.

OK I see.

...

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!

Sounds good to me, yes.

Regards, Simon

...

...

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