On Thu, 13 Mar 2025 at 02:12, Heinrich Schuchardt <[email protected]> wrote: > > On 12.03.25 09:54, Sughosh Ganu wrote: > > From: Masahisa Kojima <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Sughosh Ganu <[email protected]> > > --- > > 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 */ >
