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 <[email protected]>
---
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
On Thu, Oct 19, 2023 at 1:49 PM Oreoluwa Babatunde
<[email protected]> 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 <[email protected]>
> ---
> 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
On 10/19/2023 12:46 PM, Rob Herring wrote:
> On Thu, Oct 19, 2023 at 1:49 PM Oreoluwa Babatunde
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
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
> > <[email protected]> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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