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@kernel.org/
>
> 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 at kernel.org>
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 at kernel.org>
---
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@quicinc.com/
---
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