On Thu, Feb 01, 2024 at 09:08:06AM -0800, Oreoluwa Babatunde wrote: > > On 1/30/2024 4:07 PM, Rob Herring wrote: > > On Fri, Jan 26, 2024 at 03:53:39PM -0800, Oreoluwa Babatunde wrote: > >> The reserved_mem array is used to store data for the different > >> reserved memory regions defined in the DT of a device. The array > >> stores information such as region name, node, start-address, and size > >> of the reserved memory regions. > >> > >> 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 extends the use of the static array for > >> reserved_mem, and introduces a dynamically allocated array using > >> memblock_alloc() based on the number of reserved memory regions > >> specified in the DT. > >> > >> Some architectures such as arm64 require the page tables to be setup > >> before memblock allocated memory is writable. Therefore, the dynamic > >> allocation of the reserved_mem array will need to be done after the > >> page tables have been setup on these architectures. In most cases that > >> will be after paging_init(). > >> > >> Reserved memory regions can be divided into 2 groups. > >> i) Statically-placed reserved memory regions > >> i.e. regions defined in the DT using the @reg property. > >> ii) Dynamically-placed reserved memory regions. > >> i.e. regions specified in the DT using the @alloc_ranges > >> and @size properties. > >> > >> It is possible to call memblock_reserve() and memblock_mark_nomap() on > >> the statically-placed reserved memory regions and not need to save them > >> to the reserved_mem array until memory is allocated for it using > >> memblock, which will be after the page tables have been setup. > >> For the dynamically-placed reserved memory regions, it is not possible > >> to wait to store its information because the starting address is > >> allocated only at run time, and hence they need to be stored somewhere > >> after they are allocated. > >> Waiting until after the page tables have been setup to allocate memory > >> for the dynamically-placed regions is also not an option because the > >> allocations will come from memory that have already been added to the > >> page tables, which is not good for memory that is supposed to be > >> reserved and/or marked as nomap. > >> > >> Therefore, this series splits up the processing of the reserved memory > >> regions into two stages, of which the first stage is carried out by > >> early_init_fdt_scan_reserved_mem() and the second is carried out by > >> fdt_init_reserved_mem(). > >> > >> The early_init_fdt_scan_reserved_mem(), which is called before the page > >> tables are setup is used to: > >> 1. Call memblock_reserve() and memblock_mark_nomap() on all the > >> statically-placed reserved memory regions as needed. > >> 2. Allocate memory from memblock for the dynamically-placed reserved > >> memory regions and store them in the static array for reserved_mem. > >> memblock_reserve() and memblock_mark_nomap() are also called as > >> needed on all the memory allocated for the dynamically-placed > >> regions. > >> 3. Count the total number of reserved memory regions found in the DT. > >> > >> fdt_init_reserved_mem(), which should be called after the page tables > >> have been setup, is used to carry out the following: > >> 1. Allocate memory for the reserved_mem array based on the number of > >> reserved memory regions counted as mentioned above. > >> 2. Copy all the information for the dynamically-placed reserved memory > >> regions from the static array into the new allocated memory for the > >> reserved_mem array. > >> 3. Add the information for the statically-placed reserved memory into > >> reserved_mem array. > >> 4. Run the region specific init functions for each of the reserve memory > >> regions saved in the reserved_mem array. > > I don't see the need for fdt_init_reserved_mem() to be explicitly called > > by arch code. I said this already, but that can be done at the same time > > as unflattening the DT. The same conditions are needed for both: we need > > to be able to allocate memory from memblock. > > > > To put it another way, if fdt_init_reserved_mem() can be called "early", > > then unflattening could be moved earlier as well. Though I don't think > > we should optimize that. I'd rather see all arches call the DT functions > > at the same stages. > Hi Rob, > > The reason we moved fdt_init_reserved_mem() back into the arch specific code > was because we realized that there was no apparently obvious way to call > early_init_fdt_scan_reserved_mem() and fdt_init_reserved_mem() in the correct > order that will work for all archs if we placed fdt_init_reserved_mem() inside the > unflatten_devicetree() function. > > early_init_fdt_scan_reserved_mem() needs to be > called first before fdt_init_reserved_mem(). But on some archs, > unflatten_devicetree() is called before early_init_fdt_scan_reserved_mem(), which > means that if we have fdt_init_reserved_mem() inside the unflatten_devicetree() > function, it will be called before early_init_fdt_scan_reserved_mem(). > > This is connected to your other comments on Patch 7 & Patch 14. > I agree, unflatten_devicetree() should NOT be getting called before we reserve > memory for the reserved memory regions because that could cause memory to be > allocated from regions that should be reserved. > > Hence, resolving this issue should allow us to call fdt_init_reserved_mem() from > the unflatten_devicetree() function without it changing the order that we are > trying to have. There's one issue I've found which is unflatten_device_tree() isn't called for ACPI case on arm64. Turns out we need /reserved-memory handled in that case too. However, I think we're going to change calling unflatten_device_tree() unconditionally for another reason[1]. [1] https://lore.kernel.org/all/efe6a7886c3491cc9c225a903efa2b1e.sboyd@xxxxxxxxxx/ > > I will work on implementing this and send another revision. I think we should go with a simpler route that's just copy the an initial array in initdata to a properly sized, allocated array like the patch below. Of course it will need some arch fixes and a follow-on patch to increase the initial array size. 8<-------------------------------------------------------------------- From: Rob Herring <robh@xxxxxxxxxx> Date: Wed, 31 Jan 2024 16:26:23 -0600 Subject: [PATCH] of: reserved-mem: Re-allocate reserved_mem array to actual size In preparation to increase the static reserved_mem array size yet again, copy the initial array to an allocated array sized based on the actual size needed. Now increasing the the size of the static reserved_mem array only eats up the initdata space. For platforms with reasonable number of reserved regions, we have a net gain in free memory. In order to do memblock allocations, fdt_init_reserved_mem() is moved a bit later to unflatten_device_tree(). On some arches this is effectively a nop. Signed-off-by: Rob Herring <robh@xxxxxxxxxx> --- RFC as this is compile tested only. This is an alternative to this series[1]. [1] https://lore.kernel.org/all/20240126235425.12233-1-quic_obabatun@xxxxxxxxxxx/ --- drivers/of/fdt.c | 4 ++-- drivers/of/of_reserved_mem.c | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index bf502ba8da95..14360f5191ae 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -645,8 +645,6 @@ void __init early_init_fdt_scan_reserved_mem(void) break; memblock_reserve(base, size); } - - fdt_init_reserved_mem(); } /** @@ -1328,6 +1326,8 @@ bool __init early_init_dt_scan(void *params) */ void __init unflatten_device_tree(void) { + fdt_init_reserved_mem(); + __unflatten_device_tree(initial_boot_params, NULL, &of_root, early_init_dt_alloc_memory_arch, false); diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 7ec94cfcbddb..ae323d6b25ad 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -27,7 +27,8 @@ #include "of_private.h" #define MAX_RESERVED_REGIONS 64 -static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; +static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS] __initdata; +static struct reserved_mem *reserved_mem_p; static int reserved_mem_count; static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, @@ -354,6 +355,13 @@ void __init fdt_init_reserved_mem(void) } } } + + reserved_mem_p = memblock_alloc(sizeof(struct reserved_mem) * reserved_mem_count, + sizeof(struct reserved_mem)); + if (WARN(!reserved_mem_p, "of: reserved-memory allocation failed, continuing with __initdata array!\n")) + reserved_mem_p = reserved_mem; + else + memcpy(reserved_mem_p, reserved_mem, sizeof(struct reserved_mem) * reserved_mem_count); } static inline struct reserved_mem *__find_rmem(struct device_node *node) @@ -364,8 +372,8 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node) return NULL; for (i = 0; i < reserved_mem_count; i++) - if (reserved_mem[i].phandle == node->phandle) - return &reserved_mem[i]; + if (reserved_mem_p[i].phandle == node->phandle) + return &reserved_mem_p[i]; return NULL; } @@ -507,8 +515,8 @@ struct reserved_mem *of_reserved_mem_lookup(struct device_node *np) name = kbasename(np->full_name); for (i = 0; i < reserved_mem_count; i++) - if (!strcmp(reserved_mem[i].name, name)) - return &reserved_mem[i]; + if (!strcmp(reserved_mem_p[i].name, name)) + return &reserved_mem_p[i]; return NULL; } -- 2.43.0
