linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] Dynamic allocation of reserved_mem array.
@ 2023-10-19 18:48 Oreoluwa Babatunde
  2023-10-19 18:48 ` [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored Oreoluwa Babatunde
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Oreoluwa Babatunde @ 2023-10-19 18:48 UTC (permalink / raw)
  To: catalin.marinas, will, robh+dt, frowand.list
  Cc: linux-arm-kernel, linux-kernel, devicetree, linux-arm-msm,
	kernel, Oreoluwa Babatunde

The reserved_mem array is used to store the data of the different
reserved memory regions specified in the DT of a device.
The array stores information such as the name, node, starting address,
and size of a reserved memory region.

The array is currently statically allocated with a size of
MAX_RESERVED_REGIONS(64). This means that any system that specifies a
number of reserved memory regions greater than MAX_RESERVED_REGIONS(64)
will not have enough space to store the information for all the regions.

Therefore, this series changes the reserved_mem array from a static
array with a fixed size of MAX_RESERVED_REGIONS(64), to a dynamically
allocated array, using memblock_alloc(), based on the number of reserved
memory regions specified in the DT.

Memory gotten from memblock_alloc() is only writable after paging_init()
is called, but the reserved memory regions need to be reserved before
then so that the system does not create page table mappings for them.

It is possible to call memblock_resverve() and memblock_mark_nomap() on
the reserved memory regions and not need to save them to the array until
we dynamically allocate the array after paging_init(), but this becomes
an issue with reserved memory regions that do not have their starting
address specified in the DT.
i.e. regions specified with the "alloc_ranges" and "size" properties.

Therefore, this series achieves the allocation and population of the
reserved_mem array in two steps:

1. Before paging_init()
   Before paging_init() is called, iterate through the reserved_mem
   nodes in the DT and do the following:
   - Allocate memory for dynamically placed reserved memory regions and
     store their starting address in the static allocated reserved_mem
     array.
   - Call memblock_reserve() and memblock_mark_nomap() on all the
     reserved memory regions.
   - Count the total number of reserved_mem nodes in the DT.

2. After paging_init()
   After paging_init() is called:
   - Allocate new memory for the reserved_mem array based on the number
     of reserved memory nodes in the DT.
   - Transfer all the information that was stored in the static array
     into the new array.
   - Store the rest of the reserved_mem regions in the new array.
     i.e. reserved memory nodes specified with the "reg" property.

The static array is no longer needed after this point, but there is
currently no obvious way to free the memory. Therefore, the size of the
initial static array is now defined using a config option.
Since its size would vary depending on the user, scaling it can help get
some memory savings.

Oreoluwa Babatunde (3):
  of: reserved_mem: Change the order that reserved_mem regions are
    stored
  of: reserved_mem: Add code to dynamically allocate reserved_mem array
  of: reserved_mem: Make MAX_RESERVED_REGIONS a config option

 arch/arm64/kernel/setup.c       |   4 ++
 drivers/of/Kconfig              |  13 ++++
 drivers/of/fdt.c                |  63 +++++++++++++++----
 drivers/of/of_private.h         |   3 +-
 drivers/of/of_reserved_mem.c    | 108 ++++++++++++++++++++++----------
 include/linux/of_fdt.h          |   1 +
 include/linux/of_reserved_mem.h |   9 +++
 7 files changed, 154 insertions(+), 47 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored
  2023-10-19 18:48 [RFC PATCH 0/3] Dynamic allocation of reserved_mem array Oreoluwa Babatunde
@ 2023-10-19 18:48 ` Oreoluwa Babatunde
  2023-10-19 19:46   ` Rob Herring
  2023-10-24 19:32   ` Rob Herring
  2023-10-19 18:48 ` [RFC PATCH 2/3] of: reserved_mem: Add code to dynamically allocate reserved_mem array Oreoluwa Babatunde
  2023-10-19 18:48 ` [RFC PATCH 3/3] of: reserved_mem: Make MAX_RESERVED_REGIONS a config option Oreoluwa Babatunde
  2 siblings, 2 replies; 9+ messages in thread
From: Oreoluwa Babatunde @ 2023-10-19 18:48 UTC (permalink / raw)
  To: catalin.marinas, will, robh+dt, frowand.list
  Cc: linux-arm-kernel, linux-kernel, devicetree, linux-arm-msm,
	kernel, Oreoluwa Babatunde

The dynamic allocation of the reserved_mem array needs to be done after
paging_init() is called because memory allocated using memblock_alloc()
is not writeable before that.

Nodes that already have their starting address specified in the DT
(i.e. nodes that are defined using the "reg" property) can wait until
after paging_init() to be stored in the array.
But nodes that are dynamically placed need to be reserved and saved in
the array before paging_init() so that page table entries are not
created for these regions.

Hence, change the code to:
1. Before paging_init(), allocate and store information for the
   dynamically placed reserved memory regions.
2. After paging_init(), store the rest of the reserved memory regions
   which are defined with the "reg" property.

Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
---
 arch/arm64/kernel/setup.c       |  4 +++
 drivers/of/fdt.c                | 56 ++++++++++++++++++++++++++-------
 drivers/of/of_private.h         |  1 -
 drivers/of/of_reserved_mem.c    | 54 ++++++++++++++-----------------
 include/linux/of_fdt.h          |  1 +
 include/linux/of_reserved_mem.h |  9 ++++++
 6 files changed, 83 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 417a8a86b2db..6002d3ad0b19 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -27,6 +27,8 @@
 #include <linux/proc_fs.h>
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
+#include <linux/of_reserved_mem.h>
+
 #include <linux/efi.h>
 #include <linux/psci.h>
 #include <linux/sched/task.h>
@@ -346,6 +348,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
 
 	paging_init();
 
+	fdt_init_reserved_mem();
+
 	acpi_table_upgrade();
 
 	/* Parse the ACPI tables for possible boot-time configuration */
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bf502ba8da95..d51a1176a7b9 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -504,7 +504,6 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 	phys_addr_t base, size;
 	int len;
 	const __be32 *prop;
-	int first = 1;
 	bool nomap;
 
 	prop = of_get_flat_dt_prop(node, "reg", &len);
@@ -532,10 +531,6 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 			       uname, &base, (unsigned long)(size / SZ_1M));
 
 		len -= t_len;
-		if (first) {
-			fdt_reserved_mem_save_node(node, uname, base, size);
-			first = 0;
-		}
 	}
 	return 0;
 }
@@ -564,9 +559,44 @@ static int __init __reserved_mem_check_root(unsigned long node)
 }
 
 /*
- * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory
+ * Save the reserved_mem reg nodes in the reserved_mem array
  */
-static int __init fdt_scan_reserved_mem(void)
+static void save_reserved_mem_reg_nodes(unsigned long node, const char *uname)
+
+{
+	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+	phys_addr_t base, size;
+	int len;
+	const __be32 *prop;
+
+	prop = of_get_flat_dt_prop(node, "reg", &len);
+	if (!prop)
+		return;
+
+	if (len && len % t_len != 0) {
+		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
+		       uname);
+		return;
+	}
+	base = dt_mem_next_cell(dt_root_addr_cells, &prop);
+	size = dt_mem_next_cell(dt_root_size_cells, &prop);
+
+	if (size)
+		fdt_reserved_mem_save_node(node, uname, base, size);
+}
+
+/*
+ * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory.
+ * @save_only: Option to determine what kind of fdt scan the caller is
+ * requesting.
+ *
+ * The fdt is scanned twice here during device bootup. The first scan
+ * is used to save the dynamically allocated reserved memory regions to
+ * the reserved_mem array. The second scan is used to save the 'reg'
+ * defined regions to the array. @save_only indicates which of the scans
+ * the caller is requesting.
+ */
+int __init fdt_scan_reserved_mem(bool save_only)
 {
 	int node, child;
 	const void *fdt = initial_boot_params;
@@ -589,9 +619,14 @@ static int __init fdt_scan_reserved_mem(void)
 
 		uname = fdt_get_name(fdt, child, NULL);
 
+		if (save_only) {
+			save_reserved_mem_reg_nodes(child, uname);
+			continue;
+		}
+
 		err = __reserved_mem_reserve_reg(child, uname);
 		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL))
-			fdt_reserved_mem_save_node(child, uname, 0, 0);
+			__reserved_mem_alloc_size(child, uname);
 	}
 	return 0;
 }
@@ -631,11 +666,12 @@ void __init early_init_fdt_scan_reserved_mem(void)
 {
 	int n;
 	u64 base, size;
+	bool save_only = false;
 
 	if (!initial_boot_params)
 		return;
 
-	fdt_scan_reserved_mem();
+	fdt_scan_reserved_mem(save_only);
 	fdt_reserve_elfcorehdr();
 
 	/* Process header /memreserve/ fields */
@@ -645,8 +681,6 @@ void __init early_init_fdt_scan_reserved_mem(void)
 			break;
 		memblock_reserve(base, size);
 	}
-
-	fdt_init_reserved_mem();
 }
 
 /**
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index f38397c7b582..e52b27b8392d 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -175,7 +175,6 @@ static inline struct device_node *__of_get_dma_parent(const struct device_node *
 }
 #endif
 
-void fdt_init_reserved_mem(void);
 void fdt_reserved_mem_save_node(unsigned long node, const char *uname,
 			       phys_addr_t base, phys_addr_t size);
 
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 7ec94cfcbddb..13e694f5e316 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -132,8 +132,7 @@ static int __init __reserved_mem_alloc_in_range(phys_addr_t size,
  * __reserved_mem_alloc_size() - allocate reserved memory described by
  *	'size', 'alignment'  and 'alloc-ranges' properties.
  */
-static int __init __reserved_mem_alloc_size(unsigned long node,
-	const char *uname, phys_addr_t *res_base, phys_addr_t *res_size)
+int __init __reserved_mem_alloc_size(unsigned long node, const char *uname)
 {
 	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
 	phys_addr_t start = 0, end = 0;
@@ -212,10 +211,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
 		       uname, (unsigned long)(size / SZ_1M));
 		return -ENOMEM;
 	}
-
-	*res_base = base;
-	*res_size = size;
-
+	fdt_reserved_mem_save_node(node, uname, base, size);
 	return 0;
 }
 
@@ -309,6 +305,9 @@ static void __init __rmem_check_for_overlap(void)
 void __init fdt_init_reserved_mem(void)
 {
 	int i;
+	bool save_only = true;
+
+	fdt_scan_reserved_mem(save_only);
 
 	/* check for overlapping reserved regions */
 	__rmem_check_for_overlap();
@@ -328,30 +327,25 @@ void __init fdt_init_reserved_mem(void)
 		if (prop)
 			rmem->phandle = of_read_number(prop, len/4);
 
-		if (rmem->size == 0)
-			err = __reserved_mem_alloc_size(node, rmem->name,
-						 &rmem->base, &rmem->size);
-		if (err == 0) {
-			err = __reserved_mem_init_node(rmem);
-			if (err != 0 && err != -ENOENT) {
-				pr_info("node %s compatible matching fail\n",
-					rmem->name);
-				if (nomap)
-					memblock_clear_nomap(rmem->base, rmem->size);
-				else
-					memblock_phys_free(rmem->base,
-							   rmem->size);
-			} else {
-				phys_addr_t end = rmem->base + rmem->size - 1;
-				bool reusable =
-					(of_get_flat_dt_prop(node, "reusable", NULL)) != NULL;
-
-				pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
-					&rmem->base, &end, (unsigned long)(rmem->size / SZ_1K),
-					nomap ? "nomap" : "map",
-					reusable ? "reusable" : "non-reusable",
-					rmem->name ? rmem->name : "unknown");
-			}
+		err = __reserved_mem_init_node(rmem);
+		if (err != 0 && err != -ENOENT) {
+			pr_info("node %s compatible matching fail\n",
+				rmem->name);
+			if (nomap)
+				memblock_clear_nomap(rmem->base, rmem->size);
+			else
+				memblock_phys_free(rmem->base,
+						   rmem->size);
+		} else {
+			phys_addr_t end = rmem->base + rmem->size - 1;
+			bool reusable =
+				(of_get_flat_dt_prop(node, "reusable", NULL)) != NULL;
+
+			pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
+				&rmem->base, &end, (unsigned long)(rmem->size / SZ_1K),
+				nomap ? "nomap" : "map",
+				reusable ? "reusable" : "non-reusable",
+				rmem->name ? rmem->name : "unknown");
 		}
 	}
 }
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index d69ad5bb1eb1..a9a9f70dabea 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -77,6 +77,7 @@ extern void early_init_dt_scan_nodes(void);
 extern const char *of_flat_dt_get_machine_name(void);
 extern const void *of_flat_dt_match_machine(const void *default_match,
 		const void * (*get_next_compat)(const char * const**));
+extern int fdt_scan_reserved_mem(bool save_only);
 
 /* Other Prototypes */
 extern void unflatten_device_tree(void);
diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
index 4de2a24cadc9..e310336cef37 100644
--- a/include/linux/of_reserved_mem.h
+++ b/include/linux/of_reserved_mem.h
@@ -38,6 +38,8 @@ int of_reserved_mem_device_init_by_name(struct device *dev,
 					struct device_node *np,
 					const char *name);
 void of_reserved_mem_device_release(struct device *dev);
+void fdt_init_reserved_mem(void);
+int __reserved_mem_alloc_size(unsigned long node, const char *uname);
 
 struct reserved_mem *of_reserved_mem_lookup(struct device_node *np);
 #else
@@ -60,6 +62,13 @@ static inline int of_reserved_mem_device_init_by_name(struct device *dev,
 
 static inline void of_reserved_mem_device_release(struct device *pdev) { }
 
+static inline int __reserved_mem_alloc_size(unsigned long node, const char *uname)
+{
+	return -ENOSYS;
+}
+
+static inline void fdt_init_reserved_mem(void) { }
+
 static inline struct reserved_mem *of_reserved_mem_lookup(struct device_node *np)
 {
 	return NULL;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH 2/3] of: reserved_mem: Add code to dynamically allocate reserved_mem array
  2023-10-19 18:48 [RFC PATCH 0/3] Dynamic allocation of reserved_mem array Oreoluwa Babatunde
  2023-10-19 18:48 ` [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored Oreoluwa Babatunde
@ 2023-10-19 18:48 ` Oreoluwa Babatunde
  2023-10-19 18:48 ` [RFC PATCH 3/3] of: reserved_mem: Make MAX_RESERVED_REGIONS a config option Oreoluwa Babatunde
  2 siblings, 0 replies; 9+ messages in thread
From: Oreoluwa Babatunde @ 2023-10-19 18:48 UTC (permalink / raw)
  To: catalin.marinas, will, robh+dt, frowand.list
  Cc: linux-arm-kernel, linux-kernel, devicetree, linux-arm-msm,
	kernel, Oreoluwa Babatunde

The reserved_mem array is statically allocated with a size of
MAX_RESERVED_REGIONS(64). Therefore, if the number of reserved_mem
regions exceeds this size, there will not be enough space to store
all the data.

To fix this, dynamically allocate memory for the reserved_mem array
based on the number of reserved memory regions speicified in the DT.

Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
---
 drivers/of/fdt.c             |  9 ++++--
 drivers/of/of_private.h      |  2 ++
 drivers/of/of_reserved_mem.c | 53 ++++++++++++++++++++++++++++++++++--
 3 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d51a1176a7b9..408fd15b7db0 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -598,7 +598,7 @@ static void save_reserved_mem_reg_nodes(unsigned long node, const char *uname)
  */
 int __init fdt_scan_reserved_mem(bool save_only)
 {
-	int node, child;
+	int node, child, count = 0;
 	const void *fdt = initial_boot_params;
 
 	node = fdt_path_offset(fdt, "/reserved-memory");
@@ -626,8 +626,13 @@ int __init fdt_scan_reserved_mem(bool save_only)
 
 		err = __reserved_mem_reserve_reg(child, uname);
 		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL))
-			__reserved_mem_alloc_size(child, uname);
+			err = __reserved_mem_alloc_size(child, uname);
+
+		if (err == 0)
+			count++;
 	}
+	if (!save_only)
+		update_reserved_mem_max_cnt(count);
 	return 0;
 }
 
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index e52b27b8392d..ca1ba9f06660 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -178,4 +178,6 @@ static inline struct device_node *__of_get_dma_parent(const struct device_node *
 void fdt_reserved_mem_save_node(unsigned long node, const char *uname,
 			       phys_addr_t base, phys_addr_t size);
 
+void update_reserved_mem_max_cnt(int max_count);
+
 #endif /* _LINUX_OF_PRIVATE_H */
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 13e694f5e316..203828ca118e 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -27,7 +27,10 @@
 #include "of_private.h"
 
 #define MAX_RESERVED_REGIONS	64
-static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
+static struct reserved_mem reserved_mem_array[MAX_RESERVED_REGIONS];
+static struct reserved_mem *reserved_mem __refdata = reserved_mem_array;
+
+static int total_reserved_mem_cnt = MAX_RESERVED_REGIONS;
 static int reserved_mem_count;
 
 static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
@@ -55,6 +58,45 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
 	return err;
 }
 
+void __init update_reserved_mem_max_cnt(int max_count)
+{
+	total_reserved_mem_cnt = max_count;
+}
+
+static int alloc_reserved_mem_array(void)
+{
+	struct reserved_mem *new_array;
+	size_t alloc_size, copy_size, memset_size;
+
+	alloc_size = array_size(total_reserved_mem_cnt, sizeof(*new_array));
+	if (alloc_size == SIZE_MAX)
+		return -1;
+
+	new_array = memblock_alloc(alloc_size, SMP_CACHE_BYTES);
+	if (!new_array)
+		return -ENOMEM;
+
+	copy_size = array_size(reserved_mem_count, sizeof(*new_array));
+	if (copy_size == SIZE_MAX)
+		goto overlow_err;
+
+	if (total_reserved_mem_cnt >= reserved_mem_count)
+		memset_size = alloc_size - copy_size;
+	else
+		memset_size = copy_size - alloc_size;
+
+	memcpy(new_array, reserved_mem, copy_size);
+	memset(new_array + reserved_mem_count, 0, memset_size);
+
+	reserved_mem = new_array;
+	return 0;
+
+overlow_err:
+	memblock_free(new_array, alloc_size);
+	total_reserved_mem_cnt = MAX_RESERVED_REGIONS;
+	return -1;
+}
+
 /*
  * fdt_reserved_mem_save_node() - save fdt node for second pass initialization
  */
@@ -63,7 +105,7 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
 {
 	struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
 
-	if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) {
+	if (reserved_mem_count == total_reserved_mem_cnt) {
 		pr_err("not enough space for all defined regions.\n");
 		return;
 	}
@@ -304,9 +346,14 @@ static void __init __rmem_check_for_overlap(void)
  */
 void __init fdt_init_reserved_mem(void)
 {
-	int i;
+	int i, ret = 0;
 	bool save_only = true;
 
+	ret = alloc_reserved_mem_array();
+	if (ret) {
+		pr_err("Failed to store reserved_mem nodes in array with err: %d", ret);
+		return;
+	}
 	fdt_scan_reserved_mem(save_only);
 
 	/* check for overlapping reserved regions */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH 3/3] of: reserved_mem: Make MAX_RESERVED_REGIONS a config option
  2023-10-19 18:48 [RFC PATCH 0/3] Dynamic allocation of reserved_mem array Oreoluwa Babatunde
  2023-10-19 18:48 ` [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored Oreoluwa Babatunde
  2023-10-19 18:48 ` [RFC PATCH 2/3] of: reserved_mem: Add code to dynamically allocate reserved_mem array Oreoluwa Babatunde
@ 2023-10-19 18:48 ` Oreoluwa Babatunde
  2 siblings, 0 replies; 9+ messages in thread
From: Oreoluwa Babatunde @ 2023-10-19 18:48 UTC (permalink / raw)
  To: catalin.marinas, will, robh+dt, frowand.list
  Cc: linux-arm-kernel, linux-kernel, devicetree, linux-arm-msm,
	kernel, Oreoluwa Babatunde

Add code to make the size of reserved_mem_array a config option which
can be modified based on user requirements.
The reserved_mem_array is required during device bootup to store the
information of the reserved memory regions that need to be dynamically
allocated. After some time, this information is transferred to another
array which is used to store all reserved memory regions, after which
the reserved_mem_array will no longer be needed.

Since the size required for the reserved_mem_array can vary and there
is currently no way to free the memory afterwards, make the size of the
array configurable in an attempt to save some memory.

Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
---
 drivers/of/Kconfig           | 13 +++++++++++++
 drivers/of/of_reserved_mem.c |  9 ++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index da9826accb1b..3cb2ec4993b5 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -102,4 +102,17 @@ config OF_OVERLAY
 config OF_NUMA
 	bool
 
+config OF_MAX_RESERVED_REGIONS
+	int "OF resvered_mem array size"
+	default "64"
+	range 1 64
+	help
+	  The reserved_mem_array is used to store information about the dynamically
+	  placed reserved memory regions before we are able to allocate the memory
+	  needed to store all the reserved memory regions defined in the DT.
+	  Because the amount of memory needed initially for this array could vary,
+	  make the size of the reserved_mem_array configurable in an attempt to
+	  save some memory when possible.
+	  if unsure, leave as default value.
+
 endif # OF
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 203828ca118e..f408dce762e1 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -26,11 +26,10 @@
 
 #include "of_private.h"
 
-#define MAX_RESERVED_REGIONS	64
-static struct reserved_mem reserved_mem_array[MAX_RESERVED_REGIONS];
-static struct reserved_mem *reserved_mem __refdata = reserved_mem_array;
+static struct reserved_mem reserved_mem_array[CONFIG_OF_MAX_RESERVED_REGIONS];
+static struct reserved_mem *reserved_mem = reserved_mem_array;
 
-static int total_reserved_mem_cnt = MAX_RESERVED_REGIONS;
+static int total_reserved_mem_cnt = CONFIG_OF_MAX_RESERVED_REGIONS;
 static int reserved_mem_count;
 
 static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
@@ -93,7 +92,7 @@ static int alloc_reserved_mem_array(void)
 
 overlow_err:
 	memblock_free(new_array, alloc_size);
-	total_reserved_mem_cnt = MAX_RESERVED_REGIONS;
+	total_reserved_mem_cnt = CONFIG_OF_MAX_RESERVED_REGIONS;
 	return -1;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored
  2023-10-19 18:48 ` [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored Oreoluwa Babatunde
@ 2023-10-19 19:46   ` Rob Herring
  2023-10-19 22:45     ` Oreoluwa Babatunde
  2023-10-24 19:32   ` Rob Herring
  1 sibling, 1 reply; 9+ messages in thread
From: Rob Herring @ 2023-10-19 19:46 UTC (permalink / raw)
  To: Oreoluwa Babatunde
  Cc: catalin.marinas, will, frowand.list, linux-arm-kernel,
	linux-kernel, devicetree, linux-arm-msm, kernel

On Thu, Oct 19, 2023 at 1:49 PM Oreoluwa Babatunde
<quic_obabatun@quicinc.com> wrote:
>
> The dynamic allocation of the reserved_mem array needs to be done after
> paging_init() is called because memory allocated using memblock_alloc()
> is not writeable before that.
>
> Nodes that already have their starting address specified in the DT
> (i.e. nodes that are defined using the "reg" property) can wait until
> after paging_init() to be stored in the array.
> But nodes that are dynamically placed need to be reserved and saved in
> the array before paging_init() so that page table entries are not
> created for these regions.
>
> Hence, change the code to:
> 1. Before paging_init(), allocate and store information for the
>    dynamically placed reserved memory regions.
> 2. After paging_init(), store the rest of the reserved memory regions
>    which are defined with the "reg" property.
>
> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> ---
>  arch/arm64/kernel/setup.c       |  4 +++
>  drivers/of/fdt.c                | 56 ++++++++++++++++++++++++++-------
>  drivers/of/of_private.h         |  1 -
>  drivers/of/of_reserved_mem.c    | 54 ++++++++++++++-----------------
>  include/linux/of_fdt.h          |  1 +
>  include/linux/of_reserved_mem.h |  9 ++++++
>  6 files changed, 83 insertions(+), 42 deletions(-)
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 417a8a86b2db..6002d3ad0b19 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -27,6 +27,8 @@
>  #include <linux/proc_fs.h>
>  #include <linux/memblock.h>
>  #include <linux/of_fdt.h>
> +#include <linux/of_reserved_mem.h>
> +
>  #include <linux/efi.h>
>  #include <linux/psci.h>
>  #include <linux/sched/task.h>
> @@ -346,6 +348,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>
>         paging_init();
>
> +       fdt_init_reserved_mem();
> +

You removed this call from the common code and add it to arm64 arch
code, doesn't that break every other arch?

The very next thing done here is unflattening the DT. So another call
from the arch code to the DT code isn't needed either.

Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored
  2023-10-19 19:46   ` Rob Herring
@ 2023-10-19 22:45     ` Oreoluwa Babatunde
  2023-10-24 19:15       ` Rob Herring
  0 siblings, 1 reply; 9+ messages in thread
From: Oreoluwa Babatunde @ 2023-10-19 22:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: catalin.marinas, will, frowand.list, linux-arm-kernel,
	linux-kernel, devicetree, linux-arm-msm, kernel


On 10/19/2023 12:46 PM, Rob Herring wrote:
> On Thu, Oct 19, 2023 at 1:49 PM Oreoluwa Babatunde
> <quic_obabatun@quicinc.com> wrote:
>> The dynamic allocation of the reserved_mem array needs to be done after
>> paging_init() is called because memory allocated using memblock_alloc()
>> is not writeable before that.
>>
>> Nodes that already have their starting address specified in the DT
>> (i.e. nodes that are defined using the "reg" property) can wait until
>> after paging_init() to be stored in the array.
>> But nodes that are dynamically placed need to be reserved and saved in
>> the array before paging_init() so that page table entries are not
>> created for these regions.
>>
>> Hence, change the code to:
>> 1. Before paging_init(), allocate and store information for the
>>    dynamically placed reserved memory regions.
>> 2. After paging_init(), store the rest of the reserved memory regions
>>    which are defined with the "reg" property.
>>
>> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
>> ---
>>  arch/arm64/kernel/setup.c       |  4 +++
>>  drivers/of/fdt.c                | 56 ++++++++++++++++++++++++++-------
>>  drivers/of/of_private.h         |  1 -
>>  drivers/of/of_reserved_mem.c    | 54 ++++++++++++++-----------------
>>  include/linux/of_fdt.h          |  1 +
>>  include/linux/of_reserved_mem.h |  9 ++++++
>>  6 files changed, 83 insertions(+), 42 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 417a8a86b2db..6002d3ad0b19 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -27,6 +27,8 @@
>>  #include <linux/proc_fs.h>
>>  #include <linux/memblock.h>
>>  #include <linux/of_fdt.h>
>> +#include <linux/of_reserved_mem.h>
>> +
>>  #include <linux/efi.h>
>>  #include <linux/psci.h>
>>  #include <linux/sched/task.h>
>> @@ -346,6 +348,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>>
>>         paging_init();
>>
>> +       fdt_init_reserved_mem();
>> +
> You removed this call from the common code and add it to arm64 arch
> code, doesn't that break every other arch?
Yes, the same changes will be needed for every other arch. I was hoping to
get some feedback on the RFC before implementing this on other archs which
is why the change is currently only in arm64.
> The very next thing done here is unflattening the DT. So another call
> from the arch code to the DT code isn't needed either.
Yes, I see that unflatten_device_tree() is being called right after here.
Just to clarify, are you suggesting to move fdt_init_reserved_mem() into the
unflatten_device_tree() call?


Thanks, Oreoluwa

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored
  2023-10-19 22:45     ` Oreoluwa Babatunde
@ 2023-10-24 19:15       ` Rob Herring
  0 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2023-10-24 19:15 UTC (permalink / raw)
  To: Oreoluwa Babatunde
  Cc: catalin.marinas, will, frowand.list, linux-arm-kernel,
	linux-kernel, devicetree, linux-arm-msm, kernel

On Thu, Oct 19, 2023 at 03:45:37PM -0700, Oreoluwa Babatunde wrote:
> 
> On 10/19/2023 12:46 PM, Rob Herring wrote:
> > On Thu, Oct 19, 2023 at 1:49 PM Oreoluwa Babatunde
> > <quic_obabatun@quicinc.com> wrote:
> >> The dynamic allocation of the reserved_mem array needs to be done after
> >> paging_init() is called because memory allocated using memblock_alloc()
> >> is not writeable before that.


> >> --- a/arch/arm64/kernel/setup.c
> >> +++ b/arch/arm64/kernel/setup.c
> >> @@ -27,6 +27,8 @@
> >>  #include <linux/proc_fs.h>
> >>  #include <linux/memblock.h>
> >>  #include <linux/of_fdt.h>
> >> +#include <linux/of_reserved_mem.h>
> >> +
> >>  #include <linux/efi.h>
> >>  #include <linux/psci.h>
> >>  #include <linux/sched/task.h>
> >> @@ -346,6 +348,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> >>
> >>         paging_init();
> >>
> >> +       fdt_init_reserved_mem();
> >> +
> > You removed this call from the common code and add it to arm64 arch
> > code, doesn't that break every other arch?
> Yes, the same changes will be needed for every other arch. I was hoping to
> get some feedback on the RFC before implementing this on other archs which
> is why the change is currently only in arm64.
> > The very next thing done here is unflattening the DT. So another call
> > from the arch code to the DT code isn't needed either.
> Yes, I see that unflatten_device_tree() is being called right after here.
> Just to clarify, are you suggesting to move fdt_init_reserved_mem() into the
> unflatten_device_tree() call?

In general, I want fewer calls between arch code and DT core and for the 
DT core to be more in control of the ordering that things happen. Your 
series does the opposite.

Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored
  2023-10-19 18:48 ` [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored Oreoluwa Babatunde
  2023-10-19 19:46   ` Rob Herring
@ 2023-10-24 19:32   ` Rob Herring
  2023-12-04  4:18     ` Oreoluwa Babatunde
  1 sibling, 1 reply; 9+ messages in thread
From: Rob Herring @ 2023-10-24 19:32 UTC (permalink / raw)
  To: Oreoluwa Babatunde
  Cc: catalin.marinas, will, frowand.list, linux-arm-kernel,
	linux-kernel, devicetree, linux-arm-msm, kernel

On Thu, Oct 19, 2023 at 11:48:23AM -0700, Oreoluwa Babatunde wrote:
> The dynamic allocation of the reserved_mem array needs to be done after
> paging_init() is called because memory allocated using memblock_alloc()
> is not writeable before that.
> 
> Nodes that already have their starting address specified in the DT
> (i.e. nodes that are defined using the "reg" property) can wait until
> after paging_init() to be stored in the array.
> But nodes that are dynamically placed need to be reserved and saved in
> the array before paging_init() so that page table entries are not
> created for these regions.
> 
> Hence, change the code to:
> 1. Before paging_init(), allocate and store information for the
>    dynamically placed reserved memory regions.
> 2. After paging_init(), store the rest of the reserved memory regions
>    which are defined with the "reg" property.
> 
> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> ---
>  arch/arm64/kernel/setup.c       |  4 +++
>  drivers/of/fdt.c                | 56 ++++++++++++++++++++++++++-------
>  drivers/of/of_private.h         |  1 -
>  drivers/of/of_reserved_mem.c    | 54 ++++++++++++++-----------------
>  include/linux/of_fdt.h          |  1 +
>  include/linux/of_reserved_mem.h |  9 ++++++
>  6 files changed, 83 insertions(+), 42 deletions(-)

Looking at this a bit more...

> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 417a8a86b2db..6002d3ad0b19 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -27,6 +27,8 @@
>  #include <linux/proc_fs.h>
>  #include <linux/memblock.h>
>  #include <linux/of_fdt.h>
> +#include <linux/of_reserved_mem.h>
> +
>  #include <linux/efi.h>
>  #include <linux/psci.h>
>  #include <linux/sched/task.h>
> @@ -346,6 +348,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>  
>  	paging_init();
>  
> +	fdt_init_reserved_mem();
> +
>  	acpi_table_upgrade();
>  
>  	/* Parse the ACPI tables for possible boot-time configuration */
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index bf502ba8da95..d51a1176a7b9 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -504,7 +504,6 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>  	phys_addr_t base, size;
>  	int len;
>  	const __be32 *prop;
> -	int first = 1;
>  	bool nomap;
>  
>  	prop = of_get_flat_dt_prop(node, "reg", &len);
> @@ -532,10 +531,6 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>  			       uname, &base, (unsigned long)(size / SZ_1M));
>  
>  		len -= t_len;
> -		if (first) {
> -			fdt_reserved_mem_save_node(node, uname, base, size);
> -			first = 0;
> -		}
>  	}
>  	return 0;
>  }
> @@ -564,9 +559,44 @@ static int __init __reserved_mem_check_root(unsigned long node)
>  }
>  
>  /*
> - * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory
> + * Save the reserved_mem reg nodes in the reserved_mem array
>   */
> -static int __init fdt_scan_reserved_mem(void)
> +static void save_reserved_mem_reg_nodes(unsigned long node, const char *uname)
> +
> +{
> +	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> +	phys_addr_t base, size;
> +	int len;
> +	const __be32 *prop;
> +
> +	prop = of_get_flat_dt_prop(node, "reg", &len);
> +	if (!prop)
> +		return;
> +
> +	if (len && len % t_len != 0) {
> +		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
> +		       uname);
> +		return;
> +	}
> +	base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +	size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +
> +	if (size)
> +		fdt_reserved_mem_save_node(node, uname, base, size);
> +}
> +
> +/*
> + * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory.
> + * @save_only: Option to determine what kind of fdt scan the caller is
> + * requesting.
> + *
> + * The fdt is scanned twice here during device bootup. The first scan
> + * is used to save the dynamically allocated reserved memory regions to
> + * the reserved_mem array. The second scan is used to save the 'reg'
> + * defined regions to the array. @save_only indicates which of the scans
> + * the caller is requesting.
> + */
> +int __init fdt_scan_reserved_mem(bool save_only)

It's generally discouraged to use flags to make a function do 2 
different operations. Splitting it will naturally happen though with my 
next comment.

>  {
>  	int node, child;
>  	const void *fdt = initial_boot_params;
> @@ -589,9 +619,14 @@ static int __init fdt_scan_reserved_mem(void)
>  
>  		uname = fdt_get_name(fdt, child, NULL);
>  
> +		if (save_only) {
> +			save_reserved_mem_reg_nodes(child, uname);
> +			continue;
> +		}
> +
>  		err = __reserved_mem_reserve_reg(child, uname);
>  		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL))
> -			fdt_reserved_mem_save_node(child, uname, 0, 0);
> +			__reserved_mem_alloc_size(child, uname);
>  	}
>  	return 0;
>  }
> @@ -631,11 +666,12 @@ void __init early_init_fdt_scan_reserved_mem(void)
>  {
>  	int n;
>  	u64 base, size;
> +	bool save_only = false;
>  
>  	if (!initial_boot_params)
>  		return;
>  
> -	fdt_scan_reserved_mem();
> +	fdt_scan_reserved_mem(save_only);
>  	fdt_reserve_elfcorehdr();
>  
>  	/* Process header /memreserve/ fields */
> @@ -645,8 +681,6 @@ void __init early_init_fdt_scan_reserved_mem(void)
>  			break;
>  		memblock_reserve(base, size);
>  	}
> -
> -	fdt_init_reserved_mem();
>  }
>  
>  /**
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index f38397c7b582..e52b27b8392d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -175,7 +175,6 @@ static inline struct device_node *__of_get_dma_parent(const struct device_node *
>  }
>  #endif
>  
> -void fdt_init_reserved_mem(void);
>  void fdt_reserved_mem_save_node(unsigned long node, const char *uname,
>  			       phys_addr_t base, phys_addr_t size);
>  
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 7ec94cfcbddb..13e694f5e316 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -132,8 +132,7 @@ static int __init __reserved_mem_alloc_in_range(phys_addr_t size,
>   * __reserved_mem_alloc_size() - allocate reserved memory described by
>   *	'size', 'alignment'  and 'alloc-ranges' properties.
>   */
> -static int __init __reserved_mem_alloc_size(unsigned long node,
> -	const char *uname, phys_addr_t *res_base, phys_addr_t *res_size)
> +int __init __reserved_mem_alloc_size(unsigned long node, const char *uname)
>  {
>  	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
>  	phys_addr_t start = 0, end = 0;
> @@ -212,10 +211,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
>  		       uname, (unsigned long)(size / SZ_1M));
>  		return -ENOMEM;
>  	}
> -
> -	*res_base = base;
> -	*res_size = size;
> -
> +	fdt_reserved_mem_save_node(node, uname, base, size);
>  	return 0;
>  }
>  
> @@ -309,6 +305,9 @@ static void __init __rmem_check_for_overlap(void)
>  void __init fdt_init_reserved_mem(void)
>  {
>  	int i;
> +	bool save_only = true;
> +
> +	fdt_scan_reserved_mem(save_only);

This is now called just before unflattening. Why can't it be done right 
after unflattening instead. Then we can use the unflattened API rather 
than the flatten API which is less efficient.

Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored
  2023-10-24 19:32   ` Rob Herring
@ 2023-12-04  4:18     ` Oreoluwa Babatunde
  0 siblings, 0 replies; 9+ messages in thread
From: Oreoluwa Babatunde @ 2023-12-04  4:18 UTC (permalink / raw)
  To: Rob Herring
  Cc: catalin.marinas, will, frowand.list, linux-arm-kernel,
	linux-kernel, devicetree, linux-arm-msm, kernel


On 10/24/2023 12:32 PM, Rob Herring wrote:
> On Thu, Oct 19, 2023 at 11:48:23AM -0700, Oreoluwa Babatunde wrote:
>> The dynamic allocation of the reserved_mem array needs to be done after
>> paging_init() is called because memory allocated using memblock_alloc()
>> is not writeable before that.
>>
>> Nodes that already have their starting address specified in the DT
>> (i.e. nodes that are defined using the "reg" property) can wait until
>> after paging_init() to be stored in the array.
>> But nodes that are dynamically placed need to be reserved and saved in
>> the array before paging_init() so that page table entries are not
>> created for these regions.
>>
>> Hence, change the code to:
>> 1. Before paging_init(), allocate and store information for the
>>    dynamically placed reserved memory regions.
>> 2. After paging_init(), store the rest of the reserved memory regions
>>    which are defined with the "reg" property.
>>
>> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
>> ---
>>  arch/arm64/kernel/setup.c       |  4 +++
>>  drivers/of/fdt.c                | 56 ++++++++++++++++++++++++++-------
>>  drivers/of/of_private.h         |  1 -
>>  drivers/of/of_reserved_mem.c    | 54 ++++++++++++++-----------------
>>  include/linux/of_fdt.h          |  1 +
>>  include/linux/of_reserved_mem.h |  9 ++++++
>>  6 files changed, 83 insertions(+), 42 deletions(-)
> Looking at this a bit more...
>
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 417a8a86b2db..6002d3ad0b19 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -27,6 +27,8 @@
>>  #include <linux/proc_fs.h>
>>  #include <linux/memblock.h>
>>  #include <linux/of_fdt.h>
>> +#include <linux/of_reserved_mem.h>
>> +
>>  #include <linux/efi.h>
>>  #include <linux/psci.h>
>>  #include <linux/sched/task.h>
>> @@ -346,6 +348,8 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>>  
>>  	paging_init();
>>  
>> +	fdt_init_reserved_mem();
>> +
>>  	acpi_table_upgrade();
>>  
>>  	/* Parse the ACPI tables for possible boot-time configuration */
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index bf502ba8da95..d51a1176a7b9 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -504,7 +504,6 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>>  	phys_addr_t base, size;
>>  	int len;
>>  	const __be32 *prop;
>> -	int first = 1;
>>  	bool nomap;
>>  
>>  	prop = of_get_flat_dt_prop(node, "reg", &len);
>> @@ -532,10 +531,6 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>>  			       uname, &base, (unsigned long)(size / SZ_1M));
>>  
>>  		len -= t_len;
>> -		if (first) {
>> -			fdt_reserved_mem_save_node(node, uname, base, size);
>> -			first = 0;
>> -		}
>>  	}
>>  	return 0;
>>  }
>> @@ -564,9 +559,44 @@ static int __init __reserved_mem_check_root(unsigned long node)
>>  }
>>  
>>  /*
>> - * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory
>> + * Save the reserved_mem reg nodes in the reserved_mem array
>>   */
>> -static int __init fdt_scan_reserved_mem(void)
>> +static void save_reserved_mem_reg_nodes(unsigned long node, const char *uname)
>> +
>> +{
>> +	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
>> +	phys_addr_t base, size;
>> +	int len;
>> +	const __be32 *prop;
>> +
>> +	prop = of_get_flat_dt_prop(node, "reg", &len);
>> +	if (!prop)
>> +		return;
>> +
>> +	if (len && len % t_len != 0) {
>> +		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
>> +		       uname);
>> +		return;
>> +	}
>> +	base = dt_mem_next_cell(dt_root_addr_cells, &prop);
>> +	size = dt_mem_next_cell(dt_root_size_cells, &prop);
>> +
>> +	if (size)
>> +		fdt_reserved_mem_save_node(node, uname, base, size);
>> +}
>> +
>> +/*
>> + * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory.
>> + * @save_only: Option to determine what kind of fdt scan the caller is
>> + * requesting.
>> + *
>> + * The fdt is scanned twice here during device bootup. The first scan
>> + * is used to save the dynamically allocated reserved memory regions to
>> + * the reserved_mem array. The second scan is used to save the 'reg'
>> + * defined regions to the array. @save_only indicates which of the scans
>> + * the caller is requesting.
>> + */
>> +int __init fdt_scan_reserved_mem(bool save_only)
> It's generally discouraged to use flags to make a function do 2 
> different operations. Splitting it will naturally happen though with my 
> next comment.
>
>>  {
>>  	int node, child;
>>  	const void *fdt = initial_boot_params;
>> @@ -589,9 +619,14 @@ static int __init fdt_scan_reserved_mem(void)
>>  
>>  		uname = fdt_get_name(fdt, child, NULL);
>>  
>> +		if (save_only) {
>> +			save_reserved_mem_reg_nodes(child, uname);
>> +			continue;
>> +		}
>> +
>>  		err = __reserved_mem_reserve_reg(child, uname);
>>  		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL))
>> -			fdt_reserved_mem_save_node(child, uname, 0, 0);
>> +			__reserved_mem_alloc_size(child, uname);
>>  	}
>>  	return 0;
>>  }
>> @@ -631,11 +666,12 @@ void __init early_init_fdt_scan_reserved_mem(void)
>>  {
>>  	int n;
>>  	u64 base, size;
>> +	bool save_only = false;
>>  
>>  	if (!initial_boot_params)
>>  		return;
>>  
>> -	fdt_scan_reserved_mem();
>> +	fdt_scan_reserved_mem(save_only);
>>  	fdt_reserve_elfcorehdr();
>>  
>>  	/* Process header /memreserve/ fields */
>> @@ -645,8 +681,6 @@ void __init early_init_fdt_scan_reserved_mem(void)
>>  			break;
>>  		memblock_reserve(base, size);
>>  	}
>> -
>> -	fdt_init_reserved_mem();
>>  }
>>  
>>  /**
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index f38397c7b582..e52b27b8392d 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -175,7 +175,6 @@ static inline struct device_node *__of_get_dma_parent(const struct device_node *
>>  }
>>  #endif
>>  
>> -void fdt_init_reserved_mem(void);
>>  void fdt_reserved_mem_save_node(unsigned long node, const char *uname,
>>  			       phys_addr_t base, phys_addr_t size);
>>  
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 7ec94cfcbddb..13e694f5e316 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -132,8 +132,7 @@ static int __init __reserved_mem_alloc_in_range(phys_addr_t size,
>>   * __reserved_mem_alloc_size() - allocate reserved memory described by
>>   *	'size', 'alignment'  and 'alloc-ranges' properties.
>>   */
>> -static int __init __reserved_mem_alloc_size(unsigned long node,
>> -	const char *uname, phys_addr_t *res_base, phys_addr_t *res_size)
>> +int __init __reserved_mem_alloc_size(unsigned long node, const char *uname)
>>  {
>>  	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
>>  	phys_addr_t start = 0, end = 0;
>> @@ -212,10 +211,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
>>  		       uname, (unsigned long)(size / SZ_1M));
>>  		return -ENOMEM;
>>  	}
>> -
>> -	*res_base = base;
>> -	*res_size = size;
>> -
>> +	fdt_reserved_mem_save_node(node, uname, base, size);
>>  	return 0;
>>  }
>>  
>> @@ -309,6 +305,9 @@ static void __init __rmem_check_for_overlap(void)
>>  void __init fdt_init_reserved_mem(void)
>>  {
>>  	int i;
>> +	bool save_only = true;
>> +
>> +	fdt_scan_reserved_mem(save_only);
> This is now called just before unflattening. Why can't it be done right 
> after unflattening instead. Then we can use the unflattened API rather 
> than the flatten API which is less efficient.
>
> Rob

Hi Rob,

My apologies for the delayed response.
I was able to implement the use of the unflattened devicetree APIs to
scan through the DT after paging_init() as per one of your comments.

Regarding your second comment, I moved the call to
fdt_init_reserved_mem() into the unflatten_device_tree() function
to remove the dependency of the DT code on the arch specific code.

This was an easy change for the arm64 arch which I am currently working
with, but some other archs needed an additional change for this to work.
I noticed that some other archs have the ordering reversed for how
paging_init() and unflatten_device_tree() are called.

These are the archs which have the call order reversed:
nios2, loongarch, mips, openrisc, powerpc, sh, um, xtensa.

Since the order in which paging_init() and fdt_init_reserved_mem() are
called is important for this series to work, moving the call to
fdt_init_reserved_mem() into the unflatten_device_tree() function would
have broken for the above mentioned architectures.

Hence, in my v2 rfc patch, I have changed the order in which
paging_init() and unflatten_device_tree() functions are called for the
above mentioned archs to make unflatten_device_tree() be called
after paging_init().

Regards,

Oreoluwa

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-12-04  4:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-19 18:48 [RFC PATCH 0/3] Dynamic allocation of reserved_mem array Oreoluwa Babatunde
2023-10-19 18:48 ` [RFC PATCH 1/3] of: reserved_mem: Change the order that reserved_mem regions are stored Oreoluwa Babatunde
2023-10-19 19:46   ` Rob Herring
2023-10-19 22:45     ` Oreoluwa Babatunde
2023-10-24 19:15       ` Rob Herring
2023-10-24 19:32   ` Rob Herring
2023-12-04  4:18     ` Oreoluwa Babatunde
2023-10-19 18:48 ` [RFC PATCH 2/3] of: reserved_mem: Add code to dynamically allocate reserved_mem array Oreoluwa Babatunde
2023-10-19 18:48 ` [RFC PATCH 3/3] of: reserved_mem: Make MAX_RESERVED_REGIONS a config option Oreoluwa Babatunde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).