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

[Simon Goldschmidt]

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?

>   		/*
>   		 * 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/

> +	    !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).

> +		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]).

> +			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.

> +			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;
> 

Overall it works, but it needs little tweaks for v2 :-)

Regards,
Simon