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

[Sughosh Ganu]

On Thu, 13 Mar 2025 at 02:12, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 12.03.25 09:54, Sughosh Ganu wrote:
> > From: Masahisa Kojima <kojima.masahisa at 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 at socionext.com>
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at 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 */
>