Hi Daniel,
On 2026-08-23T19:12:43, Daniel Golle <[email protected]> wrote:
> boot: add imagemap on-demand loading from storage
>
> Introduce imagemap, a small layer that reads image data from a storage
> device on demand and keeps a translation table of the byte ranges
> already loaded into RAM, instead of copying the whole image up front.
>
> An imagemap device (UCLASS_IMAGEMAP) is created over a partition of a
> block device with imagemap_create(); imagemap_map() then returns a RAM
> pointer for any byte range, reading it in on first access and reusing
> it on later accesses -- so a header probe, verification and the final
> load of the same range share a single read. Block devices cover more
> than raw disks: an MTD partition is reached through mtdblock and a UBI
> volume through ubiblock, each exposed as a named block-device
> partition, so imagemap needs no storage-specific code of its own.
>
> The read path is built on the SPL struct spl_load_info abstraction and
> a new spl_load_region() helper (added to spl.h): the block reader
> reports the device block length and spl_load_region() performs the
> native sector alignment. imagemap_map_to() keeps a payload byte-exact
> at a caller's load address, reading the aligned middle straight to the
> [...]
>
> MAINTAINERS | 7 +
> boot/Kconfig | 16 ++
> boot/Makefile | 2 +
> boot/imagemap.c | 451
> +++++++++++++++++++++++++++++++++++++++++++++++++
> include/dm/uclass-id.h | 1 +
> include/imagemap.h | 136 +++++++++++++++
> include/spl.h | 47 +++++-
> 7 files changed, 657 insertions(+), 3 deletions(-)
> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -0,0 +1,451 @@
> +void *imagemap_map_to(struct udevice *dev, loff_t img_offset,
> + ulong size, void *dst)
> +{
> + struct imagemap_priv *priv = dev_get_uclass_priv(dev);
> + void *p;
> + int ret;
> +
> + /* If already mapped to this exact destination, return it */
> + p = imagemap_lookup(dev, img_offset, size);
> + if (p && p == dst)
> + return p;
> +
> + ret = imagemap_read_exact(&priv->info, img_offset, size, dst);
> + if (ret) {
> + log_err("imagemap: read failed at offset 0x%llx (size 0x%lx):
> %d\n",
> + (unsigned long long)img_offset, size, ret);
> + return ERR_PTR(ret);
> + }
> +
> + if (!imagemap_record(dev, img_offset, size, dst, false))
> + return ERR_PTR(-ENOMEM);
> +
> + return dst;
> +}
If an earlier imagemap_map() has already recorded a region at the same
img_offset with lmb_reserved=true, this calls imagemap_record() with
lmb_reserved=false, overwriting the entry in place - so it looks like
the LMB reservation is silently orphaned and never freed by
imagemap_cleanup(). Please free the old reservation before overwriting
or refuse the overwrite.
> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -0,0 +1,451 @@
> + if (bl_len == 1)
> + return spl_load_region(info, off, size, dst) < 0 ? -EIO : 0;
> +
> + /* Partial head block: bounce and copy the wanted tail of the block */
> + if (overhead) {
> + ALLOC_CACHE_ALIGN_BUFFER(u8, blk, bl_len);
> + ulong chunk = min(left, bl_len - overhead);
> +
> + if (info->read(info, cur - overhead, bl_len, blk) < bl_len)
> + return -EIO;
ALLOC_CACHE_ALIGN_BUFFER() with a runtime bl_len expands to a stack
VLA. For MMC that's 512 bytes, but a UBI/ubiblock leb_size or MTD
erase-block-sized reader can push this into the KB range. Probably
this is fine, but do you know what the upper limit will be?
> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -0,0 +1,451 @@
> + alist_for_each(r, &priv->regions) {
> + if (r->img_offset == base_off && r->size < read_size) {
> + addr = map_to_sysmem(r->ram);
> +
> + /* Free old LMB reservation if we own it */
> + if (r->lmb_reserved)
> + lmb_free(addr,
> + ALIGN(r->size, ARCH_DMA_MINALIGN),
> + LMB_NONE);
> +
> + /* Try to re-reserve at the same address with new size
> */
> + if (lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &addr,
> + alloc_size, LMB_NONE)) {
Two concerns with this extend path:
1. If r->lmb_reserved is false (the region was recorded by
imagemap_map_to() at a caller-supplied dst), this happily
lmb_alloc_mem()s over that memory and then re-reads on top of it.
Whatever the caller placed there is silently clobbered. Please skip
the extend path in that case, or refuse to extend a non-LMB region.
2. Even in the LMB-owned case, when the in-place re-reservation
succeeds the whole range gets re-read although the first r->size bytes
are already valid - the whole point of the translation table is to
avoid redundant reads. Only the newly-covered tail needs reading.
> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -0,0 +1,451 @@
> + ret = device_bind_driver(desc->bdev, "imagemap",
> + name && *name ? name : "imagemap", &imdev);
Just to check - is the intent that only one imagemap device is ever
active per block device? Two calls with name == NULL (or two different
partitions selected by index) will bind two children both called
imagemap under the same parent, which works but makes the DM tree
confusing. How about deriving a unique name (e.g. including the
partition number) or documenting the single-instance assumption in
imagemap_create()'s kernel-doc ?
> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -0,0 +1,451 @@
> +void *imagemap_lookup(struct udevice *dev, loff_t img_offset, ulong size)
> +{
> + struct imagemap_priv *priv = dev_get_uclass_priv(dev);
> + const struct imagemap_region *r;
The long block comment is useful but says the second guard prevents
underflow on LP64 'where ulong and loff_t have the same rank'. loff_t
is signed long long, 64-bit everywhere U-Boot runs; on ILP32 it
doesn't share rank with ulong. Please tighten the wording - the guard
is needed unconditionally because size is unsigned. Also you could
wrap the comment to 80cols so more of it fits on the screen.
Regards,
Simon
