[PATCH v8 8/8] blkmap: pass information on ISO image to the OS
Sughosh Ganu
sughosh.ganu at linaro.org
Thu Mar 13 10:06:41 CET 2025
On Thu, 13 Mar 2025 at 14:08, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 13.03.25 08:46, Sughosh Ganu wrote:
> > On Thu, 13 Mar 2025 at 02:38, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >> On 12.03.25 09:54, Sughosh Ganu wrote:
> >>> The EFI HTTP boot puts the ISO installer image at some location in
> >>> memory. Information about this image has to be passed on to the OS
> >>> kernel, which is done by adding a persistent memory(pmem) node to the
> >>> devicetree(DT) that is passed to the OS. The OS kernel then gets
> >>> information about the presence of this ISO image and proceeds with the
> >>> installation.
> >>>
> >>> In U-Boot, this ISO image gets mounted as a memory mapped blkmap
> >>> device slice, with the 'preserve' attribute. Add a helper function
> >>> which iterates through all such slices, and invokes a callback. The
> >>> callback adds the pmem node to the DT and removes the corresponding
> >>> memory region from the EFI memory map. Invoke this helper function as
> >>> part of the DT fixup which happens before booting the OS.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> >>> Reviewed-by: Tobias Waldekranz <tobias at waldekranz.com>
> >>> ---
> >>> Changes since V7:
> >>> * Fix checkpatch errors in blkmap.c and efi_helper.c
> >>>
> >>> boot/image-fdt.c | 7 ++++++
> >>> drivers/block/blkmap.c | 44 +++++++++++++++++++++++++++++++++++++
> >>> include/blkmap.h | 17 ++++++++++++++
> >>> include/efi.h | 13 +++++++++++
> >>> lib/efi_loader/efi_helper.c | 39 ++++++++++++++++++++++++++++++++
> >>> 5 files changed, 120 insertions(+)
> >>>
> >>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> >>> index 9d1598b1a93..8f718ad29f6 100644
> >>> --- a/boot/image-fdt.c
> >>> +++ b/boot/image-fdt.c
> >>> @@ -11,6 +11,7 @@
> >>> #include <command.h>
> >>> #include <fdt_support.h>
> >>> #include <fdtdec.h>
> >>> +#include <efi.h>
> >>> #include <env.h>
> >>> #include <errno.h>
> >>> #include <image.h>
> >>> @@ -649,6 +650,12 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
> >>> if (!ft_verify_fdt(blob))
> >>> goto err;
> >>>
> >>> + if (CONFIG_IS_ENABLED(BLKMAP) && CONFIG_IS_ENABLED(EFI_LOADER)) {
> >>> + fdt_ret = fdt_efi_pmem_setup(blob);
> >>> + if (fdt_ret)
> >>> + goto err;
> >>> + }
> >>> +
> >>> /* after here we are using a livetree */
> >>> if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
> >>> struct event_ft_fixup fixup;
> >>> diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
> >>> index eefed615998..5f7602d0405 100644
> >>> --- a/drivers/block/blkmap.c
> >>> +++ b/drivers/block/blkmap.c
> >>> @@ -498,6 +498,50 @@ err:
> >>> return err;
> >>> }
> >>>
> >>
> >> Please, add the missing documentation.
> >> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
> >
> > Sorry, I do not understand. Which function do you need documentation for?
>
> blkmap_mem_preserve_slice() is not documented. See line below.
blkmap_mem_preserve_slice() is a static function and not an API. Are
we supposed to document minor helper functions as well. Yes, I can
understand needing to document a static function which is providing an
important functionality in a module, e.g. lmb_add_region_flags(), but
I don't think this qualifies.
>
> >
> >>
> >>> +static bool blkmap_mem_preserve_slice(struct blkmap_slice *bms)
> >>> +{
> >>> + return (bms->attr & (BLKMAP_SLICE_MEM | BLKMAP_SLICE_PRESERVE)) ==
> >>> + (BLKMAP_SLICE_MEM | BLKMAP_SLICE_PRESERVE);
> >>> +}
> >>> +
> >>> +int blkmap_get_preserved_pmem_slice(int (*cb)(void *ctx, ulong addr,
> >>> + u32 size), void *ctx)
> >>> +{
> >>> + int ret;
> >>> + u32 size;
> >>> + ulong addr;
> >>> + struct udevice *dev;
> >>> + struct uclass *uc;
> >>> + struct blkmap *bm;
> >>> + struct blkmap_mem *bmm;
> >>> + struct blkmap_slice *bms;
> >>> + struct blk_desc *bd;
> >>> +
> >>> + if (!cb) {
> >>> + log_debug("%s: No callback passed to the function\n", __func__);
> >>> + return 0;
> >>> + }
> >>> +
> >>> + uclass_id_foreach_dev(UCLASS_BLKMAP, dev, uc) {
> >>> + bm = dev_get_plat(dev);
> >>> + bd = dev_get_uclass_plat(bm->blk);
> >>> +
> >>> + list_for_each_entry(bms, &bm->slices, node) {
> >>> + if (!blkmap_mem_preserve_slice(bms))
> >>> + continue;
> >>> +
> >>> + bmm = container_of(bms, struct blkmap_mem, slice);
> >>> + addr = (ulong)(uintptr_t)bmm->addr;
> >>> + size = (u32)bms->blkcnt << bd->log2blksz;
> >>> + ret = cb(ctx, addr, size);
> >>> + if (ret)
> >>> + return ret;
> >>> + }
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> int blkmap_destroy(struct udevice *dev)
> >>> {
> >>> int err;
> >>> diff --git a/include/blkmap.h b/include/blkmap.h
> >>> index 754d8671b01..30ce80c0ad1 100644
> >>> --- a/include/blkmap.h
> >>> +++ b/include/blkmap.h
> >>> @@ -7,6 +7,7 @@
> >>> #ifndef _BLKMAP_H
> >>> #define _BLKMAP_H
> >>>
> >>> +#include <blk.h>
> >>> #include <dm/lists.h>
> >>>
> >>> /**
> >>> @@ -104,4 +105,20 @@ int blkmap_destroy(struct udevice *dev);
> >>> int blkmap_create_ramdisk(const char *label, ulong image_addr, ulong image_size,
> >>> struct udevice **devp);
> >>>
> >>> +/**
> >>> + * blkmap_get_preserved_pmem_slice() - Look for memory mapped preserved slice
> >>> + * @cb: Callback function to call for the blkmap slice
> >>
> >> Please, describe in sufficient detail for a developer who has to
> >> implement such a callback function what information should be provided
> >> by the function.
> >>
> >> All arguments of cb and its return value need to be described.
> >
> > Shouldn't the callback be an abstract interface which allows the
> > caller of this function to do whatever configuration that the caller
> > wants to do. Which is why I have framed it as such in the function's
> > comments.
>
> The description should provide all information that is needed to use
> function without reading its implementation.
>
> The existing documentation does not describe:
>
> * What information ctx, addr, size will contain.
Okay, I can add this information. But again, I think 'ctx' is supposed
to be an opaque object. Maybe I can give an example.
> * When the callback function will be invoked. Is it just once or for
> every matching slice?
Okay
> * Does the callback function have to fill some data structure that
> blkmap_get_preserved_pmem_slice() needs to do its job.
The function description says that the callback is to be called for
additional configuration that might be needed. Since there is no
mention of this fact(filling any data structure), does it not imply
that.
>
> It is unclear what values the return value should take and if it has any
> influence on the output of blkmap_get_preserved_pmem_slice(). Will
> iterating stop at the first error reported by the callback? Or will all
> iterations be executed?
Okay, I will add this information.
-sughosh
>
> >
> > -sughosh
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> + * @ctx: Argument to be passed to the callback function
> >>> + *
> >>> + * The function is used to iterate through all the blkmap slices, looking
> >>> + * specifically for memory mapped blkmap mapping which has been
>
> %s/mapping which has been/mappings which have been/
>
> >>> + * created with the preserve attribute. The function looks for such slices
> >>> + * with the relevant attributes and then calls the callback function which
> >>> + * then does additional configuration as needed.
> >>> + *
> >>> + * Return: 0 on success, negative error on failure
> >>> + */
> >>> +int blkmap_get_preserved_pmem_slice(int (*cb)(void *ctx, ulong addr,
> >>> + u32 size), void *ctx);
> >>> +
> >>> #endif /* _BLKMAP_H */
> >>> diff --git a/include/efi.h b/include/efi.h
> >>> index d005cb6181e..f9bbb175c3a 100644
> >>> --- a/include/efi.h
> >>> +++ b/include/efi.h
> >>> @@ -705,4 +705,17 @@ static inline bool efi_use_host_arch(void)
> >>> */
> >>> int efi_get_pxe_arch(void);
> >>>
> >>> +/**
> >>> + * fdt_efi_pmem_setup() - Pmem setup in DT and EFI memory map
> >>> + * @fdt: Devicetree to add the pmem nodes to
> >>> + *
> >>> + * Iterate through all the blkmap devices, look for BLKMAP_MEM devices,
> >>> + * and add pmem nodes corresponding to the blkmap slice to the
> >>> + * devicetree along with removing the corresponding region from the
> >>> + * EFI memory map.
> >>> + *
> >>> + * Returns: 0 on success, negative error on failure
> >>> + */
> >>> +int fdt_efi_pmem_setup(void *fdt);
> >>> +
> >>> #endif /* _LINUX_EFI_H */
> >>> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> >>> index 37e5859741f..62772a0d747 100644
> >>> --- a/lib/efi_loader/efi_helper.c
> >>> +++ b/lib/efi_loader/efi_helper.c
> >>> @@ -5,6 +5,7 @@
> >>>
> >>> #define LOG_CATEGORY LOGC_EFI
> >>>
> >>> +#include <blkmap.h>
> >>> #include <bootm.h>
> >>> #include <env.h>
> >>> #include <image.h>
> >>> @@ -680,3 +681,41 @@ out:
> >>>
> >>> return ret;
> >>> }
> >>> +
> >>> +/**
> >>> + * pmem_node_efi_memmap_setup() - Add pmem node and tweak EFI memmap
> >>> + * @fdt: The devicetree to which pmem node is added
> >>> + * @addr: start address of the pmem node
> >>> + * @size: size of the memory of the pmem node
> >>> + *
> >>> + * The function adds the pmem node to the device-tree along with removing
>
> %s/along with removing/and removes/
>
> Best regards
>
> Heinrich
>
> >>> + * the corresponding region from the EFI memory map. Used primarily to
> >>> + * pass the information of a RAM based ISO image to the OS.
> >>> + *
> >>> + * Return: 0 on success, -ve value on error
> >>> + */
> >>> +static int pmem_node_efi_memmap_setup(void *fdt, ulong addr, u32 size)
> >>> +{
> >>> + int ret;
> >>> + efi_status_t status;
> >>> +
> >>> + ret = fdt_fixup_pmem_region(fdt, addr, size);
> >>> + if (ret) {
> >>> + log_err("Failed to setup pmem node for addr %#lx, size %#x, err %d\n",
> >>> + addr, size, ret);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + status = efi_remove_memory_map(addr, size,
> >>> + EFI_CONVENTIONAL_MEMORY);
> >>> + if (status != EFI_SUCCESS)
> >>> + return -1;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +int fdt_efi_pmem_setup(void *fdt)
> >>> +{
> >>> + return blkmap_get_preserved_pmem_slice(pmem_node_efi_memmap_setup,
> >>> + fdt);
> >>> +}
> >>
>
More information about the U-Boot
mailing list