Re: [PATCH v2 1/2] mm/memblock: Add "reserve_mem" to reserved named memory at boot up

[Wei Yang]

On Thu, Jun 06, 2024 at 11:01:44AM -0400, Steven Rostedt wrote:
>From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
>
>In order to allow for requesting a memory region that can be used for
>things like pstore on multiple machines where the memory layout is not the
>same, add a new option to the kernel command line called "reserve_mem".
>
>The format is:  reserve_mem=nn:align:name
>
>Where it will find nn amount of memory at the given alignment of align.
>The name field is to allow another subsystem to retrieve where the memory
>was found. For example:
>
>  reserve_mem=12M:4096:oops ramoops.mem_name=oops
>
>Where ramoops.mem_name will tell ramoops that memory was reserved for it
>via the reserve_mem option and it can find it by calling:
>
>  if (reserve_mem_find_by_name("oops", &start, &size)) {
>	// start holds the start address and size holds the size given
>
>This is typically used for systems that do not wipe the RAM, and this
>command line will try to reserve the same physical memory on soft reboots.
>Note, it is not guaranteed to be the same location. For example, if KASLR
>places the kernel at the location of where the RAM reservation was from a
>previous boot, the new reservation will be at a different location.  Any
>subsystem using this feature must add a way to verify that the contents of
>the physical memory is from a previous boot, as there may be cases where
>the memory will not be located at the same location.
>
>Not all systems may work either. There could be bit flips if the reboot
>goes through the BIOS. Using kexec to reboot the machine is likely to
>have better results in such cases.
>
>Link: https://lore.kernel.org/all/ZjJVnZUX3NZiGW6q@xxxxxxxxxx/
>
>Suggested-by: Mike Rapoport <rppt@xxxxxxxxxx>
>Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
>---
> .../admin-guide/kernel-parameters.txt         | 20 ++++
> include/linux/mm.h                            |  2 +
> mm/memblock.c                                 | 97 +++++++++++++++++++
> 3 files changed, 119 insertions(+)
>
>diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>index b600df82669d..4b2f7fb8de66 100644
>--- a/Documentation/admin-guide/kernel-parameters.txt
>+++ b/Documentation/admin-guide/kernel-parameters.txt
>@@ -5710,6 +5710,26 @@
> 			them.  If <base> is less than 0x10000, the region
> 			is assumed to be I/O ports; otherwise it is memory.
> 
>+	reserve_mem=	[RAM]
>+			Format: nn[KNG]:<align>:<label>
>+			Reserve physical memory and label it with a name that
>+			other subsystems can use to access it. This is typically
>+			used for systems that do not wipe the RAM, and this command
>+			line will try to reserve the same physical memory on
>+			soft reboots. Note, it is not guaranteed to be the same
>+			location. For example, if KASLR places the kernel at the
>+			location of where the RAM reservation was from a previous
>+			boot, the new reservation will be at a different location.
>+			Any subsystem using this feature must add a way to verify
>+			that the contents of the physical memory is from a previous
>+			boot, as there may be cases where the memory will not be
>+			located at the same location.
>+
>+			The format is size:align:label for example, to request
>+			12 megabytes of 4096 alignment for ramoops:
>+
>+			reserver_mem=12M:4096:oops ramoops.mem_name=oops
>+
> 	reservetop=	[X86-32,EARLY]
> 			Format: nn[KMG]
> 			Reserves a hole at the top of the kernel virtual
>diff --git a/include/linux/mm.h b/include/linux/mm.h
>index 9849dfda44d4..b4455cc02f2c 100644
>--- a/include/linux/mm.h
>+++ b/include/linux/mm.h
>@@ -4263,4 +4263,6 @@ static inline bool pfn_is_unaccepted_memory(unsigned long pfn)
> void vma_pgtable_walk_begin(struct vm_area_struct *vma);
> void vma_pgtable_walk_end(struct vm_area_struct *vma);
> 
>+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned long *size);
>+
> #endif /* _LINUX_MM_H */
>diff --git a/mm/memblock.c b/mm/memblock.c
>index d09136e040d3..a8bf0ee9e2b4 100644
>--- a/mm/memblock.c
>+++ b/mm/memblock.c
>@@ -2244,6 +2244,103 @@ void __init memblock_free_all(void)
> 	totalram_pages_add(pages);
> }
> 
>+/* Keep a table to reserve named memory */
>+#define RESERVE_MEM_MAX_ENTRIES		8
>+#define RESERVE_MEM_NAME_SIZE		16
                                        ^
Suggest to align with previous line.

>+struct reserve_mem_table {
>+	char			name[RESERVE_MEM_NAME_SIZE];
>+	unsigned long		start;
>+	unsigned long		size;

phys_addr_t looks more precise?

>+};
>+static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
>+static int reserved_mem_count;

Seems no matter we use this feature or not, these memory would be occupied?

>+
>+/* Add wildcard region with a lookup name */
>+static int __init reserved_mem_add(unsigned long start, unsigned long size,
>+				   const char *name)
>+{
>+	struct reserve_mem_table *map;
>+
>+	if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
>+		return -EINVAL;
>+
>+	if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
>+		return -1;

return ENOSPC? Not good at it, but a raw value maybe not a good practice.

Also, we'd better do this check before allocation.

>+
>+	map = &reserved_mem_table[reserved_mem_count++];
>+	map->start = start;
>+	map->size = size;
>+	strscpy(map->name, name);
>+	return 0;
>+}
>+
>+/**
>+ * reserve_mem_find_by_name - Find reserved memory region with a given name
>+ * @name: The name that is attached to a reserved memory region
>+ * @start: If found, holds the start address
>+ * @size: If found, holds the size of the address.
>+ *
>+ * Returns: 1 if found or 0 if not found.
>+ */
>+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned long *size)
>+{
>+	struct reserve_mem_table *map;
>+	int i;
>+
>+	for (i = 0; i < reserved_mem_count; i++) {
>+		map = &reserved_mem_table[i];
>+		if (!map->size)
>+			continue;
>+		if (strcmp(name, map->name) == 0) {
>+			*start = map->start;
>+			*size = map->size;
>+			return 1;
>+		}
>+	}
>+	return 0;
>+}
>+
>+/*
>+ * Parse early_reserve_mem=nn:align:name

early_reserve_mem or reserve_mem ?

>+ */
>+static int __init reserve_mem(char *p)
>+{
>+	phys_addr_t start, size, align;
>+	char *oldp;
>+	int err;
>+
>+	if (!p)
>+		return -EINVAL;
>+
>+	oldp = p;
>+	size = memparse(p, &p);
>+	if (p == oldp)
>+		return -EINVAL;
>+
>+	if (*p != ':')
>+		return -EINVAL;
>+
>+	align = memparse(p+1, &p);
>+	if (*p != ':')
>+		return -EINVAL;
>+

Better to check if the name is valid here. 

Make sure command line parameters are valid before doing the allocation.

>+	start = memblock_phys_alloc(size, align);
>+	if (!start)
>+		return -ENOMEM;
>+
>+	p++;
>+	err = reserved_mem_add(start, size, p);
>+	if (err) {
>+		memblock_phys_free(start, size);
>+		return err;
>+	}
>+
>+	p += strlen(p);
>+
>+	return *p == '\0' ? 0: -EINVAL;

We won't free the memory if return -EINVAL?

>+}
>+__setup("reserve_mem=", reserve_mem);
>+
> #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
> static const char * const flagname[] = {
> 	[ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",
>-- 
>2.43.0
>
>

-- 
Wei Yang
Help you, Help me