[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