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@xxxxxxxxxxx>
>> ---
>> 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
