On 3/11/2024 10:13 AM, Rob Herring wrote: > On Fri, Mar 08, 2024 at 11:12:01AM -0800, Oreoluwa Babatunde wrote: >> The current implementation processes the reserved memory regions in two >> stages which are done with two separate functions within the >> early_init_fdt_scan_reserved_mem() function. >> >> Within the two stages of processing, the reserved memory regions are >> broken up into two groups which are processed differently: >> i) Statically-placed reserved memory regions >> i.e. regions defined with a static start address and size using the >> "reg" property in the DT. >> ii) Dynamically-placed reserved memory regions. >> i.e. regions defined by specifying a range of addresses where they can >> be placed in memory using the "alloc_ranges" and "size" properties >> in the DT. >> >> Stage 1: fdt_scan_reserved_mem() >> This stage of the reserved memory processing is used to scan through the >> reserved memory nodes defined in the devicetree and do the following on >> each of the nodes: >> >> 1) If the node represents a statically-placed reserved memory region, >> i.e. it is defined using the "reg" property: >> - Call memblock_reserve() or memblock_mark_nomap() as needed. >> - Add the information for the reserved region to the reserved_mem array. >> eg: fdt_reserved_mem_save_node(node, name, base, size); >> >> 2) If the node represents a dynamically-placed reserved memory region, >> i.e. it is defined using "alloc-ranges" and "size" properties: >> - Add the information for the region to the reserved_mem array with >> the starting address and size set to 0. >> eg: fdt_reserved_mem_save_node(node, name, 0, 0); >> >> Stage 2: fdt_init_reserved_mem() >> This stage of the reserved memory processing is used to iterate through >> the reserved_mem array which was populated in stage 1 and do the >> following on each of the entries: >> >> 1) If the entry represents a statically-placed reserved memory region: >> - Call the region specific init function. >> 2) If the entry represents a dynamically-placed reserved memory region: >> - Call __reserved_mem_alloc_size() which is used to allocate memory >> for the region using memblock_phys_alloc_range(), and call >> memblock_mark_nomap() on the allocated region if the region is >> specified as a no-map region. >> - Call the region specific init function. >> >> On architectures such as arm64, the dynamic allocation of the >> reserved_mem array needs to be done after the page tables have been >> setup because memblock allocated memory is not writable until then. This >> means that the reserved_mem array will not be available to store any >> reserved memory information until after the page tables have been setup. >> >> 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 later. This is because all the >> information we need is present in the devicetree. >> Dynamically-placed reserved memory regions on the other hand get assigned >> a start address only at runtime, and since memblock_reserve() and >> memblock_mark_nomap() need to be called before the memory mappings are >> created, the allocation needs to happen before the page tables are setup. >> >> To make it easier to handle dynamically-placed reserved memory regions >> before the page tables are setup, this patch makes changes to the steps >> above to process the reserved memory regions in the following ways: >> >> Step 1: fdt_scan_reserved_mem() >> This stage of the reserved memory processing is used to scan through the >> reserved memory nodes defined in the devicetree and do the following on >> each of the nodes: >> >> 1) If the node represents a statically-placed reserved memory region, >> i.e. it is defined using the "reg" property: >> - Call memblock_reserve() or memblock_mark_nomap() as needed. >> >> 2) If the node represents a dynamically-placed reserved memory region, >> i.e. it is defined using "alloc-ranges" and "size" properties: >> - Call __reserved_mem_alloc_size() which will: >> i) Allocate memory for the reserved memory region. >> ii) Call memblock_mark_nomap() as needed. >> Note: There is no need to explicitly call memblock_reserve() here >> because it is already called by memblock when the memory for the >> region is being allocated. >> iii) Save the information for the region in the reserved_mem array. >> >> Step 2: fdt_init_reserved_mem() >> This stage of the reserved memory processing is used to: >> >> 1) Add the information for the statically-placed reserved memory into >> the reserved_mem array. >> >> 2) Iterate through all the entries in the array and call the region >> specific init function for each of them. >> >> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@xxxxxxxxxxx> >> --- >> drivers/of/fdt.c | 84 ++++++++++++++++++++++++++++++--- >> drivers/of/of_private.h | 2 +- >> drivers/of/of_reserved_mem.c | 54 +++++++++------------ >> include/linux/of_fdt.h | 1 + >> include/linux/of_reserved_mem.h | 9 ++++ >> 5 files changed, 111 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index bf502ba8da95..fe6c75c5a8c0 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; >> } >> @@ -563,12 +558,70 @@ static int __init __reserved_mem_check_root(unsigned long node) >> return 0; >> } >> >> +/** >> + * fdt_scan_reserved_mem_reg_nodes() - Store info for the "reg" defined >> + * reserved memory regions. >> + * >> + * This function is used to scan through the DT and store the >> + * information for the reserved memory regions that are defined using >> + * the "reg" property. The region node number, name, base address, and >> + * size are all stored in the reserved_mem array by calling the >> + * fdt_reserved_mem_save_node() function. >> + */ >> +void __init fdt_scan_reserved_mem_reg_nodes(void) >> + >> +{ >> + int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32); >> + const void *fdt = initial_boot_params; >> + phys_addr_t base, size; >> + const __be32 *prop; >> + int node, child; >> + int len; >> + >> + node = fdt_path_offset(fdt, "/reserved-memory"); >> + if (node < 0) { >> + pr_err("Reserved memory: Did not find reserved-memory node\n"); > No reserved regions is perfectly valid. ack >> + return; >> + } >> + >> + if (__reserved_mem_check_root(node)) { >> + pr_err("Reserved memory: unsupported node format, ignoring\n"); >> + return; >> + } >> + >> + fdt_for_each_subnode(child, fdt, node) { >> + const char *uname; >> + >> + prop = of_get_flat_dt_prop(child, "reg", &len); >> + if (!prop) >> + continue; >> + >> + if (!of_fdt_device_is_available(fdt, child)) >> + continue; >> + >> + uname = fdt_get_name(fdt, child, NULL); >> + if (len && len % t_len != 0) { >> + pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n", >> + uname); >> + continue; >> + } >> + >> + 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(child, uname, base, size); >> + } >> +} >> + >> /* >> * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory >> */ >> static int __init fdt_scan_reserved_mem(void) >> { >> int node, child; >> + int dynamic_nodes_cnt = 0; >> + int dynamic_nodes[MAX_RESERVED_REGIONS]; >> const void *fdt = initial_boot_params; >> >> node = fdt_path_offset(fdt, "/reserved-memory"); >> @@ -590,8 +643,25 @@ static int __init fdt_scan_reserved_mem(void) >> uname = fdt_get_name(fdt, child, NULL); >> >> 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); >> + >> + /* >> + * Delay allocation of the dynamically-placed regions >> + * until after all other statically-placed regions have >> + * been reserved or marked as nomap >> + */ >> + if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL)) { >> + dynamic_nodes[dynamic_nodes_cnt] = child; >> + dynamic_nodes_cnt++; > Can't you just call __reserved_mem_alloc_size() here instead of looping > twice? The reason for looping twice here is in the comment right above the if-statement; "Delay allocation of the dynamically-placed regions until after all other statically-placed regions have been reserved or marked as nomap". If we call "__reserved_mem_alloc_size()" at this point then there is a possibility of allocating memory from one of the statically-placed reserved memory regions since not all of them have been marked as reserved or nomap yet. >> + } >> + } >> + >> + for (int i = 0; i < dynamic_nodes_cnt; i++) { >> + const char *uname; >> + >> + child = dynamic_nodes[i]; >> + uname = fdt_get_name(fdt, child, NULL); >> + >> + __reserved_mem_alloc_size(child, uname); >> } >> return 0; >> } >> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h >> index f38397c7b582..542e37a37a24 100644 >> --- a/drivers/of/of_private.h >> +++ b/drivers/of/of_private.h >> @@ -36,6 +36,7 @@ struct alias_prop { >> #endif >> >> #define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1 >> +#define MAX_RESERVED_REGIONS 64 >> >> extern struct mutex of_mutex; >> extern raw_spinlock_t devtree_lock; >> @@ -175,7 +176,6 @@ static inline struct device_node *__of_get_dma_parent(const struct device_node * >> } >> #endif >> >> -void fdt_init_reserved_mem(void); > I don't see why this is moved. True, there is no need for this anymore. Will revert it in the next revision. >> [...] >> >> @@ -328,30 +325,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); > Can be 1 line now. ack. >> + if (nomap) >> + memblock_clear_nomap(rmem->base, rmem->size); >> + else >> + memblock_phys_free(rmem->base, >> + rmem->size); > Can be 1 line now. ack. >> + } 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..7b2a5d93d719 100644 >> --- a/include/linux/of_fdt.h >> +++ b/include/linux/of_fdt.h >> @@ -73,6 +73,7 @@ extern int early_init_dt_scan_root(void); >> extern bool early_init_dt_scan(void *params); >> extern bool early_init_dt_verify(void *params); >> extern void early_init_dt_scan_nodes(void); >> +extern void fdt_scan_reserved_mem_reg_nodes(void); > This is internal to drivers/of/, so it goes in of_private.h ack. >> >> extern const char *of_flat_dt_get_machine_name(void); >> extern const void *of_flat_dt_match_machine(const void *default_match, >> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h >> index 4de2a24cadc9..2a3178920bae 100644 >> --- a/include/linux/of_reserved_mem.h >> +++ b/include/linux/of_reserved_mem.h >> @@ -32,12 +32,14 @@ typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem); >> #define RESERVEDMEM_OF_DECLARE(name, compat, init) \ >> _OF_DECLARE(reservedmem, name, compat, init, reservedmem_of_init_fn) >> >> +void fdt_init_reserved_mem(void); >> int of_reserved_mem_device_init_by_idx(struct device *dev, >> struct device_node *np, int idx); >> 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); >> +int __reserved_mem_alloc_size(unsigned long node, const char *uname); > This is internal to drivers/of/, so it goes in of_private.h ack. > > But really, I think fdt_scan_reserved_mem() should move to > of_reserved_mem.c first. Then everything you add to fdt.c goes there > too. ack. Regards, Oreoluwa