[PATCH 0/4] Add pmem node for preserving distro ISO's

[PATCH 0/4] Add pmem node for preserving distro ISO's

[Sughosh Ganu]

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.



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 (1):
  efi: add helper functions to insert pmem node for DT fixup

 boot/fdt_support.c           | 41 +++++++++++++++++++++++++++--
 boot/image-fdt.c             |  9 +++++++
 include/efi_loader.h         | 28 +++++++++++++++++---
 include/fdt_support.h        | 13 +++++++++
 lib/efi_loader/efi_bootmgr.c | 43 ++++++++++++++++++++++++++----
 lib/efi_loader/efi_helper.c  | 12 +++++++++
 lib/efi_loader/efi_memory.c  | 51 +++++++++++++++++++++++++++---------
 lib/lmb.c                    |  4 +--
 8 files changed, 175 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH 1/4] fdt: add support for adding pmem nodes

[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>
---
 boot/fdt_support.c    | 41 +++++++++++++++++++++++++++++++++++++++--
 include/fdt_support.h | 13 +++++++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/boot/fdt_support.c b/boot/fdt_support.c
index 2392027d40..61f725389b 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>
@@ -463,7 +464,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
  */
@@ -491,7 +491,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
@@ -2221,3 +2221,40 @@ 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 needs at 2MB alignment\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 741e2360c2..63ece0de32 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -473,4 +473,17 @@ 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 injects 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

Re: [PATCH 1/4] fdt: add support for adding pmem nodes

[Ilias Apalodimas]

Thanks Sughosh

+CC Anton who tested that

On Fri, 25 Oct 2024 at 14:14, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> 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>
> ---
>  boot/fdt_support.c    | 41 +++++++++++++++++++++++++++++++++++++++--
>  include/fdt_support.h | 13 +++++++++++++
>  2 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/boot/fdt_support.c b/boot/fdt_support.c
> index 2392027d40..61f725389b 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>
> @@ -463,7 +464,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
>   */
> @@ -491,7 +491,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
> @@ -2221,3 +2221,40 @@ 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 needs at 2MB alignment\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 741e2360c2..63ece0de32 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -473,4 +473,17 @@ 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 injects 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
>

Re: [PATCH 1/4] fdt: add support for adding pmem nodes

[Simon Glass]

Hi Sughosh,

On Fri, 25 Oct 2024 at 13:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> 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>
> ---
>  boot/fdt_support.c    | 41 +++++++++++++++++++++++++++++++++++++++--
>  include/fdt_support.h | 13 +++++++++++++
>  2 files changed, 52 insertions(+), 2 deletions(-)

Can you please use the ofnode interface for this? We are trying not to
add more of this kind of fixup, as it means someone will have to port
it over later.

See EVT_FT_FIXUP

Regards,
Simon

Re: [PATCH 1/4] fdt: add support for adding pmem nodes

[Sughosh Ganu]

On Sun, 27 Oct 2024 at 20:30, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 25 Oct 2024 at 13:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > 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>
> > ---
> >  boot/fdt_support.c    | 41 +++++++++++++++++++++++++++++++++++++++--
> >  include/fdt_support.h | 13 +++++++++++++
> >  2 files changed, 52 insertions(+), 2 deletions(-)
>
> Can you please use the ofnode interface for this? We are trying not to
> add more of this kind of fixup, as it means someone will have to port
> it over later.
>
> See EVT_FT_FIXUP

Are you suggesting using the EVT_FT_FIXUP interface for adding the
pmem node? If so, how does this work on platforms which enable
livetree? Looking at the code, I see that the EVT_FT_FIXUP gets called
only when the livetree is not active. However, I am not able to find
any place where the livetree is getting disabled. We do have a
function, oftree_to_fdt() which flattens a livetree, but I don't see
it getting called from anywhere except from a 'upl' command. The
of_root pointer does get set in initr_of_live(), but apart from
dm_test_pre_run(), I do not see where we disable the livetree and set
of_root to NULL.

So on platforms which do enable livetree, how does this fixup work?
What am I missing?

-sughosh

>
> Regards,
> Simon

Re: [PATCH 1/4] fdt: add support for adding pmem nodes

[Simon Glass]

Hi Sughosh,

On Mon, 28 Oct 2024 at 12:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Sun, 27 Oct 2024 at 20:30, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 25 Oct 2024 at 13:15, Sughosh Ganu <sughosh.ganu@linaro.org>
wrote:
> > >
> > > 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>
> > > ---
> > >  boot/fdt_support.c    | 41 +++++++++++++++++++++++++++++++++++++++--
> > >  include/fdt_support.h | 13 +++++++++++++
> > >  2 files changed, 52 insertions(+), 2 deletions(-)
> >
> > Can you please use the ofnode interface for this? We are trying not to
> > add more of this kind of fixup, as it means someone will have to port
> > it over later.
> >
> > See EVT_FT_FIXUP
>
> Are you suggesting using the EVT_FT_FIXUP interface for adding the
> pmem node? If so, how does this work on platforms which enable
> livetree? Looking at the code, I see that the EVT_FT_FIXUP gets called
> only when the livetree is not active. However, I am not able to find
> any place where the livetree is getting disabled. We do have a
> function, oftree_to_fdt() which flattens a livetree, but I don't see
> it getting called from anywhere except from a 'upl' command. The
> of_root pointer does get set in initr_of_live(), but apart from
> dm_test_pre_run(), I do not see where we disable the livetree and set
> of_root to NULL.
>
> So on platforms which do enable livetree, how does this fixup work?
> What am I missing?

It isn't implemented yet for OF_LIVE in image_setup_libfdt(). Most of the
code is there though. It just needs to be hooked up - see oftree_to_fdt()
and oftree_from_fdt().

The idea would be to remove the of_live_active() check, so the event is
always sent (will need to 'select EVENT' for OF_LIVE).

Then in that same code, after the event, call oftree_to_fdt() and copy the
fdt back to blob.

If you have time, it should be a nice mini project and will allow us to
migrate more things to the ofnode API.

Regards,
Simon

Re: [PATCH 1/4] fdt: add support for adding pmem nodes

[Sughosh Ganu]

On Mon, 28 Oct 2024 at 22:32, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 28 Oct 2024 at 12:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Sun, 27 Oct 2024 at 20:30, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Fri, 25 Oct 2024 at 13:15, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > 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>
> > > > ---
> > > >  boot/fdt_support.c    | 41 +++++++++++++++++++++++++++++++++++++++--
> > > >  include/fdt_support.h | 13 +++++++++++++
> > > >  2 files changed, 52 insertions(+), 2 deletions(-)
> > >
> > > Can you please use the ofnode interface for this? We are trying not to
> > > add more of this kind of fixup, as it means someone will have to port
> > > it over later.
> > >
> > > See EVT_FT_FIXUP
> >
> > Are you suggesting using the EVT_FT_FIXUP interface for adding the
> > pmem node? If so, how does this work on platforms which enable
> > livetree? Looking at the code, I see that the EVT_FT_FIXUP gets called
> > only when the livetree is not active. However, I am not able to find
> > any place where the livetree is getting disabled. We do have a
> > function, oftree_to_fdt() which flattens a livetree, but I don't see
> > it getting called from anywhere except from a 'upl' command. The
> > of_root pointer does get set in initr_of_live(), but apart from
> > dm_test_pre_run(), I do not see where we disable the livetree and set
> > of_root to NULL.
> >
> > So on platforms which do enable livetree, how does this fixup work?
> > What am I missing?
>
> It isn't implemented yet for OF_LIVE in image_setup_libfdt(). Most of the code is there though. It just needs to be hooked up - see oftree_to_fdt() and oftree_from_fdt().
>
> The idea would be to remove the of_live_active() check, so the event is always sent (will need to 'select EVENT' for OF_LIVE).
>
> Then in that same code, after the event, call oftree_to_fdt() and copy the fdt back to blob.

Sorry, I got pulled into other tasks, so could not reply to this
earlier. Looking at the code, I don't think that using livetree is a
good use-case in this context. What is happening here is that the
image_setup_libfdt() function gets called for fixing up the devicetree
before the OS is called. However, in this case, we cannot assume that
the FDT that is being passed to the OS is the same as the livetree.
What that means is that it would be required to first convert the FDT
to a livetree, apply the fixup's and then again convert the livetree
back to FDT. I can understand why you would want to convert a FDT to a
livetree during init, and then use the livetree for driver model init.
But why do we need this transformation to and back from livetree in
this context, only for adding a node to the FDT.

-sughosh

>
> If you have time, it should be a nice mini project and will allow us to migrate more things to the ofnode API.
>
> Regards,
> Simon

[PATCH 2/4] efi_loader: add a function to remove memory from the EFI map

[Sughosh Ganu]

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

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

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/efi_loader.h        | 11 +++++---
 lib/efi_loader/efi_memory.c | 51 +++++++++++++++++++++++++++----------
 lib/lmb.c                   |  4 +--
 3 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 291eca5c07..d450e304c6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -786,7 +786,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
 
 /**
- * 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
@@ -794,11 +794,14 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
  * @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_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);
+
+/* 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);
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 3d742fa191..cb93bfa55f 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,11 +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
  */
-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;
@@ -278,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;
@@ -363,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();
@@ -400,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);
 }
 
 /**
@@ -500,7 +525,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 
 	addr = (u64)(uintptr_t)map_sysmem(addr, 0);
 	/* Reserve that map in our memory maps */
-	ret = efi_add_memory_map_pg(addr, pages, memory_type, true);
+	ret = efi_update_memory_map(addr, pages, memory_type, true, false);
 	if (ret != EFI_SUCCESS)
 		/* Map would overlap, bail out */
 		return  EFI_OUT_OF_RESOURCES;
@@ -542,8 +567,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
 	if (status)
 		return EFI_NOT_FOUND;
 
-	ret = efi_add_memory_map_pg(memory, pages, EFI_CONVENTIONAL_MEMORY,
-				    false);
+	ret = efi_update_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY,
+				    false, false);
 	if (ret != EFI_SUCCESS)
 		return EFI_NOT_FOUND;
 
@@ -828,8 +853,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)
diff --git a/lib/lmb.c b/lib/lmb.c
index 7e90f17876..05f3cd093d 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -55,11 +55,11 @@ static int __maybe_unused lmb_map_update_notify(phys_addr_t addr,
 	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 == MAP_OP_RESERVE ?
 				       EFI_BOOT_SERVICES_DATA :
 				       EFI_CONVENTIONAL_MEMORY,
-				       false);
+				       false, false);
 	if (status != EFI_SUCCESS) {
 		log_err("%s: LMB Map notify failure %lu\n", __func__,
 			status & ~EFI_ERROR_MASK);
-- 
2.34.1

Re: [PATCH 2/4] efi_loader: add a function to remove memory from the EFI map

[Ilias Apalodimas]

+CC Anton

On Fri, 25 Oct 2024 at 14:14, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> With upcoming changes supporting pmem nodes, we need to ommit the
> pmem area rfom the EFI memory map. Add a function to do that
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/efi_loader.h        | 11 +++++---
>  lib/efi_loader/efi_memory.c | 51 +++++++++++++++++++++++++++----------
>  lib/lmb.c                   |  4 +--
>  3 files changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 291eca5c07..d450e304c6 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -786,7 +786,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>  efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
>
>  /**
> - * 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
> @@ -794,11 +794,14 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
>   * @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_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);
> +
> +/* 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);
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 3d742fa191..cb93bfa55f 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,11 +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
>   */
> -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;
> @@ -278,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;
> @@ -363,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();
> @@ -400,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);
>  }
>
>  /**
> @@ -500,7 +525,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>
>         addr = (u64)(uintptr_t)map_sysmem(addr, 0);
>         /* Reserve that map in our memory maps */
> -       ret = efi_add_memory_map_pg(addr, pages, memory_type, true);
> +       ret = efi_update_memory_map(addr, pages, memory_type, true, false);
>         if (ret != EFI_SUCCESS)
>                 /* Map would overlap, bail out */
>                 return  EFI_OUT_OF_RESOURCES;
> @@ -542,8 +567,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>         if (status)
>                 return EFI_NOT_FOUND;
>
> -       ret = efi_add_memory_map_pg(memory, pages, EFI_CONVENTIONAL_MEMORY,
> -                                   false);
> +       ret = efi_update_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY,
> +                                   false, false);
>         if (ret != EFI_SUCCESS)
>                 return EFI_NOT_FOUND;
>
> @@ -828,8 +853,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)
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 7e90f17876..05f3cd093d 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -55,11 +55,11 @@ static int __maybe_unused lmb_map_update_notify(phys_addr_t addr,
>         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 == MAP_OP_RESERVE ?
>                                        EFI_BOOT_SERVICES_DATA :
>                                        EFI_CONVENTIONAL_MEMORY,
> -                                      false);
> +                                      false, false);
>         if (status != EFI_SUCCESS) {
>                 log_err("%s: LMB Map notify failure %lu\n", __func__,
>                         status & ~EFI_ERROR_MASK);
> --
> 2.34.1
>

Re: [PATCH 2/4] efi_loader: add a function to remove memory from the EFI map

[Heinrich Schuchardt]

On 10/25/24 13:14, Sughosh Ganu wrote:
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> With upcoming changes supporting pmem nodes, we need to ommit the

Nits:
%s/ommit/remove/

> pmem area rfom the EFI memory map. Add a function to do that

%s/rfom/from/
%s/that/that./

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

The patch is not applicable. Please, rebase on origin/master.

In the medium term we should get rid of the double book keeping. The
memory map should be generated on the fly from LMB data.

Best regards

Heinrich

> ---
>   include/efi_loader.h        | 11 +++++---
>   lib/efi_loader/efi_memory.c | 51 +++++++++++++++++++++++++++----------
>   lib/lmb.c                   |  4 +--
>   3 files changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 291eca5c07..d450e304c6 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -786,7 +786,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>   efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
>
>   /**
> - * 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
> @@ -794,11 +794,14 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
>    * @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_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);
> +
> +/* 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);
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 3d742fa191..cb93bfa55f 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,11 +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
>    */
> -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;
> @@ -278,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;
> @@ -363,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();
> @@ -400,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);
>   }
>
>   /**
> @@ -500,7 +525,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>
>   	addr = (u64)(uintptr_t)map_sysmem(addr, 0);
>   	/* Reserve that map in our memory maps */
> -	ret = efi_add_memory_map_pg(addr, pages, memory_type, true);
> +	ret = efi_update_memory_map(addr, pages, memory_type, true, false);
>   	if (ret != EFI_SUCCESS)
>   		/* Map would overlap, bail out */
>   		return  EFI_OUT_OF_RESOURCES;
> @@ -542,8 +567,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>   	if (status)
>   		return EFI_NOT_FOUND;
>
> -	ret = efi_add_memory_map_pg(memory, pages, EFI_CONVENTIONAL_MEMORY,
> -				    false);
> +	ret = efi_update_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY,
> +				    false, false);
>   	if (ret != EFI_SUCCESS)
>   		return EFI_NOT_FOUND;
>
> @@ -828,8 +853,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)
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 7e90f17876..05f3cd093d 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -55,11 +55,11 @@ static int __maybe_unused lmb_map_update_notify(phys_addr_t addr,
>   	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 == MAP_OP_RESERVE ?
>   				       EFI_BOOT_SERVICES_DATA :
>   				       EFI_CONVENTIONAL_MEMORY,
> -				       false);
> +				       false, false);
>   	if (status != EFI_SUCCESS) {
>   		log_err("%s: LMB Map notify failure %lu\n", __func__,
>   			status & ~EFI_ERROR_MASK);

Re: [PATCH 2/4] efi_loader: add a function to remove memory from the EFI map

[Ilias Apalodimas]

Hi Heinrich

On Mon, 11 Nov 2024 at 09:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/24 13:14, Sughosh Ganu wrote:
> > From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
> > With upcoming changes supporting pmem nodes, we need to ommit the
>
> Nits:
> %s/ommit/remove/
>
> > pmem area rfom the EFI memory map. Add a function to do that
>
> %s/rfom/from/
> %s/that/that./
>
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> The patch is not applicable. Please, rebase on origin/master.

Will do thanks!

>
> In the medium term we should get rid of the double book keeping. The
> memory map should be generated on the fly from LMB data.

Yes, I plan to have an RFC so we can see how that looks like

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> > ---
> >   include/efi_loader.h        | 11 +++++---
> >   lib/efi_loader/efi_memory.c | 51 +++++++++++++++++++++++++++----------
> >   lib/lmb.c                   |  4 +--
> >   3 files changed, 47 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 291eca5c07..d450e304c6 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -786,7 +786,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
> >   efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
> >
> >   /**
> > - * 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
> > @@ -794,11 +794,14 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
> >    * @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_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);
> > +
> > +/* 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);
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index 3d742fa191..cb93bfa55f 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,11 +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
> >    */
> > -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;
> > @@ -278,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;
> > @@ -363,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();
> > @@ -400,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);
> >   }
> >
> >   /**
> > @@ -500,7 +525,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> >
> >       addr = (u64)(uintptr_t)map_sysmem(addr, 0);
> >       /* Reserve that map in our memory maps */
> > -     ret = efi_add_memory_map_pg(addr, pages, memory_type, true);
> > +     ret = efi_update_memory_map(addr, pages, memory_type, true, false);
> >       if (ret != EFI_SUCCESS)
> >               /* Map would overlap, bail out */
> >               return  EFI_OUT_OF_RESOURCES;
> > @@ -542,8 +567,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
> >       if (status)
> >               return EFI_NOT_FOUND;
> >
> > -     ret = efi_add_memory_map_pg(memory, pages, EFI_CONVENTIONAL_MEMORY,
> > -                                 false);
> > +     ret = efi_update_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY,
> > +                                 false, false);
> >       if (ret != EFI_SUCCESS)
> >               return EFI_NOT_FOUND;
> >
> > @@ -828,8 +853,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)
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index 7e90f17876..05f3cd093d 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -55,11 +55,11 @@ static int __maybe_unused lmb_map_update_notify(phys_addr_t addr,
> >       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 == MAP_OP_RESERVE ?
> >                                      EFI_BOOT_SERVICES_DATA :
> >                                      EFI_CONVENTIONAL_MEMORY,
> > -                                    false);
> > +                                    false, false);
> >       if (status != EFI_SUCCESS) {
> >               log_err("%s: LMB Map notify failure %lu\n", __func__,
> >                       status & ~EFI_ERROR_MASK);
>

[PATCH 3/4] efi_loader: preserve installer images in pmem

[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>
---
 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 a3aa2b8d1b..16f75555f6 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;
@@ -358,13 +360,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;
 	}
 
@@ -486,6 +491,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

Re: [PATCH 3/4] efi_loader: preserve installer images in pmem

[Ilias Apalodimas]

+CC Anton

On Fri, 25 Oct 2024 at 14:14, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> 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>
> ---
>  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 a3aa2b8d1b..16f75555f6 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;
> @@ -358,13 +360,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;
>         }
>
> @@ -486,6 +491,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
>

Re: [PATCH 3/4] efi_loader: preserve installer images in pmem

[Heinrich Schuchardt]

On 10/25/24 13:14, Sughosh Ganu wrote:
> 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>
> ---
>   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 a3aa2b8d1b..16f75555f6 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;
> @@ -358,13 +360,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;
>   	}
>
> @@ -486,6 +491,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);

The code regarding .iso and .img handling seems to be misplaced. Why
should we treat a file loaded from a block device differently to a file
loaded from the network?

Best regards

Heinrich

>
>   	/*
>   	 * If the file extension is ".iso" or ".img", mount it and try to load

Re: [PATCH 3/4] efi_loader: preserve installer images in pmem

[Ilias Apalodimas]

On Mon, 11 Nov 2024 at 10:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/24 13:14, Sughosh Ganu wrote:
> > 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>
> > ---
> >   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 a3aa2b8d1b..16f75555f6 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;
> > @@ -358,13 +360,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;
> >       }
> >
> > @@ -486,6 +491,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);
>
> The code regarding .iso and .img handling seems to be misplaced. Why
> should we treat a file loaded from a block device differently to a file
> loaded from the network?

If the installer is located in a block device, the kernel will find
the image when it scans those devices during boot. It's only when we
have it on a memory area that we need to preserve it

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> >       /*
> >        * If the file extension is ".iso" or ".img", mount it and try to load
>

Re: [PATCH 3/4] efi_loader: preserve installer images in pmem

[Heinrich Schuchardt]

On 11/11/24 11:46, Ilias Apalodimas wrote:
> On Mon, 11 Nov 2024 at 10:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/25/24 13:14, Sughosh Ganu wrote:
>>> 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>
>>> ---
>>>    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 a3aa2b8d1b..16f75555f6 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;
>>> @@ -358,13 +360,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;
>>>        }
>>>
>>> @@ -486,6 +491,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);
>>
>> The code regarding .iso and .img handling seems to be misplaced. Why
>> should we treat a file loaded from a block device differently to a file
>> loaded from the network?
>
> If the installer is located in a block device, the kernel will find
> the image when it scans those devices during boot. It's only when we
> have it on a memory area that we need to preserve it

I meant we could have a block device with an '*.iso' file on it, which
we load into memory.

Simon has been working on keeping track of loaded images. Possibly
besides EFI binaries we can keep track of loaded ISOs in the same way in
future.

For the moment being let't use the patch as is.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>
> Thanks
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>        /*
>>>         * If the file extension is ".iso" or ".img", mount it and try to load
>>

[PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Sughosh Ganu]

The EFI HTTP boot puts the iso installer image at some location in
memory which needs to be reserved in the devicetree as persistent
memory (pmem). Add helper functions which add this pmem node when the
EFI_DT_FIXUP protocol's fixup callback is invoked.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 boot/image-fdt.c             |  9 +++++++++
 include/efi_loader.h         | 17 +++++++++++++++++
 lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
 lib/efi_loader/efi_helper.c  | 12 ++++++++++++
 4 files changed, 59 insertions(+)

diff --git a/boot/image-fdt.c b/boot/image-fdt.c
index 8eda521693..b39e81ad30 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_loader.h>
 #include <env.h>
 #include <errno.h>
 #include <image.h>
@@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
 	if (!ft_verify_fdt(blob))
 		goto err;
 
+	if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
+		fdt_ret = fdt_efi_pmem_setup(blob);
+		if (fdt_ret) {
+			printf("ERROR: HTTP boot pmem fixup failed\n");
+			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/include/efi_loader.h b/include/efi_loader.h
index d450e304c6..031de18746 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
 efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
 				    efi_guid_t *guid);
 
+/**
+ * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
+ *
+ * @fdt:	Pointer to the devicetree
+ *
+ * Return:	0 on success, negative on failure
+ */
+int fdt_efi_pmem_setup(void *fdt);
+
 /**
  * efi_size_in_pages() - convert size in bytes to size in pages
  *
@@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
 				  void *load_options);
 efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
 
+/**
+ * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
+ *
+ * @fdt:	Pointer to the DT blob
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_pmem_setup(void *fdt);
+
 /**
  * struct efi_image_regions - A list of memory regions
  *
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 16f75555f6..1d9246be61 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -41,6 +41,8 @@ struct uridp_context {
 	efi_handle_t mem_handle;
 };
 
+static struct uridp_context *uctx;
+
 const efi_guid_t efi_guid_bootmenu_auto_generated =
 		EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
 
@@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
 
 	efi_free_pool(ctx->loaded_dp);
 	free(ctx);
+	uctx = NULL;
 
 	return ret == EFI_SUCCESS ? ret2 : ret;
 }
@@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
 	EFI_EXIT(ret);
 }
 
+/**
+ * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
+ *
+ * @fdt:	Pointer to the DT blob
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_pmem_setup(void *fdt)
+{
+	if (!uctx) {
+		log_warning("No EFI HTTP boot context found\n");
+		return EFI_SUCCESS;
+	}
+
+	return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
+		EFI_SUCCESS : EFI_INVALID_PARAMETER;
+}
+
 /**
  * try_load_from_uri_path() - Handle the URI device path
  *
@@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 	if (!ctx)
 		return EFI_OUT_OF_RESOURCES;
 
+	uctx = ctx;
 	s = env_get("loadaddr");
 	if (!s) {
 		log_err("Error: loadaddr is not set\n");
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index 00167bd2a1..33cd8b9a50 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
 	return 0;
 }
 
+/**
+ * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
+ *
+ * @fdt:	Pointer to the devicetree
+ *
+ * Return:	0 on success, negative on failure
+ */
+int fdt_efi_pmem_setup(void *fdt)
+{
+	return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
+}
+
 static int u16_tohex(u16 c)
 {
 	if (c >= '0' && c <= '9')
-- 
2.34.1

Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Ilias Apalodimas]

+CC Anton

On Fri, 25 Oct 2024 at 14:14, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The EFI HTTP boot puts the iso installer image at some location in
> memory which needs to be reserved in the devicetree as persistent
> memory (pmem). Add helper functions which add this pmem node when the
> EFI_DT_FIXUP protocol's fixup callback is invoked.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  boot/image-fdt.c             |  9 +++++++++
>  include/efi_loader.h         | 17 +++++++++++++++++
>  lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>  lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>  4 files changed, 59 insertions(+)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 8eda521693..b39e81ad30 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_loader.h>
>  #include <env.h>
>  #include <errno.h>
>  #include <image.h>
> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>         if (!ft_verify_fdt(blob))
>                 goto err;
>
> +       if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> +               fdt_ret = fdt_efi_pmem_setup(blob);
> +               if (fdt_ret) {
> +                       printf("ERROR: HTTP boot pmem fixup failed\n");
> +                       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/include/efi_loader.h b/include/efi_loader.h
> index d450e304c6..031de18746 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>  efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>                                     efi_guid_t *guid);
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:       Pointer to the devicetree
> + *
> + * Return:     0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt);
> +
>  /**
>   * efi_size_in_pages() - convert size in bytes to size in pages
>   *
> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>                                   void *load_options);
>  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:       Pointer to the DT blob
> + * Return:     status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> +
>  /**
>   * struct efi_image_regions - A list of memory regions
>   *
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 16f75555f6..1d9246be61 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -41,6 +41,8 @@ struct uridp_context {
>         efi_handle_t mem_handle;
>  };
>
> +static struct uridp_context *uctx;
> +
>  const efi_guid_t efi_guid_bootmenu_auto_generated =
>                 EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>
> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>
>         efi_free_pool(ctx->loaded_dp);
>         free(ctx);
> +       uctx = NULL;
>
>         return ret == EFI_SUCCESS ? ret2 : ret;
>  }
> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>         EFI_EXIT(ret);
>  }
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:       Pointer to the DT blob
> + * Return:     status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> +{
> +       if (!uctx) {
> +               log_warning("No EFI HTTP boot context found\n");
> +               return EFI_SUCCESS;
> +       }
> +
> +       return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> +               EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +}
> +
>  /**
>   * try_load_from_uri_path() - Handle the URI device path
>   *
> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>         if (!ctx)
>                 return EFI_OUT_OF_RESOURCES;
>
> +       uctx = ctx;
>         s = env_get("loadaddr");
>         if (!s) {
>                 log_err("Error: loadaddr is not set\n");
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 00167bd2a1..33cd8b9a50 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>         return 0;
>  }
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:       Pointer to the devicetree
> + *
> + * Return:     0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt)
> +{
> +       return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> +}
> +
>  static int u16_tohex(u16 c)
>  {
>         if (c >= '0' && c <= '9')
> --
> 2.34.1
>

Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Heinrich Schuchardt]

On 10/25/24 13:14, Sughosh Ganu wrote:
> The EFI HTTP boot puts the iso installer image at some location in
> memory which needs to be reserved in the devicetree as persistent
> memory (pmem). Add helper functions which add this pmem node when the
> EFI_DT_FIXUP protocol's fixup callback is invoked.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   boot/image-fdt.c             |  9 +++++++++
>   include/efi_loader.h         | 17 +++++++++++++++++
>   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>   4 files changed, 59 insertions(+)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 8eda521693..b39e81ad30 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_loader.h>
>   #include <env.h>
>   #include <errno.h>
>   #include <image.h>
> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>   	if (!ft_verify_fdt(blob))
>   		goto err;
>
> +	if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> +		fdt_ret = fdt_efi_pmem_setup(blob);

Having EFI_HTTP_BOOT is not the only way to load an image to memory. You
could have downloaded it via TFTP or taken it from a block device. So
this config check looks wrong. Why don't you check for the presence of
memory block devices here?

You need to check if the board is using ACPI, if yes, provide the ACPI
table.

Best regards

Heinrich

> +		if (fdt_ret) {
> +			printf("ERROR: HTTP boot pmem fixup failed\n");
> +			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/include/efi_loader.h b/include/efi_loader.h
> index d450e304c6..031de18746 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>   				    efi_guid_t *guid);
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt);
> +
>   /**
>    * efi_size_in_pages() - convert size in bytes to size in pages
>    *
> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>   				  void *load_options);
>   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> +
>   /**
>    * struct efi_image_regions - A list of memory regions
>    *
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 16f75555f6..1d9246be61 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -41,6 +41,8 @@ struct uridp_context {
>   	efi_handle_t mem_handle;
>   };
>
> +static struct uridp_context *uctx;
> +
>   const efi_guid_t efi_guid_bootmenu_auto_generated =
>   		EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>
> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>
>   	efi_free_pool(ctx->loaded_dp);
>   	free(ctx);
> +	uctx = NULL;
>
>   	return ret == EFI_SUCCESS ? ret2 : ret;
>   }
> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>   	EFI_EXIT(ret);
>   }
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> +{
> +	if (!uctx) {
> +		log_warning("No EFI HTTP boot context found\n");
> +		return EFI_SUCCESS;
> +	}
> +
> +	return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> +		EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +}
> +
>   /**
>    * try_load_from_uri_path() - Handle the URI device path
>    *
> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>   	if (!ctx)
>   		return EFI_OUT_OF_RESOURCES;
>
> +	uctx = ctx;
>   	s = env_get("loadaddr");
>   	if (!s) {
>   		log_err("Error: loadaddr is not set\n");
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 00167bd2a1..33cd8b9a50 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>   	return 0;
>   }
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt)
> +{
> +	return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> +}
> +
>   static int u16_tohex(u16 c)
>   {
>   	if (c >= '0' && c <= '9')

Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Ilias Apalodimas]

Hi Heinrich

On Mon, 28 Oct 2024 at 08:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/24 13:14, Sughosh Ganu wrote:
> > The EFI HTTP boot puts the iso installer image at some location in
> > memory which needs to be reserved in the devicetree as persistent
> > memory (pmem). Add helper functions which add this pmem node when the
> > EFI_DT_FIXUP protocol's fixup callback is invoked.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   boot/image-fdt.c             |  9 +++++++++
> >   include/efi_loader.h         | 17 +++++++++++++++++
> >   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
> >   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
> >   4 files changed, 59 insertions(+)
> >
> > diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> > index 8eda521693..b39e81ad30 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_loader.h>
> >   #include <env.h>
> >   #include <errno.h>
> >   #include <image.h>
> > @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
> >       if (!ft_verify_fdt(blob))
> >               goto err;
> >
> > +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> > +             fdt_ret = fdt_efi_pmem_setup(blob);
>
> Having EFI_HTTP_BOOT is not the only way to load an image to memory. You
> could have downloaded it via TFTP or taken it from a block device. So
> this config check looks wrong. Why don't you check for the presence of
> memory block devices here?

If it's an existing block device (e.g a USB), there's no need to
preserve the .iso. The installer will scan the disks and find it
again.
TFTP might make sense, but this only applies to booting EFI images, so
scanning the memory block devices is not enough.

Sughosh can you take a look and see if scanning memory blocks is doable?

>
> You need to check if the board is using ACPI, if yes, provide the ACPI
> table.

The ACPI table is out of scope on these patches. We need to add a
check and only do the fixup if we are booting with a DT, but we dont
currently plan on fixing NFIT ACPI tables.

Thanks
/Ilias

> Best regards
>
> Heinrich
>
> > +             if (fdt_ret) {
> > +                     printf("ERROR: HTTP boot pmem fixup failed\n");
> > +                     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/include/efi_loader.h b/include/efi_loader.h
> > index d450e304c6..031de18746 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
> >   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >                                   efi_guid_t *guid);
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt);
> > +
> >   /**
> >    * efi_size_in_pages() - convert size in bytes to size in pages
> >    *
> > @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> >                                 void *load_options);
> >   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> > +
> >   /**
> >    * struct efi_image_regions - A list of memory regions
> >    *
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 16f75555f6..1d9246be61 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -41,6 +41,8 @@ struct uridp_context {
> >       efi_handle_t mem_handle;
> >   };
> >
> > +static struct uridp_context *uctx;
> > +
> >   const efi_guid_t efi_guid_bootmenu_auto_generated =
> >               EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
> >
> > @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
> >
> >       efi_free_pool(ctx->loaded_dp);
> >       free(ctx);
> > +     uctx = NULL;
> >
> >       return ret == EFI_SUCCESS ? ret2 : ret;
> >   }
> > @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
> >       EFI_EXIT(ret);
> >   }
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> > +{
> > +     if (!uctx) {
> > +             log_warning("No EFI HTTP boot context found\n");
> > +             return EFI_SUCCESS;
> > +     }
> > +
> > +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> > +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
> > +}
> > +
> >   /**
> >    * try_load_from_uri_path() - Handle the URI device path
> >    *
> > @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >       if (!ctx)
> >               return EFI_OUT_OF_RESOURCES;
> >
> > +     uctx = ctx;
> >       s = env_get("loadaddr");
> >       if (!s) {
> >               log_err("Error: loadaddr is not set\n");
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index 00167bd2a1..33cd8b9a50 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
> >       return 0;
> >   }
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt)
> > +{
> > +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> > +}
> > +
> >   static int u16_tohex(u16 c)
> >   {
> >       if (c >= '0' && c <= '9')
>

Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Heinrich Schuchardt]

On 10/25/24 13:14, Sughosh Ganu wrote:
> The EFI HTTP boot puts the iso installer image at some location in
> memory which needs to be reserved in the devicetree as persistent
> memory (pmem). Add helper functions which add this pmem node when the
> EFI_DT_FIXUP protocol's fixup callback is invoked.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   boot/image-fdt.c             |  9 +++++++++
>   include/efi_loader.h         | 17 +++++++++++++++++
>   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>   4 files changed, 59 insertions(+)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 8eda521693..b39e81ad30 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_loader.h>
>   #include <env.h>
>   #include <errno.h>
>   #include <image.h>
> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>   	if (!ft_verify_fdt(blob))
>   		goto err;
>
> +	if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> +		fdt_ret = fdt_efi_pmem_setup(blob);

I can see no reason why pmem setup should depend on HTTP boot.

It should be possible to pass a memory block device to the kernel no
matter how it was created.

Best regards

Heinrich

> +		if (fdt_ret) {
> +			printf("ERROR: HTTP boot pmem fixup failed\n");
> +			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/include/efi_loader.h b/include/efi_loader.h
> index d450e304c6..031de18746 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>   				    efi_guid_t *guid);
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt);
> +
>   /**
>    * efi_size_in_pages() - convert size in bytes to size in pages
>    *
> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>   				  void *load_options);
>   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> +
>   /**
>    * struct efi_image_regions - A list of memory regions
>    *
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 16f75555f6..1d9246be61 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -41,6 +41,8 @@ struct uridp_context {
>   	efi_handle_t mem_handle;
>   };
>
> +static struct uridp_context *uctx;
> +
>   const efi_guid_t efi_guid_bootmenu_auto_generated =
>   		EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>
> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>
>   	efi_free_pool(ctx->loaded_dp);
>   	free(ctx);
> +	uctx = NULL;
>
>   	return ret == EFI_SUCCESS ? ret2 : ret;
>   }
> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>   	EFI_EXIT(ret);
>   }
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> +{
> +	if (!uctx) {
> +		log_warning("No EFI HTTP boot context found\n");
> +		return EFI_SUCCESS;
> +	}
> +
> +	return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> +		EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +}
> +
>   /**
>    * try_load_from_uri_path() - Handle the URI device path
>    *
> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>   	if (!ctx)
>   		return EFI_OUT_OF_RESOURCES;
>
> +	uctx = ctx;
>   	s = env_get("loadaddr");
>   	if (!s) {
>   		log_err("Error: loadaddr is not set\n");
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 00167bd2a1..33cd8b9a50 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>   	return 0;
>   }
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt)
> +{
> +	return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> +}
> +
>   static int u16_tohex(u16 c)
>   {
>   	if (c >= '0' && c <= '9')

Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Ilias Apalodimas]

Hi Heinrich

On Mon, 11 Nov 2024 at 10:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/24 13:14, Sughosh Ganu wrote:
> > The EFI HTTP boot puts the iso installer image at some location in
> > memory which needs to be reserved in the devicetree as persistent
> > memory (pmem). Add helper functions which add this pmem node when the
> > EFI_DT_FIXUP protocol's fixup callback is invoked.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   boot/image-fdt.c             |  9 +++++++++
> >   include/efi_loader.h         | 17 +++++++++++++++++
> >   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
> >   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
> >   4 files changed, 59 insertions(+)
> >
> > diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> > index 8eda521693..b39e81ad30 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_loader.h>
> >   #include <env.h>
> >   #include <errno.h>
> >   #include <image.h>
> > @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
> >       if (!ft_verify_fdt(blob))
> >               goto err;
> >
> > +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> > +             fdt_ret = fdt_efi_pmem_setup(blob);
>
> I can see no reason why pmem setup should depend on HTTP boot.

The reason is that we *know* we want to preserve image on HTTP
installers. but we should only add it if that image is booted, not
unconditionally if EFI_HTTP is enabled

>
> It should be possible to pass a memory block device to the kernel no
> matter how it was created.

Yes, we can. But how do we know we need to setup a pmem node? In this
specific case, we know http installers need the image.
If we can find similar rules (or perhaps a command line option), we
can preserve it for all images

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> > +             if (fdt_ret) {
> > +                     printf("ERROR: HTTP boot pmem fixup failed\n");
> > +                     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/include/efi_loader.h b/include/efi_loader.h
> > index d450e304c6..031de18746 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
> >   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >                                   efi_guid_t *guid);
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt);
> > +
> >   /**
> >    * efi_size_in_pages() - convert size in bytes to size in pages
> >    *
> > @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> >                                 void *load_options);
> >   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> > +
> >   /**
> >    * struct efi_image_regions - A list of memory regions
> >    *
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 16f75555f6..1d9246be61 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -41,6 +41,8 @@ struct uridp_context {
> >       efi_handle_t mem_handle;
> >   };
> >
> > +static struct uridp_context *uctx;
> > +
> >   const efi_guid_t efi_guid_bootmenu_auto_generated =
> >               EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
> >
> > @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
> >
> >       efi_free_pool(ctx->loaded_dp);
> >       free(ctx);
> > +     uctx = NULL;
> >
> >       return ret == EFI_SUCCESS ? ret2 : ret;
> >   }
> > @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
> >       EFI_EXIT(ret);
> >   }
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> > +{
> > +     if (!uctx) {
> > +             log_warning("No EFI HTTP boot context found\n");
> > +             return EFI_SUCCESS;
> > +     }
> > +
> > +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> > +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
> > +}
> > +
> >   /**
> >    * try_load_from_uri_path() - Handle the URI device path
> >    *
> > @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >       if (!ctx)
> >               return EFI_OUT_OF_RESOURCES;
> >
> > +     uctx = ctx;
> >       s = env_get("loadaddr");
> >       if (!s) {
> >               log_err("Error: loadaddr is not set\n");
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index 00167bd2a1..33cd8b9a50 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
> >       return 0;
> >   }
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt)
> > +{
> > +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> > +}
> > +
> >   static int u16_tohex(u16 c)
> >   {
> >       if (c >= '0' && c <= '9')
>

Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Heinrich Schuchardt]

On 11/11/24 11:45, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Mon, 11 Nov 2024 at 10:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/25/24 13:14, Sughosh Ganuefi_bootmgr_pmem_setup wrote:
>>> The EFI HTTP boot puts the iso installer image at some location in
>>> memory which needs to be reserved in the devicetree as persistent
>>> memory (pmem). Add helper functions which add this pmem node when the
>>> EFI_DT_FIXUP protocol's fixup callback is invoked.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>> ---
>>>    boot/image-fdt.c             |  9 +++++++++
>>>    include/efi_loader.h         | 17 +++++++++++++++++
>>>    lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>>>    lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>>>    4 files changed, 59 insertions(+)
>>>
>>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>>> index 8eda521693..b39e81ad30 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_loader.h>
>>>    #include <env.h>
>>>    #include <errno.h>
>>>    #include <image.h>
>>> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>>>        if (!ft_verify_fdt(blob))
>>>                goto err;
>>>
>>> +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
>>> +             fdt_ret = fdt_efi_pmem_setup(blob);
>>
>> I can see no reason why pmem setup should depend on HTTP boot.
>
> The reason is that we *know* we want to preserve image on HTTP
> installers. but we should only add it if that image is booted, not
> unconditionally if EFI_HTTP is enabled
>
>>
>> It should be possible to pass a memory block device to the kernel no
>> matter how it was created.
>
> Yes, we can. But how do we know we need to setup a pmem node? In this
> specific case, we know http installers need the image.
> If we can find similar rules (or perhaps a command line option), we
> can preserve it for all images

Simon is working on patches to track loaded images. I guess there we
would need to add the detection of image types including bare kernels,
EFI binaries, and ISOs.

>
> Thanks
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>> +             if (fdt_ret) {
>>> +                     printf("ERROR: HTTP boot pmem fixup failed\n");

Please, use log_err() and remove "ERROR: ".

>>> +                     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/include/efi_loader.h b/include/efi_loader.h
>>> index d450e304c6..031de18746 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>>>    efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>>>                                    efi_guid_t *guid);
>>>
>>> +/**
>>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
>>> + *
>>> + * @fdt:     Pointer to the devicetree
>>> + *
>>> + * Return:   0 on success, negative on failure
>>> + */
>>> +int fdt_efi_pmem_setup(void *fdt);
>>> +
>>>    /**
>>>     * efi_size_in_pages() - convert size in bytes to size in pages
>>>     *
>>> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>>>                                  void *load_options);
>>>    efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>>>
>>> +/**
>>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
>>> + *
>>> + * @fdt:     Pointer to the DT blob
>>> + * Return:   status code
>>> + */
>>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
>>> +
>>>    /**
>>>     * struct efi_image_regions - A list of memory regions
>>>     *
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 16f75555f6..1d9246be61 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -41,6 +41,8 @@ struct uridp_context {
>>>        efi_handle_t mem_handle;
>>>    };
>>>
>>> +static struct uridp_context *uctx;
>>> +
>>>    const efi_guid_t efi_guid_bootmenu_auto_generated =
>>>                EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>>>
>>> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>>>
>>>        efi_free_pool(ctx->loaded_dp);
>>>        free(ctx);
>>> +     uctx = NULL;
>>>
>>>        return ret == EFI_SUCCESS ? ret2 : ret;
>>>    }
>>> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>>>        EFI_EXIT(ret);
>>>    }
>>>
>>> +/**
>>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
>>> + *
>>> + * @fdt:     Pointer to the DT blob
>>> + * Return:   status code
>>> + */
>>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
>>> +{
>>> +     if (!uctx) {
>>> +             log_warning("No EFI HTTP boot context found\n");

Are we writing a warning here when loading grubriscv64.efi from disk and
executing it?

Best regards

Heinrich

>>> +             return EFI_SUCCESS;
>>> +     }
>>> +
>>> +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
>>> +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
>>> +}
>>> +
>>>    /**
>>>     * try_load_from_uri_path() - Handle the URI device path
>>>     *
>>> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>>>        if (!ctx)
>>>                return EFI_OUT_OF_RESOURCES;
>>>
>>> +     uctx = ctx;
>>>        s = env_get("loadaddr");
>>>        if (!s) {
>>>                log_err("Error: loadaddr is not set\n");
>>> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
>>> index 00167bd2a1..33cd8b9a50 100644
>>> --- a/lib/efi_loader/efi_helper.c
>>> +++ b/lib/efi_loader/efi_helper.c
>>> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>>>        return 0;
>>>    }
>>>
>>> +/**
>>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
>>> + *
>>> + * @fdt:     Pointer to the devicetree
>>> + *
>>> + * Return:   0 on success, negative on failure
>>> + */
>>> +int fdt_efi_pmem_setup(void *fdt)
>>> +{
>>> +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
>>> +}
>>> +
>>>    static int u16_tohex(u16 c)
>>>    {
>>>        if (c >= '0' && c <= '9')
>>

Re: [PATCH 4/4] efi: add helper functions to insert pmem node for DT fixup

[Ilias Apalodimas]

On Mon, 11 Nov 2024 at 13:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/11/24 11:45, Ilias Apalodimas wrote:
> > Hi Heinrich
> >
> > On Mon, 11 Nov 2024 at 10:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/25/24 13:14, Sughosh Ganuefi_bootmgr_pmem_setup wrote:
> >>> The EFI HTTP boot puts the iso installer image at some location in
> >>> memory which needs to be reserved in the devicetree as persistent
> >>> memory (pmem). Add helper functions which add this pmem node when the
> >>> EFI_DT_FIXUP protocol's fixup callback is invoked.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>> ---
> >>>    boot/image-fdt.c             |  9 +++++++++
> >>>    include/efi_loader.h         | 17 +++++++++++++++++
> >>>    lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
> >>>    lib/efi_loader/efi_helper.c  | 12 ++++++++++++
> >>>    4 files changed, 59 insertions(+)
> >>>
> >>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> >>> index 8eda521693..b39e81ad30 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_loader.h>
> >>>    #include <env.h>
> >>>    #include <errno.h>
> >>>    #include <image.h>
> >>> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
> >>>        if (!ft_verify_fdt(blob))
> >>>                goto err;
> >>>
> >>> +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> >>> +             fdt_ret = fdt_efi_pmem_setup(blob);
> >>
> >> I can see no reason why pmem setup should depend on HTTP boot.
> >
> > The reason is that we *know* we want to preserve image on HTTP
> > installers. but we should only add it if that image is booted, not
> > unconditionally if EFI_HTTP is enabled
> >
> >>
> >> It should be possible to pass a memory block device to the kernel no
> >> matter how it was created.
> >
> > Yes, we can. But how do we know we need to setup a pmem node? In this
> > specific case, we know http installers need the image.
> > If we can find similar rules (or perhaps a command line option), we
> > can preserve it for all images
>
> Simon is working on patches to track loaded images. I guess there we
> would need to add the detection of image types including bare kernels,
> EFI binaries, and ISOs.
>

Yes that sounds like a good idea.

> >
> > Thanks
> > /Ilias
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +             if (fdt_ret) {
> >>> +                     printf("ERROR: HTTP boot pmem fixup failed\n");
>
> Please, use log_err() and remove "ERROR: ".
>
> >>> +                     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/include/efi_loader.h b/include/efi_loader.h
> >>> index d450e304c6..031de18746 100644
> >>> --- a/include/efi_loader.h
> >>> +++ b/include/efi_loader.h
> >>> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
> >>>    efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >>>                                    efi_guid_t *guid);
> >>>
> >>> +/**
> >>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> >>> + *
> >>> + * @fdt:     Pointer to the devicetree
> >>> + *
> >>> + * Return:   0 on success, negative on failure
> >>> + */
> >>> +int fdt_efi_pmem_setup(void *fdt);
> >>> +
> >>>    /**
> >>>     * efi_size_in_pages() - convert size in bytes to size in pages
> >>>     *
> >>> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> >>>                                  void *load_options);
> >>>    efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> >>>
> >>> +/**
> >>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> >>> + *
> >>> + * @fdt:     Pointer to the DT blob
> >>> + * Return:   status code
> >>> + */
> >>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> >>> +
> >>>    /**
> >>>     * struct efi_image_regions - A list of memory regions
> >>>     *
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 16f75555f6..1d9246be61 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -41,6 +41,8 @@ struct uridp_context {
> >>>        efi_handle_t mem_handle;
> >>>    };
> >>>
> >>> +static struct uridp_context *uctx;
> >>> +
> >>>    const efi_guid_t efi_guid_bootmenu_auto_generated =
> >>>                EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
> >>>
> >>> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
> >>>
> >>>        efi_free_pool(ctx->loaded_dp);
> >>>        free(ctx);
> >>> +     uctx = NULL;
> >>>
> >>>        return ret == EFI_SUCCESS ? ret2 : ret;
> >>>    }
> >>> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
> >>>        EFI_EXIT(ret);
> >>>    }
> >>>
> >>> +/**
> >>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> >>> + *
> >>> + * @fdt:     Pointer to the DT blob
> >>> + * Return:   status code
> >>> + */
> >>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> >>> +{
> >>> +     if (!uctx) {
> >>> +             log_warning("No EFI HTTP boot context found\n");
>
> Are we writing a warning here when loading grubriscv64.efi from disk and
> executing it?

Probably, that links to my previous comment. This should only execute
when we *know* we boot an EFI image via HTTP, not for any load.
Since we only setup that context for EFI images, simply removing the
print works for now. But as you mentioned we should plug this better
in the future once Simon series land

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >>> +             return EFI_SUCCESS;
> >>> +     }
> >>> +
> >>> +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> >>> +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
> >>> +}
> >>> +
> >>>    /**
> >>>     * try_load_from_uri_path() - Handle the URI device path
> >>>     *
> >>> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >>>        if (!ctx)
> >>>                return EFI_OUT_OF_RESOURCES;
> >>>
> >>> +     uctx = ctx;
> >>>        s = env_get("loadaddr");
> >>>        if (!s) {
> >>>                log_err("Error: loadaddr is not set\n");
> >>> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> >>> index 00167bd2a1..33cd8b9a50 100644
> >>> --- a/lib/efi_loader/efi_helper.c
> >>> +++ b/lib/efi_loader/efi_helper.c
> >>> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
> >>>        return 0;
> >>>    }
> >>>
> >>> +/**
> >>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> >>> + *
> >>> + * @fdt:     Pointer to the devicetree
> >>> + *
> >>> + * Return:   0 on success, negative on failure
> >>> + */
> >>> +int fdt_efi_pmem_setup(void *fdt)
> >>> +{
> >>> +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> >>> +}
> >>> +
> >>>    static int u16_tohex(u16 c)
> >>>    {
> >>>        if (c >= '0' && c <= '9')
> >>
>

Re: [PATCH 0/4] Add pmem node for preserving distro ISO's

[Ilias Apalodimas]

+CC Anton

On Fri, 25 Oct 2024 at 14:14, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> 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.
>
>
>
> 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 (1):
>   efi: add helper functions to insert pmem node for DT fixup
>
>  boot/fdt_support.c           | 41 +++++++++++++++++++++++++++--
>  boot/image-fdt.c             |  9 +++++++
>  include/efi_loader.h         | 28 +++++++++++++++++---
>  include/fdt_support.h        | 13 +++++++++
>  lib/efi_loader/efi_bootmgr.c | 43 ++++++++++++++++++++++++++----
>  lib/efi_loader/efi_helper.c  | 12 +++++++++
>  lib/efi_loader/efi_memory.c  | 51 +++++++++++++++++++++++++++---------
>  lib/lmb.c                    |  4 +--
>  8 files changed, 175 insertions(+), 26 deletions(-)
>
> --
> 2.34.1
>
>

Re: [PATCH 0/4] Add pmem node for preserving distro ISO's

[Heinrich Schuchardt]

On 10/25/24 13:14, Sughosh Ganu wrote:
> 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.

Are you planning to provide the missing ACPI code for U-Boot?
QEMU provides ACPI tables which U-Boot passes through.
The x86 boards also need ACPI support.

Best regards

Heinrich

>
> 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.
>
>
>
> 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 (1):
>    efi: add helper functions to insert pmem node for DT fixup
>
>   boot/fdt_support.c           | 41 +++++++++++++++++++++++++++--
>   boot/image-fdt.c             |  9 +++++++
>   include/efi_loader.h         | 28 +++++++++++++++++---
>   include/fdt_support.h        | 13 +++++++++
>   lib/efi_loader/efi_bootmgr.c | 43 ++++++++++++++++++++++++++----
>   lib/efi_loader/efi_helper.c  | 12 +++++++++
>   lib/efi_loader/efi_memory.c  | 51 +++++++++++++++++++++++++++---------
>   lib/lmb.c                    |  4 +--
>   8 files changed, 175 insertions(+), 26 deletions(-)
>