Hi Daniel,
On 2026-08-23T19:12:43, Daniel Golle <[email protected]> wrote:
> boot: fit: support on-demand loading in fit_image_load()
>
> Integrate storage-backed image loading transparently into the existing
> FIT flow so that all verification, decompression, and copy-to-load-
> address code runs unchanged for both RAM-backed and storage-backed
> images.
>
> Hook imagemap into fit_image_get_data(): when gd->imagemap is set and
> the sub-image uses external data, call imagemap_map() to bring the
> payload into RAM instead of returning the unmapped fit + offset
> pointer. Every existing caller, including fit_image_verify() called
> from fit_image_select(), transparently gets valid data without any
> changes to the authentication path.
>
> For uncompressed sub-images with a known load address, pre-populate
> the translation table via imagemap_map_to() before verification runs.
> This loads data directly to the final RAM destination; when
> fit_image_get_data() is later called, the translation table hit
> returns the same pointer, achieving zero-copy. Compressed images or
> those without a load address fall through to the lazy imagemap_map()
> [...]
>
> boot/bootm.c | 70 +++++++++++++++++++++++++++++++----
> boot/image-fit.c | 109
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> include/bootm.h | 2 +
> include/image.h | 2 +
> 4 files changed, 174 insertions(+), 9 deletions(-)
> diff --git a/boot/bootm.c b/boot/bootm.c
> @@ -147,7 +148,22 @@ static int boot_get_kernel(const char *addr_fit, struct
> bootm_headers *images,
> + buf = imagemap_map(images->imagemap, 0, 64);
Where does 64 come from? A short comment or named constant would help
- genimg_get_format() has to distinguish a legacy header, a FIT FDT
magic and possibly an Android header, each with different minimum
sizes, so an unexplained 64 looks like a coincidence.
> diff --git a/boot/bootm.c b/boot/bootm.c
> @@ -1033,11 +1063,21 @@ int bootm_run_states(struct bootm_info *bmi, int
> states)
> + if (states & BOOTM_STATE_START) {
> ret = bootm_start();
> + /*
> + * bootm_start() zeroes the global images struct. Restore
> + * the loader pointer so the storage-backed path works.
> + */
> + if (IS_ENABLED(CONFIG_IMAGEMAP) && bmi->imagemap)
> + images->imagemap = bmi->imagemap;
> + }
Restoring a field immediately after bootm_start() zeroes it is fragile
- any future state that also calls bootm_start() (BOOTM_STATE_RESTART
already runs bootm_restart(), similar in spirit) will silently drop
the pointer again. Please move this into bootm_start() itself, or
teach bootm_start() to preserve imagemap across the reset, so the
invariant is expressed in one place.
> diff --git a/boot/bootm.c b/boot/bootm.c
> @@ -1145,11 +1188,24 @@ int bootm_run_states(struct bootm_info *bmi, int
> states)
> +err:
> + /* Clean up imagemap on error (not reached on successful boot) */
> + if (IS_ENABLED(CONFIG_IMAGEMAP) && images->imagemap) {
> + imagemap_cleanup(images->imagemap);
> + images->imagemap = NULL;
> + }
Several paths in bootm_run_states() return directly rather than goto
err - 'if (images->no_os) return -ENOPKG;', 'if (ret) return ret;'
after the OS-func lookup, and the 'ERROR: booting os …' return 1 all
bypass this cleanup, so imagemap leaks on those exits. Please convert
those to goto err, or drive the cleanup from a single place that runs
on every exit.
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -1140,6 +1141,16 @@ int fit_image_get_data(const void *fit, int noffset,
> const void **data,
> +#if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
> + if (images.imagemap) {
> + void *mapped;
> +
> + mapped = imagemap_lookup(images.imagemap,
> + offset, len);
> + if (mapped)
> + *data = mapped;
> + }
> +#endif
Reaching into the global images from a low-level helper couples
fit_image_get_data() to bootm state - until now this function has been
usable purely from (fit, noffset). Please pass the imagemap in
explicitly (as a parameter or via a small wrapper), which also keeps
'ut fit' style tests free of hidden global dependencies.
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2140,6 +2151,84 @@ static const char *fit_get_image_type_property(int
> ph_type)
> + if (CONFIG_IS_ENABLED(FIT_VERITY)) {
> + u8 img_type;
> +
> + if (!fit_image_get_type(fit, noffset, &img_type) &&
> + img_type == IH_TYPE_FILESYSTEM &&
> + fdt_subnode_offset(fit, noffset, "dm-verity") >= 0) {
Please use FIT_VERITY_NODENAME from image.h rather than a literal
dm-verity, so there is one place to change the name.
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2140,6 +2151,84 @@ static const char *fit_get_image_type_property(int
> ph_type)
> + if (!fit_image_get_data_position(fit, noffset, &data_off)) {
> + external = true;
> + } else if (!fit_image_get_data_offset(fit, noffset, &data_off)) {
> + external = true;
> + data_off += ALIGN(fdt_totalsize(fit), 4);
> + }
> +
> + if (!external || fit_image_get_data_size(fit, noffset, &data_sz))
> + return 0;
This re-implements the data-position / data-offset / totalsize-align
dance that fit_image_get_data() already does a few hundred lines up,
and uses a slightly different expression (ALIGN(fdt_totalsize(fit), 4)
vs ((fdt_totalsize(fit) + 3) & ~3)). Please extract a shared helper -
otherwise a future fix in one path (e.g. the INT_MAX bounds check that
fit_image_get_data() has) will silently miss this one.
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2140,6 +2151,84 @@ static const char *fit_get_image_type_property(int
> ph_type)
> + if (img_comp == IH_COMP_NONE && load_op != FIT_LOAD_IGNORED &&
> + !fit_image_get_load(fit, noffset, &img_load)) {
> + void *dst = map_sysmem(img_load, data_sz);
> +
> + mapped = imagemap_map_to(images->imagemap, data_off, data_sz,
> + dst);
map_sysmem() should really be paired with unmap_sysmem() - a no-op on
most arches but not on sandbox, which is where the tests run.
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2349,8 +2453,9 @@ int fit_image_load(struct bootm_headers *images, ulong
> addr,
> - printf(" Loading %s from 0x%08lx to 0x%08lx\n",
> - prop_name, data, load);
> + if (!CONFIG_IS_ENABLED(IMAGEMAP) || data != load)
> + printf(" Loading %s from 0x%08lx to 0x%08lx\n",
> + prop_name, data, load);
This is a build-time gate, not a runtime one - any board with
CONFIG_IMAGEMAP=y silently loses the 'Loading … from … to …' message
for RAM-backed FITs whenever data happens to equal load, a
user-visible regression for boards that don't use imagemap at boot.
Please key the suppression on 'images->imagemap && data == load' so
the message only disappears on the genuine zero-copy path.
> diff --git a/include/image.h b/include/image.h
> @@ -412,6 +412,8 @@ struct bootm_headers {
> + struct udevice *imagemap; /* on-demand storage loader, or NULL */
It might be worth making this (and the equivalent field in struct
bootm_info) conditional on CONFIG_IMAGEMAP, in line with the
surrounding fields - otherwise every board pays the pointer with the
feature off.
Regards,
Simon
