[PATCH 06/11] boot: imagemap: read only the bytes an extend adds

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel at makrotopia.org> wrote:
> boot: imagemap: read only the bytes an extend adds
>
> Growing a region re-reads it from the start even when the reservation
> was renewed at the same address and the leading bytes are still valid.
> The translation table exists to avoid re-reading data that is already in
> RAM, so the common case of probing a FIT header and then mapping the
> whole structure pays for the header twice.
>
> Read only the range the larger request adds when the region has not
> moved. A reservation that had to be placed elsewhere still needs the
> whole range.
>
> Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")

This is an optimisation rather than a bug fix: the current code reads
more than it needs to, but the result is correct. Please can you drop
the Fixes: tag so this is not picked up as a stable fix?

> Read only the range the larger request adds when the region has not
> moved. A reservation that had to be placed elsewhere still needs the
> whole range.

Strictly, it doesn't need the whole range from storage. The old
reservation has been freed but nothing has written over it, so the old
bytes are still in RAM. A memmove() from the old address to the new
one would keep them, and copes with the new block overlapping the old
one, which is quite likely here. See also below.

> Signed-off-by: Daniel Golle <daniel at makrotopia.org>
>
> boot/imagemap.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -261,8 +265,14 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
> -                     ret = spl_load_region(&priv->info, img_offset, size,
> -                                           base);
> +                     if (addr == old_addr)
> +                             ret = spl_load_region(&priv->info,
> +                                                   base_off + old_size,
> +                                                   read_size - old_size,
> +                                                   (char *)base + old_size);
> +                     else
> +                             ret = spl_load_region(&priv->info, img_offset,
> +                                                   size, base);

Just to check: how often is the addr == old_addr case taken in
practice? LMB_MEM_ALLOC_ANY allocates top-down, so the FIT header
region is normally the last allocation, sitting directly below the
previous one or below U-Boot's own reservation. Growing it upwards at
the same address runs into that memory and fails, so we end up in the
else branch. If so, the 'common case' in the commit message mostly
does not benefit. Copying the already-loaded bytes across in the else
branch would make the saving apply in both cases.

The comment above the loop still says "re-reading the full range from
storage", so please update it too.

For the test, imagemap_test_map_extend() only checks read_count, which
is 2 whether the code reads the whole range or just the extra bytes.
The mock already records last_off and last_size. Please can you assert
that the second read is (64, 192), and add a variant using
create_mock_loader_bl() with a block size larger than 1? That would
show the new path is really used and the offsets are right for block
media.

Regards,
Simon