[U-Boot] [PATCH 1/2 v2] fit: Support compression for non-kernel components (e.g. FDT)

Simon Goldschmidt simon.k.r.goldschmidt at gmail.com
Wed May 1 08:18:50 UTC 2019



On 01.05.19 00:34, Julius Werner wrote:
> On Tue, Apr 30, 2019 at 11:25 AM Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com> wrote:
>>
>> Am 18.04.2019 um 23:08 schrieb Julius Werner:
>>> This patch adds support for compressing non-kernel image nodes in a FIT
>>> image (kernel nodes could already be compressed previously). This can
>>> reduce the size of FIT images and therefore improve boot times
>>> (especially when an image bundles many different kernel FDTs). The
>>> images will automatically be decompressed on load.
>>>
>>> This patch does not support extracting compatible strings from
>>> compressed FDTs, so it's not very helpful in conjunction with
>>> CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments
>>> that select the configuration to load explicitly.
>>>
>>> Signed-off-by: Julius Werner <jwerner at chromium.org>
>>> ---
>>>    common/image-fit.c | 83 +++++++++++++++++++++++++++-------------------
>>>    1 file changed, 49 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>> index ac901e131c..006e828b79 100644
>>> --- a/common/image-fit.c
>>> +++ b/common/image-fit.c
>>> @@ -22,6 +22,7 @@
>>>    DECLARE_GLOBAL_DATA_PTR;
>>>    #endif /* !USE_HOSTCC*/
>>>
>>> +#include <bootm.h>
>>>    #include <image.h>
>>>    #include <bootstage.h>
>>>    #include <u-boot/crc.h>
>>> @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void *fdt)
>>>                              kfdt_name);
>>>                        continue;
>>>                }
>>> +
>>> +             if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) {
>>> +                     debug("Can't extract compat from \"%s\" (compressed)\n",
>>> +                           kfdt_name);
>>> +                     continue;
>>> +             }
>>> +
>>
>> What's this block good for in this patch? Looks like it belongs to 2/2?
> 
> This is necessary because I'm removing the (image_type ==
> IH_TYPE_FLATDT && !fit_image_check_comp(fit, noffset, IH_COMP_NONE))
> check below. Previously, this function would just always hard fail
> with compressed FDTs. With this patch, compressed FDTs can be loaded,
> but they can't be used for compat string matching -- that's why I need
> to add this check here to prevent it. You can still use compressed
> FDTs when selecting a configuration explicitly (e.g. bootm
> 0x1000000#conf at 1). The next patch will then add another feature that
> makes it possible to have compat string matching with compressed FDTs.

Oh, ok. Guess I was just surprised that it says "can't extract compat" 
when it tries to check "comp"...

> 
>>
>>>                /*
>>>                 * Get a pointer to this configuration's fdt.
>>>                 */
>>> @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>>        const char *fit_uname_config;
>>>        const char *fit_base_uname_config;
>>>        const void *fit;
>>> -     const void *buf;
>>> +     void *buf;
>>> +     void *loadbuf;
>>>        size_t size;
>>>        int type_ok, os_ok;
>>> -     ulong load, data, len;
>>> -     uint8_t os;
>>> +     ulong load, load_end, data, len;
>>> +     uint8_t os, comp;
>>>    #ifndef USE_HOSTCC
>>>        uint8_t os_arch;
>>>    #endif
>>> @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>>        images->os.arch = os_arch;
>>>    #endif
>>>
>>> -     if (image_type == IH_TYPE_FLATDT &&
>>> -         !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) {
>>> -             puts("FDT image is compressed");
>>> -             return -EPROTONOSUPPORT;
>>> -     }
>>> -
>>>        bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL);
>>>        type_ok = fit_image_check_type(fit, noffset, image_type) ||
>>>                  fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) ||
>>> @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>>        bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK);
>>>
>>>        /* get image data address and length */
>>> -     if (fit_image_get_data_and_size(fit, noffset, &buf, &size)) {
>>> +     if (fit_image_get_data_and_size(fit, noffset,
>>> +                                     (const void **)&buf, &size)) {
>>>                printf("Could not find %s subimage data!\n", prop_name);
>>>                bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA);
>>>                return -ENOENT;
>>> @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>>
>>>    #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS)
>>>        /* perform any post-processing on the image data */
>>> -     board_fit_image_post_process((void **)&buf, &size);
>>> +     board_fit_image_post_process(&buf, &size);
>>>    #endif
>>>
>>>        len = (ulong)size;
>>>
>>> -     /* verify that image data is a proper FDT blob */
>>> -     if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) {
>>> -             puts("Subimage data is not a FDT");
>>> -             return -ENOEXEC;
>>> -     }
>>> -
>>>        bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK);
>>>
>>> -     /*
>>> -      * Work-around for eldk-4.2 which gives this warning if we try to
>>> -      * cast in the unmap_sysmem() call:
>>> -      * warning: initialization discards qualifiers from pointer target type
>>> -      */
>>> -     {
>>> -             void *vbuf = (void *)buf;
>>> -
>>> -             data = map_to_sysmem(vbuf);
>>> -     }
>>> -
>>> +     data = map_to_sysmem(buf);
>>> +     load = data;
>>>        if (load_op == FIT_LOAD_IGNORED) {
>>>                /* Don't load */
>>>        } else if (fit_image_get_load(fit, noffset, &load)) {
>>> @@ -1974,8 +1963,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>>                }
>>>        } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
>>>                ulong image_start, image_end;
>>> -             ulong load_end;
>>> -             void *dst;
>>>
>>>                /*
>>>                 * move image data to the load address,
>>> @@ -1993,14 +1980,42 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>>>
>>>                printf("   Loading %s from 0x%08lx to 0x%08lx\n",
>>>                       prop_name, data, load);
>>> +     }
>>>
>>> -             dst = map_sysmem(load, len);
>>> -             memmove(dst, buf, len);
>>> -             data = load;
>>> +     comp = IH_COMP_NONE;
>>> +     loadbuf = buf;
>>> +     /* Kernel images get decompressed later in bootm_host_load_image(). */
>>> +     if (!(image_type == IH_TYPE_KERNEL ||
>>> +           image_type == IH_TYPE_KERNEL_NOLOAD )&&
>>
>> Can we go without this special case for Kernel? Back last year in the
>> discussion we had, we decided that it would be better to have things
>> straightforward and let the FIT property "compression" decide about if
>> the loader should uncompress things, not if the file behind is a
>> compressed file (e.g. initrd can be passed compressed to the kernel, so
>> use "compression=none" in the FIT).
>>
>> Leaving away the extra case in the FIT loader makes the code less hard
>> to read.
>>
>> I had to add a patch to bootm.c, too (maybe because of leaving away the
>> extra case for Kernel?):
>> [1] https://patchwork.ozlabs.org/patch/984996/
> 
> Yes, I agree decompressing everything here would be better. But the
> kernel compression is pretty ingrained in bootm, also for boot modes
> that work without a FIT image, and I'm honestly not sure if I
> understand all other possibilities of how code could end up in
> bootm_load_os() well enough to be able to make and test changes that
> will keep them all working. There's also the problem of the
> decompression buffer... for FDTs and other minor components malloc()
> should work well enough in most cases, but for the kernel there may
> not be enough space, so we would still need a special case anyway (to
> use CONFIG_SYS_BOOTM_LEN in that case). I feel all of this needs more
> thought and would not be that trivial to get right. Can we please
> defer this to future work (I think this patch already adds notable
> benefit as is, and is self-contained enough that I can be confident it
> shouldn't break any existing uses)?

Ok for me. I think my patch did cover that, but I can re-test once this 
is in and send a follow-up.

> 
>>> +         !fit_image_get_comp(fit, noffset, &comp) &&
>>> +         comp != IH_COMP_NONE) {
>>> +             ulong max_decomp_len = len * 20;
>>
>> Now that's a theoretical limit which I've kept at 0 to make
>> 'bootm_decomp_image' use CONFIG_SYS_BOOTM_LEN as a maximum. See [1] above.
>>
>> To keep malloc/lmb_alloc block small, it would be best to know the size
>> in advance, but we can work on that later.
>>
>> Given that and that CONFIG_SYS_BOOTM_LEN isn't available globally, let's
>> start with your 'len * 20' and I'll supply the uncompressed size via
>> mkimage as a follow-up (I've been working on that already last year, too).
> 
> Agreed, it would be great to have a better solution here eventually.
> 
>>> +             if (load == data) {
>>> +                     loadbuf = malloc(max_decomp_len);
>>
>> For decompressing FDT, using malloc should be OK, but for larger things
>> (initrd/FPGA image), we should probably use lmb_alloc() (again, see
>> discussion in [1]).
> 
> So... how should I change this? I don't really understand how lmb
> works to be honest. Where do I get the struct lmb from that I have to
> pass into lmb_alloc()?
> 
> It also seems like there's a CONFIG_LMB that needs to be enabled for
> this to work... is that guaranteed to be available for all boards that
> support FIT images?
> 
> Maybe this is also something we could leave for future work?

Yes, we can. When a load address is present, this patch should not be 
executed anyway.

> 
>>> +                     load = map_to_sysmem(loadbuf);
>>> +             } else {
>>> +                     loadbuf = map_sysmem(load, max_decomp_len);
>>> +             }
>>> +             if (bootm_decomp_image(comp, load, data, IH_TYPE_FLATDT,
>>
>> You should probably use 'image_type' here instead of IH_TYPE_FLATDT.
>>
>>> +                             loadbuf, buf, len, max_decomp_len, &load_end)) {
>>> +                     printf("Error: Cannot decompress FDT image\n");
>>
>> Again, 'FDT' is wrong here now.
> 
> Right, those two were holdovers from the previous version, will fix.

OK, expecting v3 then. I think patchwork didn't get this one as v2 as 
you put the v2 behind the 1/2.

Regards,
Simon

> 
>>> +                     return -ENOEXEC;
>>> +             }
>>> +             len = load_end - load;
>>> +     } else if (load != data) {
>>> +             loadbuf = map_sysmem(load, len);
>>> +             memcpy(loadbuf, buf, len);
>>>        }
>>> +
>>> +     /* verify that image data is a proper FDT blob */
>>> +     if (image_type == IH_TYPE_FLATDT && fdt_check_header(loadbuf)) {
>>> +             puts("Subimage data is not a FDT");
>>> +             return -ENOEXEC;
>>> +     }
>>> +
>>>        bootstage_mark(bootstage_id + BOOTSTAGE_SUB_LOAD);
>>>
>>> -     *datap = data;
>>> +     *datap = load;
>>>        *lenp = len;
>>>        if (fit_unamep)
>>>                *fit_unamep = (char *)fit_uname;
>>>


More information about the U-Boot mailing list