[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

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 01/13] boot: Split out the first part of fit_image_load()

[Simon Glass]

Hi Tom,

On Wed, 8 Apr 2026 at 10:25, Tom Rini <trini@konsulko.com> wrote:
>
> 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.

In my view the current code is very hard to maintain and change. I
know this because I have tried to do it:

1. By my count, fit_image_load() declares 20 locals. Even without deep
nesting, tracking which variables are live at which point in a
function of ~270 lines is extremely hard (see [1] for someone else
that had this problem). Just to take one example, pulling
select_image() out makes it explicit that cfg_noffset (for example) is
only relevant during image selection and doesn't leak into the
load/decompress path.

2. The function has distinct phases: format checking, configuration
selection, hash verification, and image-node lookup. So we have 'find
what to load' vs. 'load it'. These  are much easier to understand with
clean function boundaries, rather than an arbitrary split at a line
count.

3. Reviewability - when someone adds a feature to the loading path,
they currently have to read past 90 lines of selection logic to find
where their change fits. Smaller functions with clear contracts make
it easier to see where new code belongs.

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/patch/20260324-boot-fit-fix-8-byte-alignement-for-overlays-v1-1-257b132b6bda@baylibre.com/

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

[Tom Rini]

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

On Sat, Apr 11, 2026 at 07:03:35AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 8 Apr 2026 at 10:25, Tom Rini <trini@konsulko.com> wrote:
> >
> > 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.
> 
> In my view the current code is very hard to maintain and change. I
> know this because I have tried to do it:
> 
> 1. By my count, fit_image_load() declares 20 locals. Even without deep
> nesting, tracking which variables are live at which point in a
> function of ~270 lines is extremely hard (see [1] for someone else
> that had this problem). Just to take one example, pulling
> select_image() out makes it explicit that cfg_noffset (for example) is
> only relevant during image selection and doesn't leak into the
> load/decompress path.
> 
> 2. The function has distinct phases: format checking, configuration
> selection, hash verification, and image-node lookup. So we have 'find
> what to load' vs. 'load it'. These  are much easier to understand with
> clean function boundaries, rather than an arbitrary split at a line
> count.
> 
> 3. Reviewability - when someone adds a feature to the loading path,
> they currently have to read past 90 lines of selection logic to find
> where their change fits. Smaller functions with clear contracts make
> it easier to see where new code belongs.

Or, splitting things up makes it harder to review because now what
you're reading through is over *there* instead of the next line. I find
myself having to "un-inline" function calls sometimes because we've
thrown around so many abstractions. So I stand by what I said in the
cover letter.

-- 
Tom

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

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

[Simon Glass]

Hi Tom,

On Mon, 13 Apr 2026 at 14:18, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Apr 11, 2026 at 07:03:35AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 8 Apr 2026 at 10:25, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > 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.
> >
> > In my view the current code is very hard to maintain and change. I
> > know this because I have tried to do it:
> >
> > 1. By my count, fit_image_load() declares 20 locals. Even without deep
> > nesting, tracking which variables are live at which point in a
> > function of ~270 lines is extremely hard (see [1] for someone else
> > that had this problem). Just to take one example, pulling
> > select_image() out makes it explicit that cfg_noffset (for example) is
> > only relevant during image selection and doesn't leak into the
> > load/decompress path.
> >
> > 2. The function has distinct phases: format checking, configuration
> > selection, hash verification, and image-node lookup. So we have 'find
> > what to load' vs. 'load it'. These  are much easier to understand with
> > clean function boundaries, rather than an arbitrary split at a line
> > count.
> >
> > 3. Reviewability - when someone adds a feature to the loading path,
> > they currently have to read past 90 lines of selection logic to find
> > where their change fits. Smaller functions with clear contracts make
> > it easier to see where new code belongs.
>
> Or, splitting things up makes it harder to review because now what
> you're reading through is over *there* instead of the next line. I find
> myself having to "un-inline" function calls sometimes because we've
> thrown around so many abstractions. So I stand by what I said in the
> cover letter.

So...NAK for this series?

My motivation here is partly to implement new features in FIT, which
is quite painful with the current state of fit_image_load()

Regards,
Simon

[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

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 --]

[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

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 --]

[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

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 --]

[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

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 --]

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