| Message ID | 20240308191204.819487-3-quic_obabatun@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Dynamic Allocation of the reserved_mem array | expand |
On Fri, Mar 08, 2024 at 11:12:02AM -0800, Oreoluwa Babatunde wrote: > The reserved_mem array is statically allocated with a size of > MAX_RESERVED_REGIONS(64). Therefore, if the number of reserved_mem > regions exceeds this size, there will not be enough space to store > all the data. > > Hence, extend the use of the static array by introducing a > dynamically allocated array based on the number of reserved memory > regions specified in the DT. > > On architectures such as arm64, memblock allocated memory is not > writable until after the page tables have been setup. Hence, the > dynamic allocation of the reserved_mem array will need to be done only > after the page tables have been setup. > > As a result, a temporary static array is still needed in the initial > stages to store the information of the dynamically-placed reserved memory > regions because the start address is selected only at run-time and is not > stored anywhere else. > It is not possible to wait until the reserved_mem array is allocated > because this is done after the page tables are setup and the reserved > memory regions need to be initialized before then. > > After the reserved_mem array is allocated, all entries from the static > array is copied over to the new array, and the rest of the information > for the statically-placed reserved memory regions are read in from the > DT and stored in the new array as well. > > Once the init process is completed, the temporary static array is > released back to the system because it is no longer needed. This is > achieved by marking it as __initdata. > > Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com> > --- > drivers/of/fdt.c | 15 +++++++--- > drivers/of/of_private.h | 1 + > drivers/of/of_reserved_mem.c | 53 ++++++++++++++++++++++++++++++++++-- > 3 files changed, 62 insertions(+), 7 deletions(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index fe6c75c5a8c0..2468360d6053 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -614,13 +614,15 @@ void __init fdt_scan_reserved_mem_reg_nodes(void) > } > } > > +int total_reserved_mem_cnt = MAX_RESERVED_REGIONS; > + Put this in of_reserved_mem.c. > /* > * 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_cnt = 0, count = 0; > int dynamic_nodes[MAX_RESERVED_REGIONS]; > const void *fdt = initial_boot_params; > > @@ -643,6 +645,8 @@ static int __init fdt_scan_reserved_mem(void) > uname = fdt_get_name(fdt, child, NULL); > > err = __reserved_mem_reserve_reg(child, uname); > + if (!err) > + count++; > > /* > * Delay allocation of the dynamically-placed regions > @@ -657,12 +661,16 @@ static int __init fdt_scan_reserved_mem(void) > > for (int i = 0; i < dynamic_nodes_cnt; i++) { > const char *uname; > + int err; > > child = dynamic_nodes[i]; > uname = fdt_get_name(fdt, child, NULL); > > - __reserved_mem_alloc_size(child, uname); > + err = __reserved_mem_alloc_size(child, uname); > + if (!err) > + count++; > } > + total_reserved_mem_cnt = count; > return 0; > } > > @@ -715,8 +723,6 @@ void __init early_init_fdt_scan_reserved_mem(void) > break; > memblock_reserve(base, size); > } > - > - fdt_init_reserved_mem(); > } > > /** > @@ -1405,6 +1411,7 @@ void __init unflatten_device_tree(void) > of_alias_scan(early_init_dt_alloc_memory_arch); > > unittest_unflatten_overlay_base(); > + fdt_init_reserved_mem(); This change belongs in patch 1. > } > > /** > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h > index 542e37a37a24..447b63413b39 100644 > --- a/drivers/of/of_private.h > +++ b/drivers/of/of_private.h > @@ -42,6 +42,7 @@ extern struct mutex of_mutex; > extern raw_spinlock_t devtree_lock; > extern struct list_head aliases_lookup; > extern struct kset *of_kset; > +extern int total_reserved_mem_cnt; > > #if defined(CONFIG_OF_DYNAMIC) > extern int of_property_notify(int action, struct device_node *np, > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c > index d62f1956024c..3c4373b021be 100644 > --- a/drivers/of/of_reserved_mem.c > +++ b/drivers/of/of_reserved_mem.c > @@ -26,7 +26,8 @@ > > #include "of_private.h" > > -static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; > +static struct reserved_mem reserved_mem_array[MAX_RESERVED_REGIONS] __initdata; > +static struct reserved_mem *reserved_mem __refdata = reserved_mem_array; > static int reserved_mem_count; > > static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, > @@ -54,6 +55,48 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, > return err; > } > > +/** > + * alloc_reserved_mem_array() - allocate memory for the reserved_mem > + * array using memblock > + * > + * This function is used to allocate memory for the reserved_mem array > + * according to the total number of reserved memory regions defined in > + * the DT. > + * After the new array is allocated, the information stored in the > + * initial static array is copied over to this new array and the > + * new array is used from this point on. > + */ > +static int __init alloc_reserved_mem_array(void) > +{ > + struct reserved_mem *new_array; > + size_t alloc_size, copy_size, memset_size; > + > + alloc_size = array_size(total_reserved_mem_cnt, sizeof(*new_array)); > + if (alloc_size == SIZE_MAX) > + return -1; Use EOVERFLOW > + > + new_array = memblock_alloc(alloc_size, SMP_CACHE_BYTES); > + if (!new_array) > + return -ENOMEM; > + > + copy_size = array_size(reserved_mem_count, sizeof(*new_array)); > + if (copy_size == SIZE_MAX) > + goto overlow_err; This is the only path for goto, so move the cleanup here. > + > + memset_size = alloc_size - copy_size; > + > + memcpy(new_array, reserved_mem, copy_size); > + memset(new_array + reserved_mem_count, 0, memset_size); > + > + reserved_mem = new_array; > + return 0; > + > +overlow_err: > + memblock_free(new_array, alloc_size); > + total_reserved_mem_cnt = MAX_RESERVED_REGIONS; > + return -1; Use EOVERFLOW > +} > + > /* > * fdt_reserved_mem_save_node() - save fdt node for second pass initialization > */ > @@ -62,7 +105,7 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname, > { > struct reserved_mem *rmem = &reserved_mem[reserved_mem_count]; > > - if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) { > + if (reserved_mem_count == total_reserved_mem_cnt) { > pr_err("not enough space for all defined regions.\n"); > return; > } > @@ -303,7 +346,11 @@ static void __init __rmem_check_for_overlap(void) > */ > void __init fdt_init_reserved_mem(void) > { > - int i; > + int i, ret; > + > + ret = alloc_reserved_mem_array(); > + if (ret) > + pr_err("Failed to allocate memory for reserved_mem array with err: %d", ret); As printing a message is the only error handling, better to just print something in alloc_reserved_mem_array() and return void. > > fdt_scan_reserved_mem_reg_nodes(); > > -- > 2.34.1 >
On 3/11/2024 9:55 AM, Rob Herring wrote: > On Fri, Mar 08, 2024 at 11:12:02AM -0800, Oreoluwa Babatunde wrote: >> The reserved_mem array is statically allocated with a size of >> MAX_RESERVED_REGIONS(64). Therefore, if the number of reserved_mem >> regions exceeds this size, there will not be enough space to store >> all the data. >> >> Hence, extend the use of the static array by introducing a >> dynamically allocated array based on the number of reserved memory >> regions specified in the DT. >> >> On architectures such as arm64, memblock allocated memory is not >> writable until after the page tables have been setup. Hence, the >> dynamic allocation of the reserved_mem array will need to be done only >> after the page tables have been setup. >> >> As a result, a temporary static array is still needed in the initial >> stages to store the information of the dynamically-placed reserved memory >> regions because the start address is selected only at run-time and is not >> stored anywhere else. >> It is not possible to wait until the reserved_mem array is allocated >> because this is done after the page tables are setup and the reserved >> memory regions need to be initialized before then. >> >> After the reserved_mem array is allocated, all entries from the static >> array is copied over to the new array, and the rest of the information >> for the statically-placed reserved memory regions are read in from the >> DT and stored in the new array as well. >> >> Once the init process is completed, the temporary static array is >> released back to the system because it is no longer needed. This is >> achieved by marking it as __initdata. >> >> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com> >> --- >> drivers/of/fdt.c | 15 +++++++--- >> drivers/of/of_private.h | 1 + >> drivers/of/of_reserved_mem.c | 53 ++++++++++++++++++++++++++++++++++-- >> 3 files changed, 62 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index fe6c75c5a8c0..2468360d6053 100644 >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -614,13 +614,15 @@ void __init fdt_scan_reserved_mem_reg_nodes(void) >> } >> } >> >> +int total_reserved_mem_cnt = MAX_RESERVED_REGIONS; >> + > Put this in of_reserved_mem.c. > >> /* >> * 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_cnt = 0, count = 0; >> int dynamic_nodes[MAX_RESERVED_REGIONS]; >> const void *fdt = initial_boot_params; >> >> @@ -643,6 +645,8 @@ static int __init fdt_scan_reserved_mem(void) >> uname = fdt_get_name(fdt, child, NULL); >> >> err = __reserved_mem_reserve_reg(child, uname); >> + if (!err) >> + count++; >> >> /* >> * Delay allocation of the dynamically-placed regions >> @@ -657,12 +661,16 @@ static int __init fdt_scan_reserved_mem(void) >> >> for (int i = 0; i < dynamic_nodes_cnt; i++) { >> const char *uname; >> + int err; >> >> child = dynamic_nodes[i]; >> uname = fdt_get_name(fdt, child, NULL); >> >> - __reserved_mem_alloc_size(child, uname); >> + err = __reserved_mem_alloc_size(child, uname); >> + if (!err) >> + count++; >> } >> + total_reserved_mem_cnt = count; >> return 0; >> } >> >> @@ -715,8 +723,6 @@ void __init early_init_fdt_scan_reserved_mem(void) >> break; >> memblock_reserve(base, size); >> } >> - >> - fdt_init_reserved_mem(); >> } >> >> /** >> @@ -1405,6 +1411,7 @@ void __init unflatten_device_tree(void) >> of_alias_scan(early_init_dt_alloc_memory_arch); >> >> unittest_unflatten_overlay_base(); >> + fdt_init_reserved_mem(); > This change belongs in patch 1. > >> } >> >> /** >> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h >> index 542e37a37a24..447b63413b39 100644 >> --- a/drivers/of/of_private.h >> +++ b/drivers/of/of_private.h >> @@ -42,6 +42,7 @@ extern struct mutex of_mutex; >> extern raw_spinlock_t devtree_lock; >> extern struct list_head aliases_lookup; >> extern struct kset *of_kset; >> +extern int total_reserved_mem_cnt; >> >> #if defined(CONFIG_OF_DYNAMIC) >> extern int of_property_notify(int action, struct device_node *np, >> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c >> index d62f1956024c..3c4373b021be 100644 >> --- a/drivers/of/of_reserved_mem.c >> +++ b/drivers/of/of_reserved_mem.c >> @@ -26,7 +26,8 @@ >> >> #include "of_private.h" >> >> -static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; >> +static struct reserved_mem reserved_mem_array[MAX_RESERVED_REGIONS] __initdata; >> +static struct reserved_mem *reserved_mem __refdata = reserved_mem_array; >> static int reserved_mem_count; >> >> static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, >> @@ -54,6 +55,48 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, >> return err; >> } >> >> +/** >> + * alloc_reserved_mem_array() - allocate memory for the reserved_mem >> + * array using memblock >> + * >> + * This function is used to allocate memory for the reserved_mem array >> + * according to the total number of reserved memory regions defined in >> + * the DT. >> + * After the new array is allocated, the information stored in the >> + * initial static array is copied over to this new array and the >> + * new array is used from this point on. >> + */ >> +static int __init alloc_reserved_mem_array(void) >> +{ >> + struct reserved_mem *new_array; >> + size_t alloc_size, copy_size, memset_size; >> + >> + alloc_size = array_size(total_reserved_mem_cnt, sizeof(*new_array)); >> + if (alloc_size == SIZE_MAX) >> + return -1; > Use EOVERFLOW > >> + >> + new_array = memblock_alloc(alloc_size, SMP_CACHE_BYTES); >> + if (!new_array) >> + return -ENOMEM; >> + >> + copy_size = array_size(reserved_mem_count, sizeof(*new_array)); >> + if (copy_size == SIZE_MAX) >> + goto overlow_err; > This is the only path for goto, so move the cleanup here. > >> + >> + memset_size = alloc_size - copy_size; >> + >> + memcpy(new_array, reserved_mem, copy_size); >> + memset(new_array + reserved_mem_count, 0, memset_size); >> + >> + reserved_mem = new_array; >> + return 0; >> + >> +overlow_err: >> + memblock_free(new_array, alloc_size); >> + total_reserved_mem_cnt = MAX_RESERVED_REGIONS; >> + return -1; > Use EOVERFLOW > >> +} >> + >> /* >> * fdt_reserved_mem_save_node() - save fdt node for second pass initialization >> */ >> @@ -62,7 +105,7 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname, >> { >> struct reserved_mem *rmem = &reserved_mem[reserved_mem_count]; >> >> - if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) { >> + if (reserved_mem_count == total_reserved_mem_cnt) { >> pr_err("not enough space for all defined regions.\n"); >> return; >> } >> @@ -303,7 +346,11 @@ static void __init __rmem_check_for_overlap(void) >> */ >> void __init fdt_init_reserved_mem(void) >> { >> - int i; >> + int i, ret; >> + >> + ret = alloc_reserved_mem_array(); >> + if (ret) >> + pr_err("Failed to allocate memory for reserved_mem array with err: %d", ret); > As printing a message is the only error handling, better to just print > something in alloc_reserved_mem_array() and return void. > >> >> fdt_scan_reserved_mem_reg_nodes(); >> >> -- >> 2.34.1 Hi Rob, Thank you for the feedback. I'll implement all of this in the next revision. Regards, Oreoluwa
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index fe6c75c5a8c0..2468360d6053 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -614,13 +614,15 @@ void __init fdt_scan_reserved_mem_reg_nodes(void) } } +int total_reserved_mem_cnt = MAX_RESERVED_REGIONS; + /* * 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_cnt = 0, count = 0; int dynamic_nodes[MAX_RESERVED_REGIONS]; const void *fdt = initial_boot_params; @@ -643,6 +645,8 @@ static int __init fdt_scan_reserved_mem(void) uname = fdt_get_name(fdt, child, NULL); err = __reserved_mem_reserve_reg(child, uname); + if (!err) + count++; /* * Delay allocation of the dynamically-placed regions @@ -657,12 +661,16 @@ static int __init fdt_scan_reserved_mem(void) for (int i = 0; i < dynamic_nodes_cnt; i++) { const char *uname; + int err; child = dynamic_nodes[i]; uname = fdt_get_name(fdt, child, NULL); - __reserved_mem_alloc_size(child, uname); + err = __reserved_mem_alloc_size(child, uname); + if (!err) + count++; } + total_reserved_mem_cnt = count; return 0; } @@ -715,8 +723,6 @@ void __init early_init_fdt_scan_reserved_mem(void) break; memblock_reserve(base, size); } - - fdt_init_reserved_mem(); } /** @@ -1405,6 +1411,7 @@ void __init unflatten_device_tree(void) of_alias_scan(early_init_dt_alloc_memory_arch); unittest_unflatten_overlay_base(); + fdt_init_reserved_mem(); } /** diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h index 542e37a37a24..447b63413b39 100644 --- a/drivers/of/of_private.h +++ b/drivers/of/of_private.h @@ -42,6 +42,7 @@ extern struct mutex of_mutex; extern raw_spinlock_t devtree_lock; extern struct list_head aliases_lookup; extern struct kset *of_kset; +extern int total_reserved_mem_cnt; #if defined(CONFIG_OF_DYNAMIC) extern int of_property_notify(int action, struct device_node *np, diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index d62f1956024c..3c4373b021be 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -26,7 +26,8 @@ #include "of_private.h" -static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; +static struct reserved_mem reserved_mem_array[MAX_RESERVED_REGIONS] __initdata; +static struct reserved_mem *reserved_mem __refdata = reserved_mem_array; static int reserved_mem_count; static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, @@ -54,6 +55,48 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, return err; } +/** + * alloc_reserved_mem_array() - allocate memory for the reserved_mem + * array using memblock + * + * This function is used to allocate memory for the reserved_mem array + * according to the total number of reserved memory regions defined in + * the DT. + * After the new array is allocated, the information stored in the + * initial static array is copied over to this new array and the + * new array is used from this point on. + */ +static int __init alloc_reserved_mem_array(void) +{ + struct reserved_mem *new_array; + size_t alloc_size, copy_size, memset_size; + + alloc_size = array_size(total_reserved_mem_cnt, sizeof(*new_array)); + if (alloc_size == SIZE_MAX) + return -1; + + new_array = memblock_alloc(alloc_size, SMP_CACHE_BYTES); + if (!new_array) + return -ENOMEM; + + copy_size = array_size(reserved_mem_count, sizeof(*new_array)); + if (copy_size == SIZE_MAX) + goto overlow_err; + + memset_size = alloc_size - copy_size; + + memcpy(new_array, reserved_mem, copy_size); + memset(new_array + reserved_mem_count, 0, memset_size); + + reserved_mem = new_array; + return 0; + +overlow_err: + memblock_free(new_array, alloc_size); + total_reserved_mem_cnt = MAX_RESERVED_REGIONS; + return -1; +} + /* * fdt_reserved_mem_save_node() - save fdt node for second pass initialization */ @@ -62,7 +105,7 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname, { struct reserved_mem *rmem = &reserved_mem[reserved_mem_count]; - if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) { + if (reserved_mem_count == total_reserved_mem_cnt) { pr_err("not enough space for all defined regions.\n"); return; } @@ -303,7 +346,11 @@ static void __init __rmem_check_for_overlap(void) */ void __init fdt_init_reserved_mem(void) { - int i; + int i, ret; + + ret = alloc_reserved_mem_array(); + if (ret) + pr_err("Failed to allocate memory for reserved_mem array with err: %d", ret); fdt_scan_reserved_mem_reg_nodes();
The reserved_mem array is statically allocated with a size of MAX_RESERVED_REGIONS(64). Therefore, if the number of reserved_mem regions exceeds this size, there will not be enough space to store all the data. Hence, extend the use of the static array by introducing a dynamically allocated array based on the number of reserved memory regions specified in the DT. On architectures such as arm64, memblock allocated memory is not writable until after the page tables have been setup. Hence, the dynamic allocation of the reserved_mem array will need to be done only after the page tables have been setup. As a result, a temporary static array is still needed in the initial stages to store the information of the dynamically-placed reserved memory regions because the start address is selected only at run-time and is not stored anywhere else. It is not possible to wait until the reserved_mem array is allocated because this is done after the page tables are setup and the reserved memory regions need to be initialized before then. After the reserved_mem array is allocated, all entries from the static array is copied over to the new array, and the rest of the information for the statically-placed reserved memory regions are read in from the DT and stored in the new array as well. Once the init process is completed, the temporary static array is released back to the system because it is no longer needed. This is achieved by marking it as __initdata. Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com> --- drivers/of/fdt.c | 15 +++++++--- drivers/of/of_private.h | 1 + drivers/of/of_reserved_mem.c | 53 ++++++++++++++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 7 deletions(-)