[PATCH v3 1/5] fdt: add support for adding pmem nodes
Sughosh Ganu
sughosh.ganu at linaro.org
Fri Jan 24 11:56:33 CET 2025
On Mon, 20 Jan 2025 at 19:32, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Mon, 20 Jan 2025 at 18:01, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 20.01.25 11:50, 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
> > > 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.
> >
> > We also have ACPI support in U-Boot. In a future patch series I guess we
> > could add the generation of said ACPI tables.
> >
> > >
> > > [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 V2:
> > > * Fix a checkpatch error by putting a blank line after a function
> > >
> > > boot/fdt_support.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > > include/fdt_support.h | 13 +++++++++++++
> > > 2 files changed, 52 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/boot/fdt_support.c b/boot/fdt_support.c
> > > index 49efeec3681..613685b80eb 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
> >
> > Do we need this #ifdef at all? We tend to leave it to the linker to
> > eliminate functions.
>
> This patch was authored by Kojima-san. I will have to check if we need
> to keep the ifdef.
I tried removing the ifdef from the said location. However, this
causes a size increase on platforms where this config symbol is not
enabled -- I tried this with the xilinx_zynqmp_virt_defconfig. This
could be because the fdt_fixup_memory() function is included on all
platforms as it gets called from cmd/fdt.c. The fdt_fixup_memory()
function calls fdt_fixup_memory_banks() which is protected by this
ifdef. Removing the ifdef then results in the fdt_fixup_memory_banks()
getting included, instead of the empty stub resulting in the size
increase.
-sughosh
>
> >
> > > #if CONFIG_NR_DRAM_BANKS > 4
> > > #define MEMORY_BANKS_MAX CONFIG_NR_DRAM_BANKS
> > > #else
> > > @@ -2222,3 +2223,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");
> >
> > Nits:
> >
> > %s/needs at 2MB alignment/must be 2 MiB aligned/
>
> Will change
>
> >
> > > + 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..aea24df828f 100644
> > > --- a/include/fdt_support.h
> > > +++ b/include/fdt_support.h
> > > @@ -507,4 +507,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
> >
> > If I understand the code correctly, the function adds or updates a pmem
> > device-tree node for the given start address.
>
> Will reword the comment.
>
> -sughosh
>
> >
> > Otherwise looks good to me.
> >
> > Best regards
> >
> > Heinrich
> >
> > > + *
> > > + * @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 */
> >
More information about the U-Boot
mailing list