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

On Tue, 11 Jun 2024 15:26:05 -0700
Guenter Roeck [off-list ref] wrote:

quoted

diff --git a/mm/memblock.c b/mm/memblock.c
index d09136e040d3..044ddce8f085 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c

@@ -2244,6 +2244,121 @@ 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
+struct reserve_mem_table {
+	char			name[RESERVE_MEM_NAME_SIZE];
+	phys_addr_t		start;
+	phys_addr_t		size;
+};
+static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
+static int reserved_mem_count;
+
+/* Add wildcard region with a lookup name */
+static int __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
+				   const char *name)
+{
+	struct reserve_mem_table *map;
+
+	if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
+		return -EINVAL;
+  

I know I am picky, but name is never NULL, and strlen(name) is guaranteed to be > 0.
Personally I'd suggest to check for strlen(name) >= RESERVE_MEM_NAME_SIZE together
with !strlen(name) and drop the duplicate checks here (and, as side effect, avoid
the pointless memory allocation if the name is invalid).

Yeah, it's now checked before hand. I'll remove it for v5.

quoted

+	if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
+		return -1;
+
+	map = &reserved_mem_table[reserved_mem_count++];
+	map->start = start;
+	map->size = size;
+	strscpy(map->name, name);
+	return 0;
+}
+

quoted

+/*
+ * Parse reserve_mem=nn:align:name
+ */
+static int __init reserve_mem(char *p)
+{
+	phys_addr_t start, size, align;
+	char *name;
+	char *oldp;
+	int err;
+
+	if (!p)
+		return -EINVAL;
+
+	oldp = p;
+	size = memparse(p, &p);
+	if (!size || p == oldp)
+		return -EINVAL;
+
+	if (*p != ':')
+		return -EINVAL;
+
+	align = memparse(p+1, &p);
+	if (*p != ':')
+		return -EINVAL;
+
+	/*
+	 * memblock_phys_alloc() doesn't like a zero size align,
+	 * but it is OK for this command to have it.
+	 */
+	if (align <= SMP_CACHE_BYTES)  

Any reason for using <= instead of < ?

Nope. Not sure why I did that. :-/

I'll fix that too.

Thanks,

-- Steve

Guenter

quoted

+		align = SMP_CACHE_BYTES;
+
+	name = p + 1;
+	if (!strlen(name))
+		return -EINVAL;
+
+	/* Make sure that name has text */
+	for (p = name; *p; p++) {
+		if (!isspace(*p))
+			break;
+	}
+	if (!*p)
+		return -EINVAL;
+
+	start = memblock_phys_alloc(size, align);
+	if (!start)
+		return -ENOMEM;
+
+	err = reserved_mem_add(start, size, name);
+	if (err) {
+		memblock_phys_free(start, size);
+		return err;
+	}
+
+	return 0;
+}
+__setup("reserve_mem=", reserve_mem);
+
  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
  static const char * const flagname[] = {
  	[ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",