[U-Boot] [PATCH 0/2] fit: Image node compression

[U-Boot] [PATCH 0/2] fit: Image node compression

[Julius Werner]

This patch series adds compression support for non-kernel FIT image
nodes (e.g. FDTs). The first patch adds the compression support
itself, the second adds a new feature to compatible string matching
that allows it to be useful with compressed FDTs.

Sandbox-tested with FIT images with and without compressed FDTs, with
and without overlays, in both compatible string matching and direct
config selection modes.

Julius Werner (2):
  fit: Support compression for non-kernel components (e.g. FDT)
    - Changes for v2:
      - Changed from only supporting compressed FDTs to supporting all
	non-kernel image node types.
  fit: Support compat string property in configuration node

 common/image-fit.c | 140 +++++++++++++++++++++++++++------------------
 1 file changed, 83 insertions(+), 57 deletions(-)

-- 
2.20.1

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

[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@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;
+		}
+
 		/*
 		 * 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 )&&
+	    !fit_image_get_comp(fit, noffset, &comp) &&
+	    comp != IH_COMP_NONE) {
+		ulong max_decomp_len = len * 20;
+		if (load == data) {
+			loadbuf = malloc(max_decomp_len);
+			load = map_to_sysmem(loadbuf);
+		} else {
+			loadbuf = map_sysmem(load, max_decomp_len);
+		}
+		if (bootm_decomp_image(comp, load, data, IH_TYPE_FLATDT,
+				loadbuf, buf, len, max_decomp_len, &load_end)) {
+			printf("Error: Cannot decompress FDT image\n");
+			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;
-- 
2.20.1

[U-Boot] [PATCH 2/2 v2] fit: Support compat string property in configuration node

[Julius Werner]

This patch adds support for an optional optimization to compatible
string matching where the compatible string property from the root node
of the kernel FDT can be copied into the configuration node of the FIT
image. This is most useful when using compressed FDTs or when using FDT
overlays, where the traditional extraction of the compatible string from
the kernel FDT itself is not easily possible.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 common/image-fit.c | 67 +++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index 006e828b79..4602db8414 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1522,6 +1522,10 @@ int fit_check_format(const void *fit)
  * compatible list, "foo,bar", matches a compatible string in the root of fdt1.
  * "bim,bam" in fdt2 matches the second string which isn't as good as fdt1.
  *
+ * As an optimization, the compatible property from the FDT's root node can be
+ * copied into the configuration node in the FIT image. This is required to
+ * match configurations with compressed FDTs.
+ *
  * returns:
  *     offset to the configuration to use if one was found
  *     -1 otherwise
@@ -1554,55 +1558,62 @@ int fit_conf_find_compat(const void *fit, const void *fdt)
 	for (noffset = fdt_next_node(fit, confs_noffset, &ndepth);
 			(noffset >= 0) && (ndepth > 0);
 			noffset = fdt_next_node(fit, noffset, &ndepth)) {
-		const void *kfdt;
+		const void *fdt;
 		const char *kfdt_name;
-		int kfdt_noffset;
+		int kfdt_noffset, compat_noffset;
 		const char *cur_fdt_compat;
 		int len;
-		size_t size;
+		size_t sz;
 		int i;
 
 		if (ndepth > 1)
 			continue;
 
-		kfdt_name = fdt_getprop(fit, noffset, "fdt", &len);
-		if (!kfdt_name) {
-			debug("No fdt property found.\n");
-			continue;
-		}
-		kfdt_noffset = fdt_subnode_offset(fit, images_noffset,
-						  kfdt_name);
-		if (kfdt_noffset < 0) {
-			debug("No image node named \"%s\" found.\n",
-			      kfdt_name);
-			continue;
-		}
+		/* If there's a compat property in the config node, use that. */
+		if (fdt_getprop(fit, noffset, "compatible", NULL)) {
+			fdt = fit;		  /* search in FIT image */
+			compat_noffset = noffset; /* search under config node */
+		} else {	/* Otherwise extract it from the kernel FDT. */
+			kfdt_name = fdt_getprop(fit, noffset, "fdt", &len);
+			if (!kfdt_name) {
+				debug("No fdt property found.\n");
+				continue;
+			}
+			kfdt_noffset = fdt_subnode_offset(fit, images_noffset,
+							  kfdt_name);
+			if (kfdt_noffset < 0) {
+				debug("No image node named \"%s\" found.\n",
+				      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;
-		}
+			if (!fit_image_check_comp(fit, kfdt_noffset,
+						  IH_COMP_NONE)) {
+				debug("Can't extract compat from \"%s\" "
+				      "(compressed)\n", kfdt_name);
+				continue;
+			}
 
-		/*
-		 * Get a pointer to this configuration's fdt.
-		 */
-		if (fit_image_get_data(fit, kfdt_noffset, &kfdt, &size)) {
-			debug("Failed to get fdt \"%s\".\n", kfdt_name);
-			continue;
+			/* search in this config's kernel FDT */
+			if (fit_image_get_data(fit, kfdt_noffset, &fdt, &sz)) {
+				debug("Failed to get fdt \"%s\".\n", kfdt_name);
+				continue;
+			}
+
+			compat_noffset = 0;  /* search kFDT under root node */
 		}
 
 		len = fdt_compat_len;
 		cur_fdt_compat = fdt_compat;
 		/*
 		 * Look for a match for each U-Boot compatibility string in
-		 * turn in this configuration's fdt.
+		 * turn in the compat string property.
 		 */
 		for (i = 0; len > 0 &&
 		     (!best_match_offset || best_match_pos > i); i++) {
 			int cur_len = strlen(cur_fdt_compat) + 1;
 
-			if (!fdt_node_check_compatible(kfdt, 0,
+			if (!fdt_node_check_compatible(fdt, compat_noffset,
 						       cur_fdt_compat)) {
 				best_match_offset = noffset;
 				best_match_pos = i;
-- 
2.20.1

[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@chromium.org>

Just to note it in this thread, too: this is somewhat similar to what I 
suggested in this RFC last year:
https://patchwork.ozlabs.org/patch/984996/

So I tested this and it doesn't really seem to work for me.
Somehow, the Kernel loaded from my FIT image doesn't start, but I'm not 
convinced that's related to this patch.

However, compared to my above mentioned RFC patch, this here somehow 
fails when loading something from the image, e.g. using the cmd 'fpga 
loadmk'. Seems like this one doesn't transparently uncompress?

Regards,
Simon


> ---
>   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;
> +		}
> +
>   		/*
>   		 * 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 )&&
> +	    !fit_image_get_comp(fit, noffset, &comp) &&
> +	    comp != IH_COMP_NONE) {
> +		ulong max_decomp_len = len * 20;
> +		if (load == data) {
> +			loadbuf = malloc(max_decomp_len);
> +			load = map_to_sysmem(loadbuf);
> +		} else {
> +			loadbuf = map_sysmem(load, max_decomp_len);
> +		}
> +		if (bootm_decomp_image(comp, load, data, IH_TYPE_FLATDT,
> +				loadbuf, buf, len, max_decomp_len, &load_end)) {
> +			printf("Error: Cannot decompress FDT image\n");
> +			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;
> 

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

[Julius Werner]

> However, compared to my above mentioned RFC patch, this here somehow
> fails when loading something from the image, e.g. using the cmd 'fpga
> loadmk'. Seems like this one doesn't transparently uncompress?

It looks to me like do_fpga_loadmk() doesn't actually call
fit_load_image()? It calls fit_image_get_data() directly, so it
bypasses the code I'm adding. Maybe it should be using
fit_load_image() instead, but that's moving pretty far away from what
I was trying to do... it could be left for a later patch?

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

[Simon Goldschmidt]

Julius Werner <jwerner@chromium.org> schrieb am Di., 30. Apr. 2019, 02:38:

> > However, compared to my above mentioned RFC patch, this here somehow
> > fails when loading something from the image, e.g. using the cmd 'fpga
> > loadmk'. Seems like this one doesn't transparently uncompress?
>
> It looks to me like do_fpga_loadmk() doesn't actually call
> fit_load_image()? It calls fit_image_get_data() directly, so it
> bypasses the code I'm adding. Maybe it should be using
> fit_load_image() instead, but that's moving pretty far away from what
> I was trying to do... it could be left for a later patch?
>

Ah, ok. I wasn't aware I had changed that command too :-) let me
concentrate on finding out why booting the compressed fit,image doesn't
work, then.

Regards,
Simon

>

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

[Simon Goldschmidt]

Am 30.04.2019 um 06:05 schrieb Simon Goldschmidt:
> 
> 
> Julius Werner <jwerner at chromium.org <mailto:jwerner@chromium.org>> 
> schrieb am Di., 30. Apr. 2019, 02:38:
> 
>      > However, compared to my above mentioned RFC patch, this here somehow
>      > fails when loading something from the image, e.g. using the cmd 'fpga
>      > loadmk'. Seems like this one doesn't transparently uncompress?
> 
>     It looks to me like do_fpga_loadmk() doesn't actually call
>     fit_load_image()? It calls fit_image_get_data() directly, so it
>     bypasses the code I'm adding. Maybe it should be using
>     fit_load_image() instead, but that's moving pretty far away from what
>     I was trying to do... it could be left for a later patch?
> 
> 
> Ah, ok. I wasn't aware I had changed that command too :-) let me 
> concentrate on finding out why booting the compressed fit,image doesn't 
> work, then.

OK, so it turns out the error was on my side somewhere. I tested this 
with a known-to-work configuration at work today and it works like a 
charm, so I'll start to comment the contents of the patch again ;-)

Regards,
Simon

[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@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

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

[Julius Werner]

On Tue, Apr 30, 2019 at 11:25 AM Simon Goldschmidt
<simon.k.r.goldschmidt@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@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.

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

> > +         !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?

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

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

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

[Simon Goldschmidt]

On 01.05.19 00:34, Julius Werner wrote:
> On Tue, Apr 30, 2019 at 11:25 AM Simon Goldschmidt
> <simon.k.r.goldschmidt@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@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;
>>>

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

[Simon Glass]

Hi Julius,

On Thu, 18 Apr 2019 at 15:09, Julius Werner <jwerner@chromium.org> wrote:
>
> 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@chromium.org>
> ---
>  common/image-fit.c | 83 +++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 34 deletions(-)

We should get a test together for this. There is an existing
test_fit.py which might be expanded, or perhaps create a new one and
share some code.

>
> 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;
> +               }
> +
>                 /*
>                  * 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);
> -       }
> -

We don't support gcc 4.2 now so this code can be simplified to the
line you have below, but perhaps this should be in a separate patch?

> +       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(). */

But only for the host, right?

> +       if (!(image_type == IH_TYPE_KERNEL ||
> +             image_type == IH_TYPE_KERNEL_NOLOAD )&&
> +           !fit_image_get_comp(fit, noffset, &comp) &&
> +           comp != IH_COMP_NONE) {
> +               ulong max_decomp_len = len * 20;
> +               if (load == data) {
> +                       loadbuf = malloc(max_decomp_len);
> +                       load = map_to_sysmem(loadbuf);
> +               } else {
> +                       loadbuf = map_sysmem(load, max_decomp_len);
> +               }
> +               if (bootm_decomp_image(comp, load, data, IH_TYPE_FLATDT,
> +                               loadbuf, buf, len, max_decomp_len, &load_end)) {
> +                       printf("Error: Cannot decompress FDT image\n");
> +                       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;
> --
> 2.20.1
>

Regards,
Simon

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

[Julius Werner]

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

Well, what I'm trying to say in that error is "can't extract the
compatible string *because* the FDT is compressed", but without making
it three lines long.

> We should get a test together for this. There is an existing
> test_fit.py which might be expanded, or perhaps create a new one and
> share some code.

Oh, neat, I didn't know that exists there! Added a test case with
compressed images.

> > @@ -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);
> > -       }
> > -
>
> We don't support gcc 4.2 now so this code can be simplified to the
> line you have below, but perhaps this should be in a separate patch?

Well, I just rearranged the whole thing by removing the const
qualifier from 'buf' itself, which I think makes this all way less
confusing. So this block should become obsolete regardless of compiler
version. That's somewhat intertwined with changing around all the load
address handling so I hope keeping it all in one patch is okay.

> > +       comp = IH_COMP_NONE;
> > +       loadbuf = buf;
> > +       /* Kernel images get decompressed later in bootm_host_load_image(). */
>
> But only for the host, right?

Oh, right, good catch, looked up the wrong function name. The normal
case goes through bootm_load_os().