diff mbox series

[v8,3/8] fdt: add support for adding pmem nodes

Message ID 20250312085424.1201148-4-sughosh.ganu@linaro.org
State New
Headers show
Series Add pmem node for preserving distro ISO's | expand

Commit Message

Sughosh Ganu March 12, 2025, 8:54 a.m. UTC 
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>
---
Changes since V7: None

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

Comments

Heinrich Schuchardt March 12, 2025, 8:42 p.m. UTC | #1 
On 12.03.25 09:54, Sughosh Ganu 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

A mounted ISO only goes away if it is a RAM disk. But you do not mention 
RAM disks in the first paragraph. Please, add an introductory sentence 
that this patch is about RAM disk support.

Whether the RAM disk contains an installer or a preinstalled OS or 
anything else is irrelevant for the the need of a pmem node to support 
RAM disks. If the RAM disk is partitioned (possibly as ISO9660) or is 
just a raw disk is irrelevant here, too.

> 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.

Nits:

%s/launhing/launching/

> 
> [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>
> ---
> Changes since V7: None
> 
>   boot/fdt_support.c    | 41 ++++++++++++++++++++++++++++++++++++++++-
>   include/fdt_support.h | 14 ++++++++++++++
>   2 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/boot/fdt_support.c b/boot/fdt_support.c
> index 49efeec3681..e20b9759138 100644
> --- a/boot/fdt_support.c
> +++ b/boot/fdt_support.c
> @@ -18,6 +18,7 @@
>   #include <dm/ofnode.h>
>   #include <linux/ctype.h>
>   #include <linux/types.h>
> +#include <linux/sizes.h>
>   #include <asm/global_data.h>
>   #include <asm/unaligned.h>
>   #include <linux/libfdt.h>
> @@ -464,7 +465,6 @@ void do_fixup_by_compat_u32(void *fdt, const char *compat,
>   	do_fixup_by_compat(fdt, compat, prop, &tmp, 4, create);
>   }
>   
> -#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
>   /*
>    * fdt_pack_reg - pack address and size array into the "reg"-suitable stream
>    */
> @@ -493,6 +493,7 @@ static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size,
>   	return p - (char *)buf;
>   }
>   
> +#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
>   #if CONFIG_NR_DRAM_BANKS > 4
>   #define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS
>   #else
> @@ -2222,3 +2223,41 @@ int fdt_valid(struct fdt_header **blobp)
>   	}
>   	return 1;
>   }
> +
> +int fdt_fixup_pmem_region(void *blob, ulong addr, u32 size)
> +{
> +	u64 pmem_start[2] = { 0 };
> +	u64 pmem_size[2] = { 0 };

You never use index 1. There is no need for defining an array here.
You could use:

u64 pmem_start;
u64 pmem_size;

But better simply define the parameters addr and size of the function as 
u64 and use them directly.

> +	char pmem_node[32] = {0};

%s/pmem_node/node_name/g

Why should we the initialize the field here?

> +	int nodeoffset, len;
> +	int err;
> +	u8 tmp[4 * 16]; /* Up to 64-bit address + 64-bit size */
> +
> +	if (!IS_ALIGNED(addr, SZ_2M) || !IS_ALIGNED(addr + size, SZ_2M)) {
> +		printf("Start and end address must be 2MiB aligned\n");
> +		return -1;
> +	}
> +
> +	snprintf(pmem_node, sizeof(pmem_node), "pmem@%lx", addr);
> +	nodeoffset = fdt_find_or_add_subnode(blob, 0, pmem_node);
> +	if (nodeoffset < 0)
> +		return nodeoffset;
> +
> +	err = fdt_setprop_string(blob, nodeoffset, "compatible", "pmem-region");
> +	if (err)
> +		return err;
> +	err = fdt_setprop_empty(blob, nodeoffset, "volatile");
> +	if (err)
> +		return err;
> +	pmem_start[0] = addr;
> +	pmem_size[0] = size;
> +	len = fdt_pack_reg(blob, tmp, pmem_start, pmem_size, 1);

len = fdt_pack_reg(blob, tmp, &addr, &size, 1);

assuming that addr, size are changed to u64.

> +	err = fdt_setprop(blob, nodeoffset, "reg", tmp, len);
> +	if (err < 0) {
> +		printf("WARNING: could not set pmem %s %s.\n", "reg",
> +		       fdt_strerror(err));
> +		return err;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index f0ad2e6b365..b72cd2920de 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -507,4 +507,18 @@ void fdt_fixup_pstore(void *blob);
>    */
>   int fdt_kaslrseed(void *blob, bool overwrite);
>   
> +/**
> + * fdt_fixup_pmem_region() - add a pmem node on the device tree
> + *
> + * This functions adds/updates a pmem node to the device tree.
> + * Usually used with EFI installers to preserve installer
> + * images
> + *
> + * @blob:	device tree provided by caller

Calling device-tree blob and not fdt is confusing.

Best regards

Heinrich

> + * @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 */
Sughosh Ganu March 13, 2025, 7:19 a.m. UTC | #2 
On Thu, 13 Mar 2025 at 02:12, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 12.03.25 09:54, Sughosh Ganu 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
>
> A mounted ISO only goes away if it is a RAM disk. But you do not mention
> RAM disks in the first paragraph. Please, add an introductory sentence
> that this patch is about RAM disk support.
>
> Whether the RAM disk contains an installer or a preinstalled OS or
> anything else is irrelevant for the the need of a pmem node to support
> RAM disks. If the RAM disk is partitioned (possibly as ISO9660) or is
> just a raw disk is irrelevant here, too.

Okay, I will re-word the commit message. Btw, I am not the author of
this patch, nor did I write this commit message. I will incorporate
all the review comments. But I do have one gripe. Please check below.

>
> > 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.
>
> Nits:
>
> %s/launhing/launching/
>
> >
> > [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>
> > ---
> > Changes since V7: None
> >
> >   boot/fdt_support.c    | 41 ++++++++++++++++++++++++++++++++++++++++-
> >   include/fdt_support.h | 14 ++++++++++++++
> >   2 files changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/boot/fdt_support.c b/boot/fdt_support.c
> > index 49efeec3681..e20b9759138 100644
> > --- a/boot/fdt_support.c
> > +++ b/boot/fdt_support.c
> > @@ -18,6 +18,7 @@
> >   #include <dm/ofnode.h>
> >   #include <linux/ctype.h>
> >   #include <linux/types.h>
> > +#include <linux/sizes.h>
> >   #include <asm/global_data.h>
> >   #include <asm/unaligned.h>
> >   #include <linux/libfdt.h>
> > @@ -464,7 +465,6 @@ void do_fixup_by_compat_u32(void *fdt, const char *compat,
> >       do_fixup_by_compat(fdt, compat, prop, &tmp, 4, create);
> >   }
> >
> > -#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
> >   /*
> >    * fdt_pack_reg - pack address and size array into the "reg"-suitable stream
> >    */
> > @@ -493,6 +493,7 @@ static int fdt_pack_reg(const void *fdt, void *buf, u64 *address, u64 *size,
> >       return p - (char *)buf;
> >   }
> >
> > +#ifdef CONFIG_ARCH_FIXUP_FDT_MEMORY
> >   #if CONFIG_NR_DRAM_BANKS > 4
> >   #define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS
> >   #else
> > @@ -2222,3 +2223,41 @@ int fdt_valid(struct fdt_header **blobp)
> >       }
> >       return 1;
> >   }
> > +
> > +int fdt_fixup_pmem_region(void *blob, ulong addr, u32 size)
> > +{
> > +     u64 pmem_start[2] = { 0 };
> > +     u64 pmem_size[2] = { 0 };
>
> You never use index 1. There is no need for defining an array here.
> You could use:
>
> u64 pmem_start;
> u64 pmem_size;
>
> But better simply define the parameters addr and size of the function as
> u64 and use them directly.
>
> > +     char pmem_node[32] = {0};
>
> %s/pmem_node/node_name/g
>
> Why should we the initialize the field here?
>
> > +     int nodeoffset, len;
> > +     int err;
> > +     u8 tmp[4 * 16]; /* Up to 64-bit address + 64-bit size */
> > +
> > +     if (!IS_ALIGNED(addr, SZ_2M) || !IS_ALIGNED(addr + size, SZ_2M)) {
> > +             printf("Start and end address must be 2MiB aligned\n");
> > +             return -1;
> > +     }
> > +
> > +     snprintf(pmem_node, sizeof(pmem_node), "pmem@%lx", addr);
> > +     nodeoffset = fdt_find_or_add_subnode(blob, 0, pmem_node);
> > +     if (nodeoffset < 0)
> > +             return nodeoffset;
> > +
> > +     err = fdt_setprop_string(blob, nodeoffset, "compatible", "pmem-region");
> > +     if (err)
> > +             return err;
> > +     err = fdt_setprop_empty(blob, nodeoffset, "volatile");
> > +     if (err)
> > +             return err;
> > +     pmem_start[0] = addr;
> > +     pmem_size[0] = size;
> > +     len = fdt_pack_reg(blob, tmp, pmem_start, pmem_size, 1);
>
> len = fdt_pack_reg(blob, tmp, &addr, &size, 1);
>
> assuming that addr, size are changed to u64.
>
> > +     err = fdt_setprop(blob, nodeoffset, "reg", tmp, len);
> > +     if (err < 0) {
> > +             printf("WARNING: could not set pmem %s %s.\n", "reg",
> > +                    fdt_strerror(err));
> > +             return err;
> > +     }
> > +
> > +     return 0;
> > +}
> > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > index f0ad2e6b365..b72cd2920de 100644
> > --- a/include/fdt_support.h
> > +++ b/include/fdt_support.h
> > @@ -507,4 +507,18 @@ void fdt_fixup_pstore(void *blob);
> >    */
> >   int fdt_kaslrseed(void *blob, bool overwrite);
> >
> > +/**
> > + * fdt_fixup_pmem_region() - add a pmem node on the device tree
> > + *
> > + * This functions adds/updates a pmem node to the device tree.
> > + * Usually used with EFI installers to preserve installer
> > + * images
> > + *
> > + * @blob:    device tree provided by caller
>
> Calling device-tree blob and not fdt is confusing.

I can make this change. But this is splitting hairs :). Anyone looking
at the code *has* to understand what this blob means.

Also,

grep -R '*blob' --include=*.c | wc -l
664

-sughosh

>
> Best regards
>
> Heinrich
>
> > + * @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 */
>
diff mbox series

Patch

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