[PATCH 07/11] boot: fit: unmap the load address after a storage read

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel at makrotopia.org> wrote:
> boot: fit: unmap the load address after a storage read
>
> fit_image_load_storage() maps the load address to get a destination for
> imagemap_map_to() and never unmaps it. The mapping is a no-op on most
> architectures but not on sandbox, which is where the unit tests run.
>
> Pair the map_sysmem() with an unmap_sysmem() once the read is done.
>
> Fixes: 46d32e38ee4e ("boot: fit: support on-demand loading in fit_image_load()")
> Signed-off-by: Daniel Golle <daniel at makrotopia.org>
>
> boot/image-fit.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

> The mapping is a no-op on most architectures but not on sandbox, which is where the unit tests run.

Just FYI, on sandbox, unmap_physmem() returns early for anything
inside emulated RAM (is_in_sandbox_mem()), and a FIT load address is
always in emulated RAM, so in practice it is a no-op there too. I'm
fine with adding the unmap for consistency (this is definitely an
arcane part of sandbox!), but please can you reword this so it doesn't
suggest the tests were leaking something? The Fixes: tag also feels a
bit strong, but OK if you want it.

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2222,10 +2223,11 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
>               mapped = imagemap_map_to(images->imagemap, data_off, data_sz,
>                                        dst);
> +             unmap_sysmem(dst);

imagemap_map_to() saves dst in the region list through
imagemap_record(), and fit_image_get_data() later hands that same
pointer back through imagemap_lookup(). So this unmaps a pointer the
imagemap still holds and keeps giving to callers, which only works
because unmap_sysmem() does nothing here. If the unmap is meant to be
correct, it should happen when the record is released (e.g. in
imagemap_cleanup() for regions where lmb_reserved is false), or the
region should store a physical address and map it on lookup. Otherwise
I'd suggest leaving the code alone and adding a comment that the
mapping lives as long as the imagemap record. What do you think?

> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -2190,6 +2190,7 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
>       void *mapped;
> +     void *dst;

There's no need to move this to function scope; it is only used inside
the if() block. Sorry if I got that wrong in the previous review.

Regards,
Simon