All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/6] Add pmem node for preserving distro ISO's
@ 2025-02-27 11:15 Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 1/6] fdt: add support for adding pmem nodes Sughosh Ganu
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 11:15 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng


When installing a distro via EFI HTTP boot some OS installers expect
the .iso image to be preserved and treat it as a "CDROM" to install
packages.

This is problematic in EFI, since U-Boot mounts the image, starts the
installer, and eventually calls ExitBootServices. At that point the
image U-Boot mounted disappears. Some distros don't care and download
the missing packages from a web archive, while others halt the
installation complaining they can't find certain packages.

If the firmware uses ACPI, this is supported by using NFIT which
provides NVDIMM ramdisks to the OS and preserves the image.
We don't have anything in place for Device Trees though. Since DT
supports persistent memory nodes (pmem) use those and preserve the
.iso for installers.

The issue can be reproduced by attempting an EFI HTTP boot with Ubuntu
live server ISO, or a Rocky Linux ISO. The installation would fail
with the failure to locate certain packages.

The patches are adding support for adding the pmem node to the DT that
is being passed to the OS, along with removing the memory region
containing the ISO image from the EFI memory map. This is being done
through a helper function in the blkmap driver which scans for all
blkmap mappings and does the above configurations for the relevant
mappings.

Changes since V4:
* Rebase patch(2) changes on top of the next branch
* Use BIT() based macros instead of enum
* Change the name of the field from type to attr as it would contain
  attributes other than the type of the slice
* Reword the commit message(patch 6)
* Add a helper function blkmap_get_preserved_pmem_slice()
* Add a function pmem_node_efi_memmap_setup() for pmem node and EFI
  memmap related setup




Ilias Apalodimas (2):
  efi_loader: add a function to remove memory from the EFI map
  efi_loader: preserve installer images in pmem

Masahisa Kojima (1):
  fdt: add support for adding pmem nodes

Sughosh Ganu (3):
  blkmap: store type of blkmap slice in corresponding structure
  blkmap: add an attribute to preserve the mem mapping
  blkmap: pass information on ISO image to the OS

 boot/fdt_support.c            | 41 ++++++++++++++++++++++-
 boot/image-fdt.c              |  7 ++++
 cmd/blkmap.c                  |  9 +++--
 drivers/block/blkmap.c        | 63 ++++++++++++++++++++++++++++++++---
 drivers/block/blkmap_helper.c |  2 +-
 include/blkmap.h              | 21 +++++++++++-
 include/efi.h                 | 13 ++++++++
 include/efi_loader.h          | 18 ++++++++++
 include/fdt_support.h         | 14 ++++++++
 lib/efi_loader/efi_bootmgr.c  | 22 +++++++++---
 lib/efi_loader/efi_helper.c   | 37 ++++++++++++++++++++
 lib/efi_loader/efi_memory.c   | 56 ++++++++++++++++++++++---------
 12 files changed, 273 insertions(+), 30 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v5 1/6] fdt: add support for adding pmem nodes
  2025-02-27 11:15 [PATCH v5 0/6] Add pmem node for preserving distro ISO's Sughosh Ganu
@ 2025-02-27 11:15 ` Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 2/6] efi_loader: add a function to remove memory from the EFI map Sughosh Ganu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 11:15 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng, Masahisa Kojima,
	Sughosh Ganu

From: Masahisa Kojima <kojima.masahisa@socionext.com>

One of the problems OS installers face, when running in EFI, is that
the mounted ISO after calling ExitBootServices goes away. For some
distros this is a problem since they rely on finding some core packages
before continuing the installation. Distros have works around this --
e.g Fedora has a special kernel command line parameter called
inst.stage2 [0].

ACPI has NFIT and NVDIMM support to provide ramdisks to the OS, but we
don't have anything in place for DTs. Linux and device trees have support
for persistent memory devices. So add a function that can inject a pmem
node in a DT, so we can use it when launhing OS installers with EFI.

[0]
https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/7/html/installation_guide/chap-anaconda-boot-options#sect-boot-options-installer

Signed-off-by: Masahisa Kojima <kojima.masahisa@socionext.com>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> 
---
Changes since V4: None

 boot/fdt_support.c    | 41 ++++++++++++++++++++++++++++++++++++++++-
 include/fdt_support.h | 14 ++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/boot/fdt_support.c b/boot/fdt_support.c
index 49efeec3681..e20b9759138 100644
--- a/boot/fdt_support.c
+++ b/boot/fdt_support.c
@@ -18,6 +18,7 @@
 #include <dm/ofnode.h>
 #include <linux/ctype.h>
 #include <linux/types.h>
+#include <linux/sizes.h>
 #include <asm/global_data.h>
 #include <asm/unaligned.h>
 #include <linux/libfdt.h>
@@ -464,7 +465,6 @@ void do_fixup_by_compat_u32(void *fdt, const char *compat,
 	do_fixup_by_compat(fdt, compat, prop, &tmp, 4, create);
 }
 
-#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
 /*
  * fdt_pack_reg - pack address and size array into the "reg"-suitable stream
  */
@@ -493,6 +493,7 @@ static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size,
 	return p - (char *)buf;
 }
 
+#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
 #if CONFIG_NR_DRAM_BANKS > 4
 #define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS
 #else
@@ -2222,3 +2223,41 @@ int fdt_valid(struct fdt_header **blobp)
 	}
 	return 1;
 }
+
+int fdt_fixup_pmem_region(void *blob, ulong addr, u32 size)
+{
+	u64 pmem_start[2] = { 0 };
+	u64 pmem_size[2] = { 0 };
+	char pmem_node[32] = {0};
+	int nodeoffset, len;
+	int err;
+	u8 tmp[4 * 16]; /* Up to 64-bit address + 64-bit size */
+
+	if (!IS_ALIGNED(addr, SZ_2M) || !IS_ALIGNED(addr + size, SZ_2M)) {
+		printf("Start and end address must be 2MiB aligned\n");
+		return -1;
+	}
+
+	snprintf(pmem_node, sizeof(pmem_node), "pmem@%lx", addr);
+	nodeoffset = fdt_find_or_add_subnode(blob, 0, pmem_node);
+	if (nodeoffset < 0)
+		return nodeoffset;
+
+	err = fdt_setprop_string(blob, nodeoffset, "compatible", "pmem-region");
+	if (err)
+		return err;
+	err = fdt_setprop_empty(blob, nodeoffset, "volatile");
+	if (err)
+		return err;
+	pmem_start[0] = addr;
+	pmem_size[0] = size;
+	len = fdt_pack_reg(blob, tmp, pmem_start, pmem_size, 1);
+	err = fdt_setprop(blob, nodeoffset, "reg", tmp, len);
+	if (err < 0) {
+		printf("WARNING: could not set pmem %s %s.\n", "reg",
+		       fdt_strerror(err));
+		return err;
+	}
+
+	return 0;
+}
diff --git a/include/fdt_support.h b/include/fdt_support.h
index f0ad2e6b365..b72cd2920de 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -507,4 +507,18 @@ void fdt_fixup_pstore(void *blob);
  */
 int fdt_kaslrseed(void *blob, bool overwrite);
 
+/**
+ * fdt_fixup_pmem_region() - add a pmem node on the device tree
+ *
+ * This functions adds/updates a pmem node to the device tree.
+ * Usually used with EFI installers to preserve installer
+ * images
+ *
+ * @blob:	device tree provided by caller
+ * @addr:	start address of the pmem node
+ * @size:	size of the memory of the pmem node
+ * Return:	0 on success or < 0 on failure
+ */
+int fdt_fixup_pmem_region(void *blob, ulong addr, u32 size);
+
 #endif /* ifndef __FDT_SUPPORT_H */
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v5 2/6] efi_loader: add a function to remove memory from the EFI map
  2025-02-27 11:15 [PATCH v5 0/6] Add pmem node for preserving distro ISO's Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 1/6] fdt: add support for adding pmem nodes Sughosh Ganu
@ 2025-02-27 11:15 ` Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 3/6] efi_loader: preserve installer images in pmem Sughosh Ganu
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 11:15 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng, Heinrich Schuchardt,
	Sughosh Ganu

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

With upcoming changes supporting pmem nodes, we need to remove the
pmem area from the EFI memory map. Add a function to do that.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V4:
* Rebase patch changes on top of the next branch

 include/efi_loader.h        | 18 ++++++++++++
 lib/efi_loader/efi_memory.c | 56 ++++++++++++++++++++++++++-----------
 2 files changed, 58 insertions(+), 16 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1d75d97ebbc..0ac00911598 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -852,6 +852,24 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 /* Adds a range into the EFI memory map */
 efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
 
+/**
+ * efi_update_memory_map() - update the memory map by adding/removing pages
+ *
+ * @start:			start address, must be a multiple of
+ *				EFI_PAGE_SIZE
+ * @pages:			number of pages to add
+ * @memory_type:		type of memory added
+ * @overlap_conventional:	region may only overlap free(conventional)
+ *				memory
+ * @remove:			remove memory map
+ * Return:			status code
+ */
+efi_status_t efi_update_memory_map(u64 start, u64 pages, int memory_type,
+				   bool overlap_conventional, bool remove);
+
+/* Remove memory from the EFI memory map */
+efi_status_t efi_remove_memory_map(u64 start, u64 size, int memory_type);
+
 /* Called by board init to initialize the EFI drivers */
 efi_status_t efi_driver_init(void);
 /* Called when a block device is added */
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 6d00b186250..3fec40b6de5 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -258,7 +258,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
 }
 
 /**
- * efi_add_memory_map_pg() - add pages to the memory map
+ * efi_update_memory_map() - update the memory map by adding/removing pages
  *
  * @start:			start address, must be a multiple of
  *				EFI_PAGE_SIZE
@@ -266,12 +266,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map,
  * @memory_type:		type of memory added
  * @overlap_conventional:	region may only overlap free(conventional)
  *				memory
+ * @remove:			remove memory map
  * Return:			status code
  */
-static
-efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
-				   int memory_type,
-				   bool overlap_conventional)
+efi_status_t efi_update_memory_map(u64 start, u64 pages, int memory_type,
+				   bool overlap_conventional, bool remove)
 {
 	struct efi_mem_list *lmem;
 	struct efi_mem_list *newlist;
@@ -279,9 +278,9 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 	uint64_t carved_pages = 0;
 	struct efi_event *evt;
 
-	EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
+	EFI_PRINT("%s: 0x%llx 0x%llx %d %s %s\n", __func__,
 		  start, pages, memory_type, overlap_conventional ?
-		  "yes" : "no");
+		  "yes" : "no", remove ? "remove" : "add");
 
 	if (memory_type >= EFI_MAX_MEMORY_TYPE)
 		return EFI_INVALID_PARAMETER;
@@ -364,7 +363,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
 	}
 
 	/* Add our new map */
-        list_add_tail(&newlist->link, &efi_mem);
+	if (!remove)
+		list_add_tail(&newlist->link, &efi_mem);
+	else
+		free(newlist);
 
 	/* And make sure memory is listed in descending order */
 	efi_mem_sort();
@@ -401,7 +403,29 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type)
 	pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK));
 	start &= ~EFI_PAGE_MASK;
 
-	return efi_add_memory_map_pg(start, pages, memory_type, false);
+	return efi_update_memory_map(start, pages, memory_type, false, false);
+}
+
+/**
+ * efi_remove_memory_map() - remove memory area to the memory map
+ *
+ * @start:		start address of the memory area
+ * @size:		length in bytes of the memory area
+ * @memory_type:	type of memory removed
+ *
+ * Return:		status code
+ *
+ * This function automatically aligns the start and size of the memory area
+ * to EFI_PAGE_SIZE.
+ */
+efi_status_t efi_remove_memory_map(u64 start, u64 size, int memory_type)
+{
+	u64 pages;
+
+	pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK));
+	start &= ~EFI_PAGE_MASK;
+
+	return efi_update_memory_map(start, pages, memory_type, false, true);
 }
 
 /**
@@ -502,7 +526,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 
 	efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0);
 	/* Reserve that map in our memory maps */
-	ret = efi_add_memory_map_pg(efi_addr, pages, memory_type, true);
+	ret = efi_update_memory_map(efi_addr, pages, memory_type, true, false);
 	if (ret != EFI_SUCCESS) {
 		/* Map would overlap, bail out */
 		lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
@@ -823,8 +847,8 @@ static void add_u_boot_and_runtime(void)
 		       uboot_stack_size) & ~EFI_PAGE_MASK;
 	uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
 		       uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
-	efi_add_memory_map_pg(uboot_start, uboot_pages, EFI_BOOT_SERVICES_CODE,
-			      false);
+	efi_update_memory_map(uboot_start, uboot_pages, EFI_BOOT_SERVICES_CODE,
+			      false, false);
 #if defined(__aarch64__)
 	/*
 	 * Runtime Services must be 64KiB aligned according to the
@@ -842,8 +866,8 @@ static void add_u_boot_and_runtime(void)
 	runtime_end = (uintptr_t)__efi_runtime_stop;
 	runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;
 	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
-	efi_add_memory_map_pg(runtime_start, runtime_pages,
-			      EFI_RUNTIME_SERVICES_CODE, false);
+	efi_update_memory_map(runtime_start, runtime_pages,
+			      EFI_RUNTIME_SERVICES_CODE, false, false);
 }
 
 int efi_memory_init(void)
@@ -878,11 +902,11 @@ int efi_map_update_notify(phys_addr_t addr, phys_size_t size,
 	pages = efi_size_in_pages(size + (efi_addr & EFI_PAGE_MASK));
 	efi_addr &= ~EFI_PAGE_MASK;
 
-	status = efi_add_memory_map_pg(efi_addr, pages,
+	status = efi_update_memory_map(efi_addr, pages,
 				       op == LMB_MAP_OP_RESERVE ?
 				       EFI_BOOT_SERVICES_DATA :
 				       EFI_CONVENTIONAL_MEMORY,
-				       false);
+				       false, false);
 	if (status != EFI_SUCCESS) {
 		log_err("LMB Map notify failure %lu\n",
 			status & ~EFI_ERROR_MASK);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v5 3/6] efi_loader: preserve installer images in pmem
  2025-02-27 11:15 [PATCH v5 0/6] Add pmem node for preserving distro ISO's Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 1/6] fdt: add support for adding pmem nodes Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 2/6] efi_loader: add a function to remove memory from the EFI map Sughosh Ganu
@ 2025-02-27 11:15 ` Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure Sughosh Ganu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 11:15 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng, Sughosh Ganu

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

One of the problems OS installers face, when running in EFI, is that
the mounted ISO after calling ExitBootServices goes away. For some
distros this is a problem since they rely on finding some core packages
before continuing the installation. Distros have works around this --
e.g Fedora has a special kernel command line parameter called
inst.stage2 [0].

ACPI has NFIT and NVDIMM support to provide ramdisks to the OS, but we
don't have anything in place for DTs. Linux and device trees have support
for persistent memory devices.

It's worth noting that for linux to instantiate the /dev/pmemX device,
the memory described in the pmem node has to be omitted from the EFI
memory map we hand over to the OS if ZONE_DEVICES and SPARSEMEM is
enabled. With those enabled the pmem driver ends up calling
devm_memremap_pages() instead of devm_memremap(). The latter works
whether the memory is omitted or marked as reserved, but mapping pages
only works if the memory is omitted.

On top of that, depending on how the kernel is configured, that memory
area must be page aligned or 2MB aligned. PowerPC is an exception here
and requires 16MB alignment, but since we don't have EFI support for
it, limit the alignment to 2MB.

Ensure that the ISO image is 2MB aligned and remove the region
occupied by the image from the EFI memory map.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V4: None

 lib/efi_loader/efi_bootmgr.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index c6124c590d9..081eff057f4 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -18,6 +18,8 @@
 #include <efi_loader.h>
 #include <efi_variable.h>
 #include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/sizes.h>
 
 static const struct efi_boot_services *bs;
 static const struct efi_runtime_services *rs;
@@ -362,13 +364,16 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
 	}
 
 	/*
-	 * TODO: expose the ramdisk to OS.
-	 * Need to pass the ramdisk information by the architecture-specific
-	 * methods such as 'pmem' device-tree node.
+	 * Linux supports 'pmem' which allows OS installers to find, reclaim
+	 * the mounted images and continue the installation since the contents
+	 * of the pmem region are treated as local media.
+	 *
+	 * The memory regions used for it needs to be carved out of the EFI
+	 * memory map.
 	 */
-	ret = efi_add_memory_map(addr, size, EFI_RESERVED_MEMORY_TYPE);
+	ret = efi_remove_memory_map(addr, size, EFI_CONVENTIONAL_MEMORY);
 	if (ret != EFI_SUCCESS) {
-		log_err("Memory reservation failed\n");
+		log_err("Failed to reserve memory\n");
 		goto err;
 	}
 
@@ -490,6 +495,13 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 		ret = EFI_INVALID_PARAMETER;
 		goto err;
 	}
+	/*
+	 * Depending on the kernel configuration, pmem memory area must be page
+	 * aligned or 2MB aligned. PowerPC is an exception here and requires
+	 * 16MB alignment, but since we don't have EFI support for it, limit
+	 * the alignment to 2MB.
+	 */
+	image_size = ALIGN(image_size, SZ_2M);
 
 	/*
 	 * If the file extension is ".iso" or ".img", mount it and try to load
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure
  2025-02-27 11:15 [PATCH v5 0/6] Add pmem node for preserving distro ISO's Sughosh Ganu
                   ` (2 preceding siblings ...)
  2025-02-27 11:15 ` [PATCH v5 3/6] efi_loader: preserve installer images in pmem Sughosh Ganu
@ 2025-02-27 11:15 ` Sughosh Ganu
  2025-02-27 19:49   ` Tobias Waldekranz
  2025-02-28  6:08   ` Ilias Apalodimas
  2025-02-27 11:15 ` [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping Sughosh Ganu
  2025-02-27 11:15 ` [PATCH v5 6/6] blkmap: pass information on ISO image to the OS Sughosh Ganu
  5 siblings, 2 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 11:15 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng, Sughosh Ganu

Add information about the type of blkmap slice as an attribute in the
corresponding slice structure. Put information in the blkmap slice
structure to identify if it is associated with a memory or linear
mapped device. Which can then be used to take specific action based on
the type of the blkmap slice.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V4:
* Use BIT() based macros instead of enum
* Change the name of the field from type to attr as it would contain
  attributes other than the type of the slice

 drivers/block/blkmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
index 34eed1380dc..453510cca62 100644
--- a/drivers/block/blkmap.c
+++ b/drivers/block/blkmap.c
@@ -16,6 +16,10 @@
 
 struct blkmap;
 
+/* Attributes of blkmap slice */
+#define BLKMAP_SLICE_LINEAR	BIT(0)
+#define BLKMAP_SLICE_MEM	BIT(1)
+
 /**
  * struct blkmap_slice - Region mapped to a blkmap
  *
@@ -25,12 +29,14 @@ struct blkmap;
  * @node: List node used to associate this slice with a blkmap
  * @blknr: Start block number of the mapping
  * @blkcnt: Number of blocks covered by this mapping
+ * @attr: Attributes of blkmap slice
  */
 struct blkmap_slice {
 	struct list_head node;
 
 	lbaint_t blknr;
 	lbaint_t blkcnt;
+	uint     attr;
 
 	/**
 	 * @read: - Read from slice
@@ -169,6 +175,7 @@ int blkmap_map_linear(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 		.slice = {
 			.blknr = blknr,
 			.blkcnt = blkcnt,
+			.attr = BLKMAP_SLICE_LINEAR,
 
 			.read = blkmap_linear_read,
 			.write = blkmap_linear_write,
@@ -248,6 +255,7 @@ int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 		.slice = {
 			.blknr = blknr,
 			.blkcnt = blkcnt,
+			.attr = BLKMAP_SLICE_MEM,
 
 			.read = blkmap_mem_read,
 			.write = blkmap_mem_write,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping
  2025-02-27 11:15 [PATCH v5 0/6] Add pmem node for preserving distro ISO's Sughosh Ganu
                   ` (3 preceding siblings ...)
  2025-02-27 11:15 ` [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure Sughosh Ganu
@ 2025-02-27 11:15 ` Sughosh Ganu
  2025-02-27 19:51   ` Tobias Waldekranz
  2025-02-28  6:24   ` Ilias Apalodimas
  2025-02-27 11:15 ` [PATCH v5 6/6] blkmap: pass information on ISO image to the OS Sughosh Ganu
  5 siblings, 2 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 11:15 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng, Sughosh Ganu

Some blkmap memory mapped devices might have to be be relevant even
after U-Boot passes control to the next image as part of the platform
boot. An example of such a mapping would be an OS installer ISO image,
information for which has to be provided to the OS kernel. Use the
'preserve' attribute for such mappings. The code for adding a pmem
node to the device-tree then checks if this attribute is set, and adds
a node only for mappings which have this attribute.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V4: New patch

 cmd/blkmap.c                  |  9 +++++++--
 drivers/block/blkmap.c        | 12 ++++++++----
 drivers/block/blkmap_helper.c |  2 +-
 include/blkmap.h              |  4 +++-
 4 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/cmd/blkmap.c b/cmd/blkmap.c
index 164f80f1387..86a123b1cd3 100644
--- a/cmd/blkmap.c
+++ b/cmd/blkmap.c
@@ -62,13 +62,18 @@ static int do_blkmap_map_mem(struct map_ctx *ctx, int argc, char *const argv[])
 {
 	phys_addr_t addr;
 	int err;
+	bool preserve = false;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
 	addr = hextoul(argv[1], NULL);
 
-	err = blkmap_map_pmem(ctx->dev, ctx->blknr, ctx->blkcnt, addr);
+	if (argc == 3 && !strcmp(argv[2], "preserve"))
+		preserve = true;
+
+	err = blkmap_map_pmem(ctx->dev, ctx->blknr, ctx->blkcnt, addr,
+			      preserve);
 	if (err) {
 		printf("Unable to map %#llx at block 0x" LBAF ": %d\n",
 		       (unsigned long long)addr, ctx->blknr, err);
@@ -221,7 +226,7 @@ U_BOOT_CMD_WITH_SUBCMDS(
 	"blkmap create <label> - create device\n"
 	"blkmap destroy <label> - destroy device\n"
 	"blkmap map <label> <blk#> <cnt> linear <interface> <dev> <blk#> - device mapping\n"
-	"blkmap map <label> <blk#> <cnt> mem <addr> - memory mapping\n",
+	"blkmap map <label> <blk#> <cnt> mem <addr> [preserve] - memory mapping\n",
 	U_BOOT_SUBCMD_MKENT(info, 2, 1, do_blkmap_common),
 	U_BOOT_SUBCMD_MKENT(part, 2, 1, do_blkmap_common),
 	U_BOOT_SUBCMD_MKENT(dev, 4, 1, do_blkmap_common),
diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
index 453510cca62..eefed615998 100644
--- a/drivers/block/blkmap.c
+++ b/drivers/block/blkmap.c
@@ -19,6 +19,7 @@ struct blkmap;
 /* Attributes of blkmap slice */
 #define BLKMAP_SLICE_LINEAR	BIT(0)
 #define BLKMAP_SLICE_MEM	BIT(1)
+#define BLKMAP_SLICE_PRESERVE	BIT(2)
 
 /**
  * struct blkmap_slice - Region mapped to a blkmap
@@ -241,7 +242,7 @@ static void blkmap_mem_destroy(struct blkmap *bm, struct blkmap_slice *bms)
 }
 
 int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
-		     void *addr, bool remapped)
+		     void *addr, bool remapped, bool preserve)
 {
 	struct blkmap *bm = dev_get_plat(dev);
 	struct blkmap_mem *bmm;
@@ -266,6 +267,9 @@ int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 		.remapped = remapped,
 	};
 
+	if (preserve)
+		bmm->slice.attr |= BLKMAP_SLICE_PRESERVE;
+
 	err = blkmap_slice_add(bm, &bmm->slice);
 	if (err)
 		free(bmm);
@@ -276,11 +280,11 @@ int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 int blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 		   void *addr)
 {
-	return __blkmap_map_mem(dev, blknr, blkcnt, addr, false);
+	return __blkmap_map_mem(dev, blknr, blkcnt, addr, false, false);
 }
 
 int blkmap_map_pmem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
-		    phys_addr_t paddr)
+		    phys_addr_t paddr, bool preserve)
 {
 	struct blkmap *bm = dev_get_plat(dev);
 	struct blk_desc *bd = dev_get_uclass_plat(bm->blk);
@@ -291,7 +295,7 @@ int blkmap_map_pmem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 	if (!addr)
 		return -ENOMEM;
 
-	err = __blkmap_map_mem(dev, blknr, blkcnt, addr, true);
+	err = __blkmap_map_mem(dev, blknr, blkcnt, addr, true, preserve);
 	if (err)
 		unmap_sysmem(addr);
 
diff --git a/drivers/block/blkmap_helper.c b/drivers/block/blkmap_helper.c
index bfba14110d2..2f1bc28ee5d 100644
--- a/drivers/block/blkmap_helper.c
+++ b/drivers/block/blkmap_helper.c
@@ -28,7 +28,7 @@ int blkmap_create_ramdisk(const char *label, ulong image_addr, ulong image_size,
 	bm = dev_get_plat(bm_dev);
 	desc = dev_get_uclass_plat(bm->blk);
 	blknum = image_size >> desc->log2blksz;
-	ret = blkmap_map_pmem(bm_dev, 0, blknum, image_addr);
+	ret = blkmap_map_pmem(bm_dev, 0, blknum, image_addr, true);
 	if (ret) {
 		log_err("Unable to map %#llx at block %d : %d\n",
 			(unsigned long long)image_addr, 0, ret);
diff --git a/include/blkmap.h b/include/blkmap.h
index d53095437fa..754d8671b01 100644
--- a/include/blkmap.h
+++ b/include/blkmap.h
@@ -60,10 +60,12 @@ int blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
  * @blknr: Start block number of the mapping
  * @blkcnt: Number of blocks to map
  * @paddr: The target physical memory address of the mapping
+ * @preserve: Mapping intended to be preserved for subsequent stages,
+ *		like the OS (e.g. ISO installer)
  * Returns: 0 on success, negative error code on failure
  */
 int blkmap_map_pmem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
-		    phys_addr_t paddr);
+		    phys_addr_t paddr, bool preserve);
 
 /**
  * blkmap_from_label() - Find blkmap from label
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v5 6/6] blkmap: pass information on ISO image to the OS
  2025-02-27 11:15 [PATCH v5 0/6] Add pmem node for preserving distro ISO's Sughosh Ganu
                   ` (4 preceding siblings ...)
  2025-02-27 11:15 ` [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping Sughosh Ganu
@ 2025-02-27 11:15 ` Sughosh Ganu
  2025-02-27 12:26   ` Sughosh Ganu
  2025-02-27 20:00   ` Tobias Waldekranz
  5 siblings, 2 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 11:15 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng, Sughosh Ganu

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@linaro.org>
---
Changes since V4:
* Reword the commit message
* Add a helper function blkmap_get_preserved_pmem_slice()
* Add a function pmem_node_efi_memmap_setup() for pmem node and EFI
  memmap related setup

 boot/image-fdt.c            |  7 ++++++
 drivers/block/blkmap.c      | 43 +++++++++++++++++++++++++++++++++++++
 include/blkmap.h            | 17 +++++++++++++++
 include/efi.h               | 13 +++++++++++
 lib/efi_loader/efi_helper.c | 37 +++++++++++++++++++++++++++++++
 5 files changed, 117 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..c9e0a3a6eea 100644
--- a/drivers/block/blkmap.c
+++ b/drivers/block/blkmap.c
@@ -498,6 +498,49 @@ err:
 	return err;
 }
 
+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;
+
+	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) || !cb)
+				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) {
+				log_err("Failed to setup pmem node for addr %#lx, size %#x\n",
+					addr, size);
+				return -1;
+			}
+		}
+	}
+
+	return 0;
+
+}
+
 int blkmap_destroy(struct udevice *dev)
 {
 	int err;
diff --git a/include/blkmap.h b/include/blkmap.h
index 754d8671b01..89bd2b65fba 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
+ * @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
+ * 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, -1 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 04b2efc4a3b..675f3efa79a 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,39 @@ 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
+ * 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)
+		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);
+}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 6/6] blkmap: pass information on ISO image to the OS
  2025-02-27 11:15 ` [PATCH v5 6/6] blkmap: pass information on ISO image to the OS Sughosh Ganu
@ 2025-02-27 12:26   ` Sughosh Ganu
  2025-02-27 20:00   ` Tobias Waldekranz
  1 sibling, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-27 12:26 UTC (permalink / raw)
  To: u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Tobias Waldekranz, Bin Meng

On Thu, 27 Feb 2025 at 16:46, Sughosh Ganu <sughosh.ganu@linaro.org> 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@linaro.org>
> ---
> Changes since V4:
> * Reword the commit message
> * Add a helper function blkmap_get_preserved_pmem_slice()
> * Add a function pmem_node_efi_memmap_setup() for pmem node and EFI
>   memmap related setup
>
>  boot/image-fdt.c            |  7 ++++++
>  drivers/block/blkmap.c      | 43 +++++++++++++++++++++++++++++++++++++
>  include/blkmap.h            | 17 +++++++++++++++
>  include/efi.h               | 13 +++++++++++
>  lib/efi_loader/efi_helper.c | 37 +++++++++++++++++++++++++++++++
>  5 files changed, 117 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..c9e0a3a6eea 100644
> --- a/drivers/block/blkmap.c
> +++ b/drivers/block/blkmap.c
> @@ -498,6 +498,49 @@ err:
>         return err;
>  }
>
> +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;
> +
> +       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) || !cb)
> +                               continue;

Realised after sending the patch that the check for the callback
function should be the first thing to happen outside the loops. Will
wait for other review comments, and incorporate this in the next
version.

-sughosh

> +
> +                       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) {
> +                               log_err("Failed to setup pmem node for addr %#lx, size %#x\n",
> +                                       addr, size);
> +                               return -1;
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +
> +}
> +
>  int blkmap_destroy(struct udevice *dev)
>  {
>         int err;
> diff --git a/include/blkmap.h b/include/blkmap.h
> index 754d8671b01..89bd2b65fba 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
> + * @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
> + * 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, -1 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 04b2efc4a3b..675f3efa79a 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,39 @@ 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
> + * 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)
> +               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);
> +}
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure
  2025-02-27 11:15 ` [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure Sughosh Ganu
@ 2025-02-27 19:49   ` Tobias Waldekranz
  2025-02-28  6:08   ` Ilias Apalodimas
  1 sibling, 0 replies; 14+ messages in thread
From: Tobias Waldekranz @ 2025-02-27 19:49 UTC (permalink / raw)
  To: Sughosh Ganu, u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Bin Meng, Sughosh Ganu

On tor, feb 27, 2025 at 16:45, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> Add information about the type of blkmap slice as an attribute in the
> corresponding slice structure. Put information in the blkmap slice
> structure to identify if it is associated with a memory or linear
> mapped device. Which can then be used to take specific action based on
> the type of the blkmap slice.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping
  2025-02-27 11:15 ` [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping Sughosh Ganu
@ 2025-02-27 19:51   ` Tobias Waldekranz
  2025-02-28  6:24   ` Ilias Apalodimas
  1 sibling, 0 replies; 14+ messages in thread
From: Tobias Waldekranz @ 2025-02-27 19:51 UTC (permalink / raw)
  To: Sughosh Ganu, u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Bin Meng, Sughosh Ganu

On tor, feb 27, 2025 at 16:45, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> Some blkmap memory mapped devices might have to be be relevant even
> after U-Boot passes control to the next image as part of the platform
> boot. An example of such a mapping would be an OS installer ISO image,
> information for which has to be provided to the OS kernel. Use the
> 'preserve' attribute for such mappings. The code for adding a pmem
> node to the device-tree then checks if this attribute is set, and adds
> a node only for mappings which have this attribute.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---

This turned out great!

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 6/6] blkmap: pass information on ISO image to the OS
  2025-02-27 11:15 ` [PATCH v5 6/6] blkmap: pass information on ISO image to the OS Sughosh Ganu
  2025-02-27 12:26   ` Sughosh Ganu
@ 2025-02-27 20:00   ` Tobias Waldekranz
  2025-02-28  6:54     ` Sughosh Ganu
  1 sibling, 1 reply; 14+ messages in thread
From: Tobias Waldekranz @ 2025-02-27 20:00 UTC (permalink / raw)
  To: Sughosh Ganu, u-boot
  Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Heinrich Schuchardt,
	Anton Antonov, Bin Meng, Sughosh Ganu

On tor, feb 27, 2025 at 16:45, Sughosh Ganu <sughosh.ganu@linaro.org> 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@linaro.org>
> ---

If a v6 is needed for some other reason (you seemed to indicate that),
then see my small comments below. Either way:

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

> Changes since V4:
> * Reword the commit message
> * Add a helper function blkmap_get_preserved_pmem_slice()
> * Add a function pmem_node_efi_memmap_setup() for pmem node and EFI
>   memmap related setup
>
>  boot/image-fdt.c            |  7 ++++++
>  drivers/block/blkmap.c      | 43 +++++++++++++++++++++++++++++++++++++
>  include/blkmap.h            | 17 +++++++++++++++
>  include/efi.h               | 13 +++++++++++
>  lib/efi_loader/efi_helper.c | 37 +++++++++++++++++++++++++++++++
>  5 files changed, 117 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..c9e0a3a6eea 100644
> --- a/drivers/block/blkmap.c
> +++ b/drivers/block/blkmap.c
> @@ -498,6 +498,49 @@ err:
>  	return err;
>  }
>  
> +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;
> +
> +	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) || !cb)
> +				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) {
> +				log_err("Failed to setup pmem node for addr %#lx, size %#x\n",
> +					addr, size);

IMO, this function should not make any assumptions about what the
callback is doing. If an error is encountered, then the callback should
decide if it warrants an entry in the log or not.

> +				return -1;

And the non-zero return value (`err`) of the callback should be passed
verbatim back to our caller here, in case it wants to discriminate
between different kinds of errors.

> +			}
> +		}
> +	}
> +
> +	return 0;
> +
> +}
> +
>  int blkmap_destroy(struct udevice *dev)
>  {
>  	int err;
> diff --git a/include/blkmap.h b/include/blkmap.h
> index 754d8671b01..89bd2b65fba 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
> + * @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
> + * 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, -1 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 04b2efc4a3b..675f3efa79a 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,39 @@ 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
> + * 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)
> +		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);

...i.e. here is the code that knows what the callback is doing, so it
can choose to emit the log message - without imposing that behavior on
any other users of the iterator.

> +}
> -- 
> 2.34.1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure
  2025-02-27 11:15 ` [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure Sughosh Ganu
  2025-02-27 19:49   ` Tobias Waldekranz
@ 2025-02-28  6:08   ` Ilias Apalodimas
  1 sibling, 0 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2025-02-28  6:08 UTC (permalink / raw)
  To: Sughosh Ganu
  Cc: u-boot, Simon Glass, Tom Rini, Heinrich Schuchardt, Anton Antonov,
	Tobias Waldekranz, Bin Meng

On Thu, 27 Feb 2025 at 13:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Add information about the type of blkmap slice as an attribute in the
> corresponding slice structure. Put information in the blkmap slice
> structure to identify if it is associated with a memory or linear
> mapped device. Which can then be used to take specific action based on
> the type of the blkmap slice.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V4:
> * Use BIT() based macros instead of enum
> * Change the name of the field from type to attr as it would contain
>   attributes other than the type of the slice
>
>  drivers/block/blkmap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
> index 34eed1380dc..453510cca62 100644
> --- a/drivers/block/blkmap.c
> +++ b/drivers/block/blkmap.c
> @@ -16,6 +16,10 @@
>
>  struct blkmap;
>
> +/* Attributes of blkmap slice */
> +#define BLKMAP_SLICE_LINEAR    BIT(0)
> +#define BLKMAP_SLICE_MEM       BIT(1)
> +
>  /**
>   * struct blkmap_slice - Region mapped to a blkmap
>   *
> @@ -25,12 +29,14 @@ struct blkmap;
>   * @node: List node used to associate this slice with a blkmap
>   * @blknr: Start block number of the mapping
>   * @blkcnt: Number of blocks covered by this mapping
> + * @attr: Attributes of blkmap slice
>   */
>  struct blkmap_slice {
>         struct list_head node;
>
>         lbaint_t blknr;
>         lbaint_t blkcnt;
> +       uint     attr;
>
>         /**
>          * @read: - Read from slice
> @@ -169,6 +175,7 @@ int blkmap_map_linear(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>                 .slice = {
>                         .blknr = blknr,
>                         .blkcnt = blkcnt,
> +                       .attr = BLKMAP_SLICE_LINEAR,
>
>                         .read = blkmap_linear_read,
>                         .write = blkmap_linear_write,
> @@ -248,6 +255,7 @@ int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>                 .slice = {
>                         .blknr = blknr,
>                         .blkcnt = blkcnt,
> +                       .attr = BLKMAP_SLICE_MEM,
>
>                         .read = blkmap_mem_read,
>                         .write = blkmap_mem_write,
> --
> 2.34.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping
  2025-02-27 11:15 ` [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping Sughosh Ganu
  2025-02-27 19:51   ` Tobias Waldekranz
@ 2025-02-28  6:24   ` Ilias Apalodimas
  1 sibling, 0 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2025-02-28  6:24 UTC (permalink / raw)
  To: Sughosh Ganu
  Cc: u-boot, Simon Glass, Tom Rini, Heinrich Schuchardt, Anton Antonov,
	Tobias Waldekranz, Bin Meng

Thanks Sughosh

On Thu, 27 Feb 2025 at 13:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Some blkmap memory mapped devices might have to be be relevant even
> after U-Boot passes control to the next image as part of the platform
> boot. An example of such a mapping would be an OS installer ISO image,
> information for which has to be provided to the OS kernel. Use the
> 'preserve' attribute for such mappings. The code for adding a pmem
> node to the device-tree then checks if this attribute is set, and adds
> a node only for mappings which have this attribute.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V4: New patch
>
>  cmd/blkmap.c                  |  9 +++++++--
>  drivers/block/blkmap.c        | 12 ++++++++----
>  drivers/block/blkmap_helper.c |  2 +-
>  include/blkmap.h              |  4 +++-
>  4 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/blkmap.c b/cmd/blkmap.c
> index 164f80f1387..86a123b1cd3 100644
> --- a/cmd/blkmap.c
> +++ b/cmd/blkmap.c
> @@ -62,13 +62,18 @@ static int do_blkmap_map_mem(struct map_ctx *ctx, int argc, char *const argv[])
>  {
>         phys_addr_t addr;
>         int err;
> +       bool preserve = false;
>
>         if (argc < 2)
>                 return CMD_RET_USAGE;
>
>         addr = hextoul(argv[1], NULL);
>
> -       err = blkmap_map_pmem(ctx->dev, ctx->blknr, ctx->blkcnt, addr);
> +       if (argc == 3 && !strcmp(argv[2], "preserve"))
> +               preserve = true;
> +
> +       err = blkmap_map_pmem(ctx->dev, ctx->blknr, ctx->blkcnt, addr,
> +                             preserve);
>         if (err) {
>                 printf("Unable to map %#llx at block 0x" LBAF ": %d\n",
>                        (unsigned long long)addr, ctx->blknr, err);
> @@ -221,7 +226,7 @@ U_BOOT_CMD_WITH_SUBCMDS(
>         "blkmap create <label> - create device\n"
>         "blkmap destroy <label> - destroy device\n"
>         "blkmap map <label> <blk#> <cnt> linear <interface> <dev> <blk#> - device mapping\n"
> -       "blkmap map <label> <blk#> <cnt> mem <addr> - memory mapping\n",
> +       "blkmap map <label> <blk#> <cnt> mem <addr> [preserve] - memory mapping\n",
>         U_BOOT_SUBCMD_MKENT(info, 2, 1, do_blkmap_common),
>         U_BOOT_SUBCMD_MKENT(part, 2, 1, do_blkmap_common),
>         U_BOOT_SUBCMD_MKENT(dev, 4, 1, do_blkmap_common),
> diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
> index 453510cca62..eefed615998 100644
> --- a/drivers/block/blkmap.c
> +++ b/drivers/block/blkmap.c
> @@ -19,6 +19,7 @@ struct blkmap;
>  /* Attributes of blkmap slice */
>  #define BLKMAP_SLICE_LINEAR    BIT(0)
>  #define BLKMAP_SLICE_MEM       BIT(1)
> +#define BLKMAP_SLICE_PRESERVE  BIT(2)
>
>  /**
>   * struct blkmap_slice - Region mapped to a blkmap
> @@ -241,7 +242,7 @@ static void blkmap_mem_destroy(struct blkmap *bm, struct blkmap_slice *bms)
>  }
>
>  int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> -                    void *addr, bool remapped)
> +                    void *addr, bool remapped, bool preserve)
>  {
>         struct blkmap *bm = dev_get_plat(dev);
>         struct blkmap_mem *bmm;
> @@ -266,6 +267,9 @@ int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>                 .remapped = remapped,
>         };
>
> +       if (preserve)
> +               bmm->slice.attr |= BLKMAP_SLICE_PRESERVE;
> +
>         err = blkmap_slice_add(bm, &bmm->slice);
>         if (err)
>                 free(bmm);
> @@ -276,11 +280,11 @@ int __blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>  int blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>                    void *addr)
>  {
> -       return __blkmap_map_mem(dev, blknr, blkcnt, addr, false);
> +       return __blkmap_map_mem(dev, blknr, blkcnt, addr, false, false);
>  }
>
>  int blkmap_map_pmem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> -                   phys_addr_t paddr)
> +                   phys_addr_t paddr, bool preserve)
>  {
>         struct blkmap *bm = dev_get_plat(dev);
>         struct blk_desc *bd = dev_get_uclass_plat(bm->blk);
> @@ -291,7 +295,7 @@ int blkmap_map_pmem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>         if (!addr)
>                 return -ENOMEM;
>
> -       err = __blkmap_map_mem(dev, blknr, blkcnt, addr, true);
> +       err = __blkmap_map_mem(dev, blknr, blkcnt, addr, true, preserve);
>         if (err)
>                 unmap_sysmem(addr);
>
> diff --git a/drivers/block/blkmap_helper.c b/drivers/block/blkmap_helper.c
> index bfba14110d2..2f1bc28ee5d 100644
> --- a/drivers/block/blkmap_helper.c
> +++ b/drivers/block/blkmap_helper.c
> @@ -28,7 +28,7 @@ int blkmap_create_ramdisk(const char *label, ulong image_addr, ulong image_size,
>         bm = dev_get_plat(bm_dev);
>         desc = dev_get_uclass_plat(bm->blk);
>         blknum = image_size >> desc->log2blksz;
> -       ret = blkmap_map_pmem(bm_dev, 0, blknum, image_addr);
> +       ret = blkmap_map_pmem(bm_dev, 0, blknum, image_addr, true);
>         if (ret) {
>                 log_err("Unable to map %#llx at block %d : %d\n",
>                         (unsigned long long)image_addr, 0, ret);
> diff --git a/include/blkmap.h b/include/blkmap.h
> index d53095437fa..754d8671b01 100644
> --- a/include/blkmap.h
> +++ b/include/blkmap.h
> @@ -60,10 +60,12 @@ int blkmap_map_mem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>   * @blknr: Start block number of the mapping
>   * @blkcnt: Number of blocks to map
>   * @paddr: The target physical memory address of the mapping
> + * @preserve: Mapping intended to be preserved for subsequent stages,
> + *             like the OS (e.g. ISO installer)
>   * Returns: 0 on success, negative error code on failure
>   */
>  int blkmap_map_pmem(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
> -                   phys_addr_t paddr);
> +                   phys_addr_t paddr, bool preserve);
>
>  /**
>   * blkmap_from_label() - Find blkmap from label
> --
> 2.34.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 6/6] blkmap: pass information on ISO image to the OS
  2025-02-27 20:00   ` Tobias Waldekranz
@ 2025-02-28  6:54     ` Sughosh Ganu
  0 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2025-02-28  6:54 UTC (permalink / raw)
  To: Tobias Waldekranz
  Cc: u-boot, Ilias Apalodimas, Simon Glass, Tom Rini,
	Heinrich Schuchardt, Anton Antonov, Bin Meng

On Fri, 28 Feb 2025 at 01:30, Tobias Waldekranz <tobias@waldekranz.com> wrote:
>
> On tor, feb 27, 2025 at 16:45, Sughosh Ganu <sughosh.ganu@linaro.org> 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@linaro.org>
> > ---
>
> If a v6 is needed for some other reason (you seemed to indicate that),
> then see my small comments below. Either way:
>
> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
>
> > Changes since V4:
> > * Reword the commit message
> > * Add a helper function blkmap_get_preserved_pmem_slice()
> > * Add a function pmem_node_efi_memmap_setup() for pmem node and EFI
> >   memmap related setup
> >
> >  boot/image-fdt.c            |  7 ++++++
> >  drivers/block/blkmap.c      | 43 +++++++++++++++++++++++++++++++++++++
> >  include/blkmap.h            | 17 +++++++++++++++
> >  include/efi.h               | 13 +++++++++++
> >  lib/efi_loader/efi_helper.c | 37 +++++++++++++++++++++++++++++++
> >  5 files changed, 117 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..c9e0a3a6eea 100644
> > --- a/drivers/block/blkmap.c
> > +++ b/drivers/block/blkmap.c
> > @@ -498,6 +498,49 @@ err:
> >       return err;
> >  }
> >
> > +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;
> > +
> > +     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) || !cb)
> > +                             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) {
> > +                             log_err("Failed to setup pmem node for addr %#lx, size %#x\n",
> > +                                     addr, size);
>
> IMO, this function should not make any assumptions about what the
> callback is doing. If an error is encountered, then the callback should
> decide if it warrants an entry in the log or not.

Yes, the right place for this log message is in the callback function.

>
> > +                             return -1;
>
> And the non-zero return value (`err`) of the callback should be passed
> verbatim back to our caller here, in case it wants to discriminate
> between different kinds of errors.

Okay, will change.

-sughosh

>
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +
> > +}
> > +
> >  int blkmap_destroy(struct udevice *dev)
> >  {
> >       int err;
> > diff --git a/include/blkmap.h b/include/blkmap.h
> > index 754d8671b01..89bd2b65fba 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
> > + * @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
> > + * 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, -1 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 04b2efc4a3b..675f3efa79a 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,39 @@ 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
> > + * 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)
> > +             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);
>
> ...i.e. here is the code that knows what the callback is doing, so it
> can choose to emit the log message - without imposing that behavior on
> any other users of the iterator.
>
> > +}
> > --
> > 2.34.1

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2025-02-28  6:54 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27 11:15 [PATCH v5 0/6] Add pmem node for preserving distro ISO's Sughosh Ganu
2025-02-27 11:15 ` [PATCH v5 1/6] fdt: add support for adding pmem nodes Sughosh Ganu
2025-02-27 11:15 ` [PATCH v5 2/6] efi_loader: add a function to remove memory from the EFI map Sughosh Ganu
2025-02-27 11:15 ` [PATCH v5 3/6] efi_loader: preserve installer images in pmem Sughosh Ganu
2025-02-27 11:15 ` [PATCH v5 4/6] blkmap: store type of blkmap slice in corresponding structure Sughosh Ganu
2025-02-27 19:49   ` Tobias Waldekranz
2025-02-28  6:08   ` Ilias Apalodimas
2025-02-27 11:15 ` [PATCH v5 5/6] blkmap: add an attribute to preserve the mem mapping Sughosh Ganu
2025-02-27 19:51   ` Tobias Waldekranz
2025-02-28  6:24   ` Ilias Apalodimas
2025-02-27 11:15 ` [PATCH v5 6/6] blkmap: pass information on ISO image to the OS Sughosh Ganu
2025-02-27 12:26   ` Sughosh Ganu
2025-02-27 20:00   ` Tobias Waldekranz
2025-02-28  6:54     ` Sughosh Ganu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.