[PATCH 00/13] boot: Make fit_image_load() easier to maintain

[PATCH 00/13] boot: Make fit_image_load() easier to maintain

[Simon Glass]

Before fit_image_load() was created, the code to load kernels, ramdisks
and devicetrees from a FIT was spread around many functions. By
combining most of the code in one place, it became possible to add more
features in a consistent way. The 'loadables' feature was much easier to
plumb in, for example.

While fit_image_load() was a substantial advance, it has never been a
svelte function and the passing years have not been entirely kind. With
a few new features on the horizon, this is a good time to improve the
implementation.

This series splits much of the code from fit_image_load() into a number
of smaller functions. Most of the changes are fairly mechanical, with
just a few renames and tweaks here and there.

This should make the function much easier to maintain. It may also
encourage someone to take a look at its callers, which could also use
some attention.

Code-size impact: about +8 bytes on aarch64 and +24-40 bytes on arm, due
to the compiler's register allocation across the new helper functions.


Simon Glass (13):
  boot: Split out the first part of fit_image_load()
  boot: Move call to fit_image_select() and rename it
  boot: Tidy up setting of the OS arch on host builds
  boot: Move type and OS checking into a new function
  boot: Move handling of the load_op into a separate function
  boot: Move obtaining data from a FIT image into a function
  boot: Check the image is allowed before setting os.arch
  boot: Move the architecture check into check_allowed()
  boot: Move image-decompression into a separate function
  boot: Move decomp_image() into handle_load_op()
  boot: Move setting the OS arch into check_allowed()
  boot: Drop unnecessary data variable
  boot: Tidy local variables in fit_image_load()

 boot/image-fit.c | 354 +++++++++++++++++++++++++++++++++++------------
 include/image.h  |  11 +-
 2 files changed, 273 insertions(+), 92 deletions(-)

-- 
2.43.0

base-commit: 1ffc541eafc96d5eebcf837ab892dccec3b93568
branch: loadb-us

[PATCH 01/13] boot: Split out the first part of fit_image_load()

[Simon Glass]

This function is over 250 lines long. Split out the image-selection
part into a new select_image() function which handles format checking,
configuration selection and image-node lookup.

Take care to only call fit_get_name() when a valid node is found, since
libfdt does not handle negative offsets gracefully.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 101 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 31 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index e7c7212195f..a4402dcff63 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2070,33 +2070,35 @@ static const char *fit_get_image_type_property(int ph_type)
 	return "unknown";
 }
 
-int fit_image_load(struct bootm_headers *images, ulong addr,
-		   const char **fit_unamep, const char **fit_uname_configp,
-		   int arch, int ph_type, int bootstage_id,
-		   enum fit_load_op load_op, ulong *datap, ulong *lenp)
+/**
+ * select_image() - Select the image to load
+ *
+ * image->fit_uname_cfg is set if the image type is IH_TYPE_KERNEL
+ *
+ * @fit: Pointer to FIT
+ * @images: Boot images structure
+ * @fit_unamep: On entry, the requested image name (e.g. "kernel") or NULL to
+ * use the default. On exit, the selected image name
+ * @fit_uname_config: Requested configuration name (e.g. "conf-1") or NULL to
+ * use the default
+ * @prop_name: Property name (in the configuration node) indicating the image
+ * to load
+ * @ph_type: Required image type (IH_TYPE_...) and phase (IH_PHASE_...)
+ * @bootstage_id: ID of starting bootstage to use for progress updates
+ * @fit_base_uname_configp: Returns config name selected, or NULL if *fit_unamep
+ * was used to select an image node
+ * Return: node offset of image node, on success, else -ve error code
+ */
+static int select_image(const void *fit, struct bootm_headers *images,
+			const char **fit_unamep, const char *fit_uname_config,
+			const char *prop_name, int ph_type, int bootstage_id,
+			const char **fit_base_uname_configp)
 {
-	int image_type = image_ph_type(ph_type);
 	int cfg_noffset, noffset;
-	const char *fit_uname;
-	const char *fit_uname_config;
-	const char *fit_base_uname_config;
-	const void *fit;
-	void *buf;
-	void *loadbuf;
-	size_t size;
-	int type_ok, os_ok;
-	ulong load, load_end, data, len;
-	uint8_t os, comp;
-	const char *prop_name;
 	int ret;
 
-	fit = map_sysmem(addr, 0);
-	fit_uname = fit_unamep ? *fit_unamep : NULL;
-	fit_uname_config = fit_uname_configp ? *fit_uname_configp : NULL;
-	fit_base_uname_config = NULL;
+	*fit_base_uname_configp = NULL;
 	prop_name = fit_get_image_type_property(ph_type);
-	printf("## Loading %s (%s) from FIT Image at %08lx ...\n",
-	       prop_name, genimg_get_phase_name(image_ph_phase(ph_type)), addr);
 
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_FORMAT);
 	ret = fit_check_format(fit, IMAGE_SIZE_INVAL);
@@ -2108,10 +2110,10 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		return ret;
 	}
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_FORMAT_OK);
-	if (fit_uname) {
+	if (*fit_unamep) {
 		/* get FIT component image node offset */
 		bootstage_mark(bootstage_id + BOOTSTAGE_SUB_UNIT_NAME);
-		noffset = fit_image_get_node(fit, fit_uname);
+		noffset = fit_image_get_node(fit, *fit_unamep);
 	} else {
 		/*
 		 * no image node unit name, try to get config
@@ -2133,11 +2135,12 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		}
 		cfg_noffset = ret;
 
-		fit_base_uname_config = fdt_get_name(fit, cfg_noffset, NULL);
-		printf("   Using '%s' configuration\n", fit_base_uname_config);
+		*fit_base_uname_configp = fdt_get_name(fit, cfg_noffset, NULL);
+		printf("   Using '%s' configuration\n",
+		       *fit_base_uname_configp);
 		/* Remember this config */
-		if (image_type == IH_TYPE_KERNEL)
-			images->fit_uname_cfg = fit_base_uname_config;
+		if (image_ph_type(ph_type) == IH_TYPE_KERNEL)
+			images->fit_uname_cfg = *fit_base_uname_configp;
 
 		if (FIT_IMAGE_ENABLE_VERIFY && images->verify) {
 			puts("   Verifying Hash Integrity ... ");
@@ -2161,10 +2164,46 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		return -ENOENT;
 	}
 
-	if (!fit_uname)
-		fit_uname = fit_get_name(fit, noffset, NULL);
+	if (!*fit_unamep)
+		*fit_unamep = fit_get_name(fit, noffset, NULL);
+
+	printf("   Trying '%s' %s subimage\n", *fit_unamep, prop_name);
+
+	return noffset;
+}
+
+int fit_image_load(struct bootm_headers *images, ulong addr,
+		   const char **fit_unamep, const char **fit_uname_configp,
+		   int arch, int ph_type, int bootstage_id,
+		   enum fit_load_op load_op, ulong *datap, ulong *lenp)
+{
+	int image_type = image_ph_type(ph_type);
+	int noffset;
+	const char *fit_uname;
+	const char *fit_uname_config;
+	const char *fit_base_uname_config;
+	const void *fit;
+	void *buf;
+	void *loadbuf;
+	size_t size;
+	int type_ok, os_ok;
+	ulong load, load_end, data, len;
+	uint8_t os, comp;
+	const char *prop_name;
+	int ret;
+
+	fit = map_sysmem(addr, 0);
+	prop_name = fit_get_image_type_property(ph_type);
+	printf("## Loading %s (%s) from FIT Image at %08lx ...\n",
+	       prop_name, genimg_get_phase_name(image_ph_phase(ph_type)), addr);
 
-	printf("   Trying '%s' %s subimage\n", fit_uname, prop_name);
+	fit_uname = fit_unamep ? *fit_unamep : NULL;
+	fit_uname_config = fit_uname_configp ? *fit_uname_configp : NULL;
+	noffset = select_image(fit, images, &fit_uname, fit_uname_config,
+			       prop_name, ph_type, bootstage_id,
+			       &fit_base_uname_config);
+	if (noffset < 0)
+		return noffset;
 
 	ret = fit_image_select(fit, noffset, images->verify);
 	if (ret) {
-- 
2.43.0

[PATCH 02/13] boot: Move call to fit_image_select() and rename it

[Simon Glass]

This function is named a bit vaguely, since it prints some info and then
does verification of the image.

Rename it to print_and_verify() and move the call to select_image().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index a4402dcff63..f8772f64cb3 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -1989,7 +1989,7 @@ int fit_get_data_conf_prop(const void *fit, const char *prop_name,
 	return fit_get_data_tail(fit, noffset, data, size);
 }
 
-static int fit_image_select(const void *fit, int rd_noffset, int verify)
+static int print_and_verify(const void *fit, int rd_noffset, int verify)
 {
 	fit_image_print(fit, rd_noffset, "   ");
 
@@ -2168,6 +2168,11 @@ static int select_image(const void *fit, struct bootm_headers *images,
 		*fit_unamep = fit_get_name(fit, noffset, NULL);
 
 	printf("   Trying '%s' %s subimage\n", *fit_unamep, prop_name);
+	ret = print_and_verify(fit, noffset, images->verify);
+	if (ret) {
+		bootstage_error(bootstage_id + BOOTSTAGE_SUB_HASH);
+		return ret;
+	}
 
 	return noffset;
 }
@@ -2190,7 +2195,6 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	ulong load, load_end, data, len;
 	uint8_t os, comp;
 	const char *prop_name;
-	int ret;
 
 	fit = map_sysmem(addr, 0);
 	prop_name = fit_get_image_type_property(ph_type);
@@ -2205,12 +2209,6 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	if (noffset < 0)
 		return noffset;
 
-	ret = fit_image_select(fit, noffset, images->verify);
-	if (ret) {
-		bootstage_error(bootstage_id + BOOTSTAGE_SUB_HASH);
-		return ret;
-	}
-
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ARCH);
 	if (!tools_build() && IS_ENABLED(CONFIG_SANDBOX)) {
 		if (!fit_image_check_target_arch(fit, noffset)) {
-- 
2.43.0

[PATCH 03/13] boot: Tidy up setting of the OS arch on host builds

[Simon Glass]

This field is not present in host builds. Create an inline function to
handle this complexity, so we can drop the #ifdef in the C file.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 10 ++--------
 include/image.h  |  7 +++++++
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index f8772f64cb3..5680cc8b496 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2193,7 +2193,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	size_t size;
 	int type_ok, os_ok;
 	ulong load, load_end, data, len;
-	uint8_t os, comp;
+	uint8_t os, comp, os_arch;
 	const char *prop_name;
 
 	fit = map_sysmem(addr, 0);
@@ -2218,14 +2218,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		}
 	}
 
-#ifndef USE_HOSTCC
-	{
-	uint8_t os_arch;
-
 	fit_image_get_arch(fit, noffset, &os_arch);
-	images->os.arch = os_arch;
-	}
-#endif
+	images_set_arch(images, os_arch);
 
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL);
 	type_ok = fit_image_check_type(fit, noffset, image_type) ||
diff --git a/include/image.h b/include/image.h
index 34efac6056d..5e11dc92559 100644
--- a/include/image.h
+++ b/include/image.h
@@ -418,6 +418,13 @@ struct bootm_headers {
 
 extern struct bootm_headers images;
 
+static inline void images_set_arch(struct bootm_headers *images, int os_arch)
+{
+#ifndef USE_HOSTCC
+	images->os.arch = os_arch;
+#endif
+}
+
 /*
  * Some systems (for example LWMON) have very short watchdog periods;
  * we must make sure to split long operations like memmove() or
-- 
2.43.0

[PATCH 04/13] boot: Move type and OS checking into a new function

[Simon Glass]

Move the code which checks whether the image can be loaded into a
separate check_allowed() function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 98 ++++++++++++++++++++++++++++++------------------
 include/image.h  |  4 +-
 2 files changed, 63 insertions(+), 39 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 5680cc8b496..3a6120e9151 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2177,9 +2177,64 @@ static int select_image(const void *fit, struct bootm_headers *images,
 	return noffset;
 }
 
+/**
+ * check_allowed() - Check if an image is allowed to be loaded
+ *
+ * @fit: FIT to check
+ * @noffset: Node offset of the image being loaded
+ * @image_type: Type of the image
+ * @arch: Expected architecture for the image
+ * @bootstage_id: ID of starting bootstage to use for progress updates
+ * Return: 0 if OK, -EIO if not
+ */
+static int check_allowed(const void *fit, int noffset,
+			 enum image_type_t image_type, enum image_arch_t arch,
+			 int bootstage_id)
+{
+	bool type_ok, os_ok;
+	uint8_t os;
+
+	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) ||
+		  fit_image_check_type(fit, noffset, IH_TYPE_TEE) ||
+		  fit_image_check_type(fit, noffset, IH_TYPE_TFA_BL31) ||
+		  (image_type == IH_TYPE_KERNEL &&
+		   fit_image_check_type(fit, noffset, IH_TYPE_KERNEL_NOLOAD));
+
+	os_ok = image_type == IH_TYPE_FLATDT ||
+		image_type == IH_TYPE_FPGA ||
+		fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
+		fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
+		fit_image_check_os(fit, noffset, IH_OS_TEE) ||
+		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
+		fit_image_check_os(fit, noffset, IH_OS_EFI) ||
+		fit_image_check_os(fit, noffset, IH_OS_VXWORKS) ||
+		fit_image_check_os(fit, noffset, IH_OS_ELF);
+
+	/*
+	 * If either of the checks fail, we should report an error, but
+	 * if the image type is coming from the "loadables" field, we
+	 * don't care what it is
+	 */
+	if ((!type_ok || !os_ok) && image_type != IH_TYPE_LOADABLE) {
+		fit_image_get_os(fit, noffset, &os);
+		printf("No %s %s %s Image\n",
+		       genimg_get_os_name(os),
+		       genimg_get_arch_name(arch),
+		       genimg_get_type_name(image_type));
+		bootstage_error(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL);
+		return -EIO;
+	}
+
+	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK);
+
+	return 0;
+}
+
 int fit_image_load(struct bootm_headers *images, ulong addr,
 		   const char **fit_unamep, const char **fit_uname_configp,
-		   int arch, int ph_type, int bootstage_id,
+		   enum image_arch_t arch, int ph_type, int bootstage_id,
 		   enum fit_load_op load_op, ulong *datap, ulong *lenp)
 {
 	int image_type = image_ph_type(ph_type);
@@ -2191,10 +2246,10 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	void *buf;
 	void *loadbuf;
 	size_t size;
-	int type_ok, os_ok;
 	ulong load, load_end, data, len;
-	uint8_t os, comp, os_arch;
+	uint8_t comp, os_arch;
 	const char *prop_name;
+	int ret;
 
 	fit = map_sysmem(addr, 0);
 	prop_name = fit_get_image_type_property(ph_type);
@@ -2221,40 +2276,9 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	fit_image_get_arch(fit, noffset, &os_arch);
 	images_set_arch(images, os_arch);
 
-	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) ||
-		  fit_image_check_type(fit, noffset, IH_TYPE_TEE) ||
-		  fit_image_check_type(fit, noffset, IH_TYPE_TFA_BL31) ||
-		  (image_type == IH_TYPE_KERNEL &&
-		   fit_image_check_type(fit, noffset, IH_TYPE_KERNEL_NOLOAD));
-
-	os_ok = image_type == IH_TYPE_FLATDT ||
-		image_type == IH_TYPE_FPGA ||
-		fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
-		fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
-		fit_image_check_os(fit, noffset, IH_OS_TEE) ||
-		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
-		fit_image_check_os(fit, noffset, IH_OS_EFI) ||
-		fit_image_check_os(fit, noffset, IH_OS_VXWORKS) ||
-		fit_image_check_os(fit, noffset, IH_OS_ELF);
-
-	/*
-	 * If either of the checks fail, we should report an error, but
-	 * if the image type is coming from the "loadables" field, we
-	 * don't care what it is
-	 */
-	if ((!type_ok || !os_ok) && image_type != IH_TYPE_LOADABLE) {
-		fit_image_get_os(fit, noffset, &os);
-		printf("No %s %s %s Image\n",
-		       genimg_get_os_name(os),
-		       genimg_get_arch_name(arch),
-		       genimg_get_type_name(image_type));
-		bootstage_error(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL);
-		return -EIO;
-	}
-
-	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK);
+	ret = check_allowed(fit, noffset, image_type, arch, bootstage_id);
+	if (ret)
+		return ret;
 
 	/* get image data address and length */
 	if (fit_image_get_data(fit, noffset, (const void **)&buf, &size)) {
diff --git a/include/image.h b/include/image.h
index 5e11dc92559..726d82deb22 100644
--- a/include/image.h
+++ b/include/image.h
@@ -111,7 +111,7 @@ enum {
  * New IDs *MUST* be appended at the end of the list and *NEVER*
  * inserted for backward compatibility.
  */
-enum {
+enum image_arch_t {
 	IH_ARCH_INVALID		= 0,	/* Invalid CPU	*/
 	IH_ARCH_ALPHA,			/* Alpha	*/
 	IH_ARCH_ARM,			/* ARM		*/
@@ -760,7 +760,7 @@ int boot_get_fdt_fit(struct bootm_headers *images, ulong addr,
  */
 int fit_image_load(struct bootm_headers *images, ulong addr,
 		   const char **fit_unamep, const char **fit_uname_configp,
-		   int arch, int image_ph_type, int bootstage_id,
+		   enum image_arch_t arch, int image_ph_type, int bootstage_id,
 		   enum fit_load_op load_op, ulong *datap, ulong *lenp);
 
 /**
-- 
2.43.0

[PATCH 05/13] boot: Move handling of the load_op into a separate function

[Simon Glass]

Since fit_image_load() is still too long, move the load-operation
handling into a separate function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 107 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 34 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 3a6120e9151..c5067b63682 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2232,6 +2232,75 @@ static int check_allowed(const void *fit, int noffset,
 	return 0;
 }
 
+/**
+ * handle_load_op() - Handle the load operation
+ *
+ * Process the load_op and figure out where the image should be loaded, now
+ * that it has been located
+ *
+ * @fit: FIT to check
+ * @noffset: Node offset of the image being loaded
+ * @prop_name: Property name (in the configuration node) indicating the image
+ * that was loaded
+ * @buf: Pointer to image (within the FIT)
+ * @size: Size of the image in bytes
+ * @image_type: Type of the image
+ * @load_op: Load operation to process
+ * @bootstage_id: ID of starting bootstage to use for progress updates
+ * @datap: Returns buf
+ * @loadp: Returns the address to which the data should be loaded
+ * @load_endp: Returns the end address of the data-loading location
+ * Return: 0 if OK, -ve on error
+ */
+static int handle_load_op(const void *fit, int noffset, const char *prop_name,
+			  void *buf, ulong size,
+			  enum image_type_t image_type,
+			  enum fit_load_op load_op, int bootstage_id,
+			  ulong *datap, ulong *loadp, ulong *load_endp)
+{
+	ulong data, load;
+
+	data = map_to_sysmem(buf);
+	load = data;
+	*load_endp = 0;
+	if (load_op == FIT_LOAD_IGNORED) {
+		log_debug("load_op: not loading\n");
+		/* Don't load */
+	} else if (fit_image_get_load(fit, noffset, &load)) {
+		if (load_op == FIT_LOAD_REQUIRED) {
+			printf("Can't get %s subimage load address!\n",
+			       prop_name);
+			bootstage_error(bootstage_id + BOOTSTAGE_SUB_LOAD);
+			return -EBADF;
+		}
+	} else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
+		ulong image_start, image_end;
+
+		/*
+		 * move image data to the load address,
+		 * make sure we don't overwrite initial image
+		 */
+		image_start = map_to_sysmem(fit);
+		image_end = image_start + fit_get_size(fit);
+
+		*load_endp = load + size;
+		if (image_type != IH_TYPE_KERNEL &&
+		    load < image_end && *load_endp > image_start) {
+			printf("Error: %s overwritten\n", prop_name);
+			return -EXDEV;
+		}
+
+		printf("   Loading %s from 0x%08lx to 0x%08lx\n",
+		       prop_name, data, load);
+	} else {
+		load = data;	/* No load address specified */
+	}
+	*datap = data;
+	*loadp = load;
+
+	return 0;
+}
+
 int fit_image_load(struct bootm_headers *images, ulong addr,
 		   const char **fit_unamep, const char **fit_uname_configp,
 		   enum image_arch_t arch, int ph_type, int bootstage_id,
@@ -2305,40 +2374,10 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK);
 
-	data = map_to_sysmem(buf);
-	load = data;
-	if (load_op == FIT_LOAD_IGNORED) {
-		log_debug("load_op: not loading\n");
-		/* Don't load */
-	} else if (fit_image_get_load(fit, noffset, &load)) {
-		if (load_op == FIT_LOAD_REQUIRED) {
-			printf("Can't get %s subimage load address!\n",
-			       prop_name);
-			bootstage_error(bootstage_id + BOOTSTAGE_SUB_LOAD);
-			return -EBADF;
-		}
-	} else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) {
-		ulong image_start, image_end;
-
-		/*
-		 * move image data to the load address,
-		 * make sure we don't overwrite initial image
-		 */
-		image_start = addr;
-		image_end = addr + fit_get_size(fit);
-
-		load_end = load + len;
-		if (image_type != IH_TYPE_KERNEL &&
-		    load < image_end && load_end > image_start) {
-			printf("Error: %s overwritten\n", prop_name);
-			return -EXDEV;
-		}
-
-		printf("   Loading %s from 0x%08lx to 0x%08lx\n",
-		       prop_name, data, load);
-	} else {
-		load = data;	/* No load address specified */
-	}
+	ret = handle_load_op(fit, noffset, prop_name, buf, len, image_type,
+			     load_op, bootstage_id, &data, &load, &load_end);
+	if (ret)
+		return ret;
 
 	comp = IH_COMP_NONE;
 	loadbuf = buf;
-- 
2.43.0

[PATCH 06/13] boot: Move obtaining data from a FIT image into a function

[Simon Glass]

Move this code into a separate function, to help further slim down
fit_image_load().

Move the bootstage_mark() call in there too.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 75 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 25 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index c5067b63682..5e90cd2cf50 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2232,6 +2232,53 @@ static int check_allowed(const void *fit, int noffset,
 	return 0;
 }
 
+/**
+ * obtain_data() - Obtain the data from the FIT
+ *
+ * Get the location of the data in the FIT and see if it needs to be deciphered
+ * or processed in some grubby board-specific way.
+ *
+ * @fit: FIT to check
+ * @noffset: Node offset of the image being loaded
+ * @prop_name: Property name (in the configuration node) indicating the image
+ * that was loaded
+ * @bootstage_id: ID of starting bootstage to use for progress updates
+ * @bufp: Returns a pointer to the data
+ * @sizep: Returns the size of the data
+ * Return: 0 if OK, -ve on error
+ */
+static int obtain_data(const void *fit, int noffset, const char *prop_name,
+		       int bootstage_id, void **bufp, ulong *sizep)
+{
+	size_t size;
+
+	/* get image data address and length */
+	if (fit_image_get_data(fit, noffset, (const void **)bufp, &size)) {
+		printf("Could not find %s subimage data!\n", prop_name);
+		bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA);
+		return -ENOENT;
+	}
+
+	/* Decrypt data before uncompress/move */
+	if (IS_ENABLED(CONFIG_FIT_CIPHER) && IMAGE_ENABLE_DECRYPT) {
+		puts("   Decrypting Data ... ");
+		if (fit_image_uncipher(fit, noffset, bufp, &size)) {
+			puts("Error\n");
+			return -EACCES;
+		}
+		puts("OK\n");
+	}
+
+	/* perform any post-processing on the image data */
+	if (!tools_build() && IS_ENABLED(CONFIG_FIT_IMAGE_POST_PROCESS))
+		board_fit_image_post_process(fit, noffset, bufp, &size);
+
+	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK);
+	*sizep = size;
+
+	return 0;
+}
+
 /**
  * handle_load_op() - Handle the load operation
  *
@@ -2314,7 +2361,6 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	const void *fit;
 	void *buf;
 	void *loadbuf;
-	size_t size;
 	ulong load, load_end, data, len;
 	uint8_t comp, os_arch;
 	const char *prop_name;
@@ -2349,30 +2395,9 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	if (ret)
 		return ret;
 
-	/* get image data address and length */
-	if (fit_image_get_data(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;
-	}
-
-	/* Decrypt data before uncompress/move */
-	if (IS_ENABLED(CONFIG_FIT_CIPHER) && IMAGE_ENABLE_DECRYPT) {
-		puts("   Decrypting Data ... ");
-		if (fit_image_uncipher(fit, noffset, &buf, &size)) {
-			puts("Error\n");
-			return -EACCES;
-		}
-		puts("OK\n");
-	}
-
-	/* perform any post-processing on the image data */
-	if (!tools_build() && IS_ENABLED(CONFIG_FIT_IMAGE_POST_PROCESS))
-		board_fit_image_post_process(fit, noffset, &buf, &size);
-
-	len = (ulong)size;
-
-	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK);
+	ret = obtain_data(fit, noffset, prop_name, bootstage_id, &buf, &len);
+	if (ret)
+		return ret;
 
 	ret = handle_load_op(fit, noffset, prop_name, buf, len, image_type,
 			     load_op, bootstage_id, &data, &load, &load_end);
-- 
2.43.0

[PATCH 07/13] boot: Check the image is allowed before setting os.arch

[Simon Glass]

There is no point in setting the architecture if the image cannot be
used. Move the check a little higher within fit_image_load().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 5e90cd2cf50..ba8a56a8ff4 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2388,13 +2388,13 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		}
 	}
 
-	fit_image_get_arch(fit, noffset, &os_arch);
-	images_set_arch(images, os_arch);
-
 	ret = check_allowed(fit, noffset, image_type, arch, bootstage_id);
 	if (ret)
 		return ret;
 
+	fit_image_get_arch(fit, noffset, &os_arch);
+	images_set_arch(images, os_arch);
+
 	ret = obtain_data(fit, noffset, prop_name, bootstage_id, &buf, &len);
 	if (ret)
 		return ret;
-- 
2.43.0

[PATCH 08/13] boot: Move the architecture check into check_allowed()

[Simon Glass]

We may as well have all the checks together, so move the call to
fit_image_check_target_arch() into check_allowed().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index ba8a56a8ff4..0a880863063 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2194,6 +2194,15 @@ static int check_allowed(const void *fit, int noffset,
 	bool type_ok, os_ok;
 	uint8_t os;
 
+	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ARCH);
+	if (!tools_build() && IS_ENABLED(CONFIG_SANDBOX)) {
+		if (!fit_image_check_target_arch(fit, noffset)) {
+			puts("Unsupported Architecture\n");
+			bootstage_error(bootstage_id + BOOTSTAGE_SUB_CHECK_ARCH);
+			return -ENOEXEC;
+		}
+	}
+
 	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) ||
@@ -2379,15 +2388,6 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	if (noffset < 0)
 		return noffset;
 
-	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ARCH);
-	if (!tools_build() && IS_ENABLED(CONFIG_SANDBOX)) {
-		if (!fit_image_check_target_arch(fit, noffset)) {
-			puts("Unsupported Architecture\n");
-			bootstage_error(bootstage_id + BOOTSTAGE_SUB_CHECK_ARCH);
-			return -ENOEXEC;
-		}
-	}
-
 	ret = check_allowed(fit, noffset, image_type, arch, bootstage_id);
 	if (ret)
 		return ret;
-- 
2.43.0

[PATCH 09/13] boot: Move image-decompression into a separate function

[Simon Glass]

Move handling of decompression (or perhaps relocation) of the image into
a separate function, to slim down fit_image_load() a little more.

Move the bootstage_mark() call in there too.

Pass the load address by reference since it may be updated when the
image is relocated for alignment.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 154 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 104 insertions(+), 50 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 0a880863063..a59226d42c5 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2288,6 +2288,104 @@ static int obtain_data(const void *fit, int noffset, const char *prop_name,
 	return 0;
 }
 
+/**
+ * decomp_image() - Decompress / relocate the image
+ *
+ * If the image is compressed, decompress it to the provided load address,
+ * making sure it fits within the allowed space.
+ *
+ * If the image is not compressed, copy it (to the load address) if needed.
+ *
+ * Kernels are not decompressed here, since that is handled in bootm_load_os()
+ *
+ * Compression is ignored on ramdisks, although a warning is provided. Since the
+ * ramdisk normally uses its own compression, doing additional compression at
+ * the FIT level is counter-productive.
+ *
+ * For devicetree, check that the image is actually a devicetree
+ *
+ * @fit: FIT to check
+ * @noffset: Node offset of the image being loaded
+ * @prop_name: Property name (in the configuration node) indicating the image
+ * that was loaded
+ * @buf: Pointer to image (within the FIT)
+ * @size: Size of the image in bytes
+ * @image_type: Type of the image
+ * @load_op: Load operation to process
+ * @bootstage_id: ID of starting bootstage to use for progress updates
+ * @data: Data buffer
+ * @loadp: On entry, the address to which the data should be loaded. Updated
+ *	if the data is relocated (e.g. for alignment)
+ * @sizep: On entry, the size of the image in bytes. Updated if the image is
+ *	decompressed
+ * @load_end: End address of the data-loading location
+ * Return: 0 if OK, -ve on error
+ */
+static int decomp_image(const void *fit, int noffset, const char *prop_name,
+			void *buf, enum image_type_t image_type,
+			enum fit_load_op load_op, int bootstage_id, ulong data,
+			ulong *loadp, ulong *sizep, ulong load_end)
+{
+	ulong load = *loadp;
+	ulong size = *sizep;
+	void *loadbuf;
+	uint8_t comp;
+
+	comp = IH_COMP_NONE;
+	loadbuf = buf;
+	/* Kernel images get decompressed later in bootm_load_os(). */
+	if (!fit_image_get_comp(fit, noffset, &comp) &&
+	    comp != IH_COMP_NONE &&
+	    load_op != FIT_LOAD_IGNORED &&
+	    !(image_type == IH_TYPE_KERNEL ||
+	      image_type == IH_TYPE_KERNEL_NOLOAD ||
+	      image_type == IH_TYPE_RAMDISK)) {
+		ulong max_decomp_len = size * 20;
+
+		log_debug("decompressing image\n");
+		if (load == data) {
+			loadbuf = aligned_alloc(8, max_decomp_len);
+			load = map_to_sysmem(loadbuf);
+		} else {
+			loadbuf = map_sysmem(load, max_decomp_len);
+		}
+		if (image_decomp(comp, load, data, image_type, loadbuf, buf,
+				 size, max_decomp_len, &load_end)) {
+			printf("Error decompressing %s\n", prop_name);
+
+			return -ENOEXEC;
+		}
+		size = load_end - load;
+	} else if (load_op != FIT_LOAD_IGNORED &&
+		   image_type == IH_TYPE_FLATDT &&
+		   ((uintptr_t)buf & 7)) {
+		loadbuf = aligned_alloc(8, size);
+		load = map_to_sysmem(loadbuf);
+		memcpy(loadbuf, buf, size);
+	} else if (load != data) {
+		log_debug("copying\n");
+		loadbuf = map_sysmem(load, size);
+		memcpy(loadbuf, buf, size);
+	}
+	*loadp = load;
+	*sizep = size;
+
+	if (image_type == IH_TYPE_RAMDISK && comp != IH_COMP_NONE)
+		puts("WARNING: 'compression' nodes for ramdisks are deprecated,"
+		     " please fix your .its file!\n");
+
+	/* verify that image data is a proper FDT blob */
+	if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
+	    fdt_check_header(loadbuf)) {
+		puts("Subimage data is not a FDT\n");
+		return -ENOEXEC;
+	}
+
+	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_LOAD);
+
+	return 0;
+}
+
 /**
  * handle_load_op() - Handle the load operation
  *
@@ -2369,9 +2467,8 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	const char *fit_base_uname_config;
 	const void *fit;
 	void *buf;
-	void *loadbuf;
 	ulong load, load_end, data, len;
-	uint8_t comp, os_arch;
+	uint8_t os_arch;
 	const char *prop_name;
 	int ret;
 
@@ -2404,54 +2501,11 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	if (ret)
 		return ret;
 
-	comp = IH_COMP_NONE;
-	loadbuf = buf;
-	/* Kernel images get decompressed later in bootm_load_os(). */
-	if (!fit_image_get_comp(fit, noffset, &comp) &&
-	    comp != IH_COMP_NONE &&
-	    load_op != FIT_LOAD_IGNORED &&
-	    !(image_type == IH_TYPE_KERNEL ||
-	      image_type == IH_TYPE_KERNEL_NOLOAD ||
-	      image_type == IH_TYPE_RAMDISK)) {
-		ulong max_decomp_len = len * 20;
-
-		log_debug("decompressing image\n");
-		if (load == data) {
-			loadbuf = aligned_alloc(8, max_decomp_len);
-			load = map_to_sysmem(loadbuf);
-		} else {
-			loadbuf = map_sysmem(load, max_decomp_len);
-		}
-		if (image_decomp(comp, load, data, image_type,
-				loadbuf, buf, len, max_decomp_len, &load_end)) {
-			printf("Error decompressing %s\n", prop_name);
-
-			return -ENOEXEC;
-		}
-		len = load_end - load;
-	} else if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
-		   ((uintptr_t)buf & 7)) {
-		loadbuf = aligned_alloc(8, len);
-		load = map_to_sysmem(loadbuf);
-		memcpy(loadbuf, buf, len);
-	} else if (load != data) {
-		log_debug("copying\n");
-		loadbuf = map_sysmem(load, len);
-		memcpy(loadbuf, buf, len);
-	}
-
-	if (image_type == IH_TYPE_RAMDISK && comp != IH_COMP_NONE)
-		puts("WARNING: 'compression' nodes for ramdisks are deprecated,"
-		     " please fix your .its file!\n");
-
-	/* verify that image data is a proper FDT blob */
-	if (load_op != FIT_LOAD_IGNORED && image_type == IH_TYPE_FLATDT &&
-	    fdt_check_header(loadbuf)) {
-		puts("Subimage data is not a FDT\n");
-		return -ENOEXEC;
-	}
-
-	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_LOAD);
+	ret = decomp_image(fit, noffset, prop_name, buf, image_type,
+			   load_op, bootstage_id, data, &load, &len,
+			   load_end);
+	if (ret)
+		return ret;
 
 	upl_add_image(fit, noffset, load, len);
 
-- 
2.43.0

[PATCH 10/13] boot: Move decomp_image() into handle_load_op()

[Simon Glass]

Decompress is really part of loading, so simplify the code slightly by
moving decompression out of the top-level fit_image_load() function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index a59226d42c5..7efd0f30dc5 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2390,7 +2390,8 @@ static int decomp_image(const void *fit, int noffset, const char *prop_name,
  * handle_load_op() - Handle the load operation
  *
  * Process the load_op and figure out where the image should be loaded, now
- * that it has been located
+ * that it has been located. Decompress and move as necessary to get the image
+ * into the right place - see decomp_image()
  *
  * @fit: FIT to check
  * @noffset: Node offset of the image being loaded
@@ -2402,21 +2403,21 @@ static int decomp_image(const void *fit, int noffset, const char *prop_name,
  * @load_op: Load operation to process
  * @bootstage_id: ID of starting bootstage to use for progress updates
  * @datap: Returns buf
- * @loadp: Returns the address to which the data should be loaded
- * @load_endp: Returns the end address of the data-loading location
+ * @loadp: Returns the address where the data was loaded
+ * @sizep: On entry, the size of the image in bytes. Updated if decompressed
  * Return: 0 if OK, -ve on error
  */
 static int handle_load_op(const void *fit, int noffset, const char *prop_name,
-			  void *buf, ulong size,
-			  enum image_type_t image_type,
+			  void *buf, ulong size, enum image_type_t image_type,
 			  enum fit_load_op load_op, int bootstage_id,
-			  ulong *datap, ulong *loadp, ulong *load_endp)
+			  ulong *datap, ulong *loadp, ulong *sizep)
 {
-	ulong data, load;
+	ulong data, load, load_end;
+	int ret;
 
 	data = map_to_sysmem(buf);
 	load = data;
-	*load_endp = 0;
+	load_end = 0;
 	if (load_op == FIT_LOAD_IGNORED) {
 		log_debug("load_op: not loading\n");
 		/* Don't load */
@@ -2437,9 +2438,9 @@ static int handle_load_op(const void *fit, int noffset, const char *prop_name,
 		image_start = map_to_sysmem(fit);
 		image_end = image_start + fit_get_size(fit);
 
-		*load_endp = load + size;
+		load_end = load + size;
 		if (image_type != IH_TYPE_KERNEL &&
-		    load < image_end && *load_endp > image_start) {
+		    load < image_end && load_end > image_start) {
 			printf("Error: %s overwritten\n", prop_name);
 			return -EXDEV;
 		}
@@ -2449,6 +2450,13 @@ static int handle_load_op(const void *fit, int noffset, const char *prop_name,
 	} else {
 		load = data;	/* No load address specified */
 	}
+
+	ret = decomp_image(fit, noffset, prop_name, buf, image_type,
+			   load_op, bootstage_id, data, &load, sizep,
+			   load_end);
+	if (ret)
+		return ret;
+
 	*datap = data;
 	*loadp = load;
 
@@ -2467,7 +2475,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	const char *fit_base_uname_config;
 	const void *fit;
 	void *buf;
-	ulong load, load_end, data, len;
+	ulong load, data, len;
 	uint8_t os_arch;
 	const char *prop_name;
 	int ret;
@@ -2497,13 +2505,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		return ret;
 
 	ret = handle_load_op(fit, noffset, prop_name, buf, len, image_type,
-			     load_op, bootstage_id, &data, &load, &load_end);
-	if (ret)
-		return ret;
-
-	ret = decomp_image(fit, noffset, prop_name, buf, image_type,
-			   load_op, bootstage_id, data, &load, &len,
-			   load_end);
+			     load_op, bootstage_id, &data, &load, &len);
 	if (ret)
 		return ret;
 
-- 
2.43.0

[PATCH 11/13] boot: Move setting the OS arch into check_allowed()

[Simon Glass]

Remove a few more lines from fit_image_load() by dealing with this small
detail in check_allowed().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 7efd0f30dc5..749cbe19ade 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2182,17 +2182,19 @@ static int select_image(const void *fit, struct bootm_headers *images,
  *
  * @fit: FIT to check
  * @noffset: Node offset of the image being loaded
+ * @images: Boot images structure
  * @image_type: Type of the image
  * @arch: Expected architecture for the image
  * @bootstage_id: ID of starting bootstage to use for progress updates
  * Return: 0 if OK, -EIO if not
  */
 static int check_allowed(const void *fit, int noffset,
+			 struct bootm_headers *images,
 			 enum image_type_t image_type, enum image_arch_t arch,
 			 int bootstage_id)
 {
 	bool type_ok, os_ok;
-	uint8_t os;
+	uint8_t os, os_arch;
 
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ARCH);
 	if (!tools_build() && IS_ENABLED(CONFIG_SANDBOX)) {
@@ -2238,6 +2240,9 @@ static int check_allowed(const void *fit, int noffset,
 
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK);
 
+	fit_image_get_arch(fit, noffset, &os_arch);
+	images_set_arch(images, os_arch);
+
 	return 0;
 }
 
@@ -2476,7 +2481,6 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	const void *fit;
 	void *buf;
 	ulong load, data, len;
-	uint8_t os_arch;
 	const char *prop_name;
 	int ret;
 
@@ -2493,13 +2497,11 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	if (noffset < 0)
 		return noffset;
 
-	ret = check_allowed(fit, noffset, image_type, arch, bootstage_id);
+	ret = check_allowed(fit, noffset, images, image_type, arch,
+			    bootstage_id);
 	if (ret)
 		return ret;
 
-	fit_image_get_arch(fit, noffset, &os_arch);
-	images_set_arch(images, os_arch);
-
 	ret = obtain_data(fit, noffset, prop_name, bootstage_id, &buf, &len);
 	if (ret)
 		return ret;
-- 
2.43.0

[PATCH 12/13] boot: Drop unnecessary data variable

[Simon Glass]

This is always the same as 'buf' and is not needed by the caller. Drop
the return of this value from handle_load_op().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 749cbe19ade..34876b66fa0 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2407,7 +2407,6 @@ static int decomp_image(const void *fit, int noffset, const char *prop_name,
  * @image_type: Type of the image
  * @load_op: Load operation to process
  * @bootstage_id: ID of starting bootstage to use for progress updates
- * @datap: Returns buf
  * @loadp: Returns the address where the data was loaded
  * @sizep: On entry, the size of the image in bytes. Updated if decompressed
  * Return: 0 if OK, -ve on error
@@ -2415,7 +2414,7 @@ static int decomp_image(const void *fit, int noffset, const char *prop_name,
 static int handle_load_op(const void *fit, int noffset, const char *prop_name,
 			  void *buf, ulong size, enum image_type_t image_type,
 			  enum fit_load_op load_op, int bootstage_id,
-			  ulong *datap, ulong *loadp, ulong *sizep)
+			  ulong *loadp, ulong *sizep)
 {
 	ulong data, load, load_end;
 	int ret;
@@ -2462,7 +2461,6 @@ static int handle_load_op(const void *fit, int noffset, const char *prop_name,
 	if (ret)
 		return ret;
 
-	*datap = data;
 	*loadp = load;
 
 	return 0;
@@ -2480,7 +2478,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	const char *fit_base_uname_config;
 	const void *fit;
 	void *buf;
-	ulong load, data, len;
+	ulong load, len;
 	const char *prop_name;
 	int ret;
 
@@ -2507,7 +2505,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		return ret;
 
 	ret = handle_load_op(fit, noffset, prop_name, buf, len, image_type,
-			     load_op, bootstage_id, &data, &load, &len);
+			     load_op, bootstage_id, &load, &len);
 	if (ret)
 		return ret;
 
-- 
2.43.0

[PATCH 13/13] boot: Tidy local variables in fit_image_load()

[Simon Glass]

Arrange these so that they look a little nicer.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 boot/image-fit.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 34876b66fa0..fe53b3a278b 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -2472,15 +2472,14 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		   enum fit_load_op load_op, ulong *datap, ulong *lenp)
 {
 	int image_type = image_ph_type(ph_type);
-	int noffset;
-	const char *fit_uname;
-	const char *fit_uname_config;
 	const char *fit_base_uname_config;
+	const char *fit_uname_config;
+	const char *fit_uname;
+	const char *prop_name;
+	int noffset, ret;
 	const void *fit;
-	void *buf;
 	ulong load, len;
-	const char *prop_name;
-	int ret;
+	void *buf;
 
 	fit = map_sysmem(addr, 0);
 	prop_name = fit_get_image_type_property(ph_type);
-- 
2.43.0

Re: [PATCH 01/13] boot: Split out the first part of fit_image_load()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Wed, Mar 25, 2026 at 10:54:10AM -0600, Simon Glass wrote:

> This function is over 250 lines long. Split out the image-selection
> part into a new select_image() function which handles format checking,
> configuration selection and image-node lookup.
> 
> Take care to only call fit_get_name() when a valid node is found, since
> libfdt does not handle negative offsets gracefully.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Function length isn't a problem in and of itself, especially when we
aren't pulling out some 4 or 5 level deep nested logic to improve
readability.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 02/13] boot: Move call to fit_image_select() and rename it

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

On Wed, Mar 25, 2026 at 10:54:11AM -0600, Simon Glass wrote:

> This function is named a bit vaguely, since it prints some info and then
> does verification of the image.
> 
> Rename it to print_and_verify() and move the call to select_image().
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Renaming functions inside a bigger overall series makes review harder,
this should be independent, either before or after.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 03/13] boot: Tidy up setting of the OS arch on host builds

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

On Wed, Mar 25, 2026 at 10:54:12AM -0600, Simon Glass wrote:

> This field is not present in host builds. Create an inline function to
> handle this complexity, so we can drop the #ifdef in the C file.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

I'm not sure hiding details of what the host tools do vs our regular
run time do is improving overall readability / understandability.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 04/13] boot: Move type and OS checking into a new function

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Wed, Mar 25, 2026 at 10:54:13AM -0600, Simon Glass wrote:

> Move the code which checks whether the image can be loaded into a
> separate check_allowed() function.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Switching to an enum is useful for various build time reasons (and
wasn't called out), refactoring code that's not heavily nested is the
same issue as in patch #1 of the series.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 05/13] boot: Move handling of the load_op into a separate function

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]

On Wed, Mar 25, 2026 at 10:54:14AM -0600, Simon Glass wrote:

> Since fit_image_load() is still too long, move the load-operation
> handling into a separate function.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Same problem as patch #1.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 00/13] boot: Make fit_image_load() easier to maintain

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2067 bytes --]

On Wed, Mar 25, 2026 at 10:54:09AM -0600, Simon Glass wrote:

> Before fit_image_load() was created, the code to load kernels, ramdisks
> and devicetrees from a FIT was spread around many functions. By
> combining most of the code in one place, it became possible to add more
> features in a consistent way. The 'loadables' feature was much easier to
> plumb in, for example.
> 
> While fit_image_load() was a substantial advance, it has never been a
> svelte function and the passing years have not been entirely kind. With
> a few new features on the horizon, this is a good time to improve the
> implementation.
> 
> This series splits much of the code from fit_image_load() into a number
> of smaller functions. Most of the changes are fairly mechanical, with
> just a few renames and tweaks here and there.
> 
> This should make the function much easier to maintain. It may also
> encourage someone to take a look at its callers, which could also use
> some attention.
> 
> Code-size impact: about +8 bytes on aarch64 and +24-40 bytes on arm, due
> to the compiler's register allocation across the new helper functions.
> 
> 
> Simon Glass (13):
>   boot: Split out the first part of fit_image_load()
>   boot: Move call to fit_image_select() and rename it
>   boot: Tidy up setting of the OS arch on host builds
>   boot: Move type and OS checking into a new function
>   boot: Move handling of the load_op into a separate function
>   boot: Move obtaining data from a FIT image into a function
>   boot: Check the image is allowed before setting os.arch
>   boot: Move the architecture check into check_allowed()
>   boot: Move image-decompression into a separate function
>   boot: Move decomp_image() into handle_load_op()
>   boot: Move setting the OS arch into check_allowed()
>   boot: Drop unnecessary data variable
>   boot: Tidy local variables in fit_image_load()

I've reviewed through 5, and I assume 6-13 have similar issues as the
rest. Please think about that before submitting a v2, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]