Mailing List Archive

[PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory
The function find_unallocated_memory is using the same code to
loop through 3 structure of the same type, in order to avoid
code duplication, rework the code to have only one loop that
goes through all the structures.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- Add comment in the loop inside find_unallocated_memory to
improve readability
v1:
- new patch
---
---
xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------
1 file changed, 25 insertions(+), 45 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 57cf92668ae6..269aaff4d067 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
static int __init find_unallocated_memory(const struct kernel_info *kinfo,
struct membanks *ext_regions)
{
- const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
- const struct membanks *mem = bootinfo_get_mem();
- const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
+ const struct membanks *mem_banks[] = {
+ bootinfo_get_mem(),
+ kernel_info_get_mem_const(kinfo),
+ bootinfo_get_reserved_mem(),
+ };
struct rangeset *unalloc_mem;
paddr_t start, end;
- unsigned int i;
+ unsigned int i, j;
int res;

dt_dprintk("Find unallocated memory for extended regions\n");
@@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
if ( !unalloc_mem )
return -ENOMEM;

- /* Start with all available RAM */
- for ( i = 0; i < mem->nr_banks; i++ )
- {
- start = mem->bank[i].start;
- end = mem->bank[i].start + mem->bank[i].size;
- res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
- PFN_DOWN(end - 1));
- if ( res )
+ /*
+ * Exclude the following regions, in order:
+ * 1) Start with all available RAM
+ * 2) Remove RAM assigned to Dom0
+ * 3) Remove reserved memory
+ * The order comes from the initialization of the variable "mem_banks"
+ * above
+ */
+ for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
+ for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
{
- printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
- start, end);
- goto out;
- }
- }
-
- /* Remove RAM assigned to Dom0 */
- for ( i = 0; i < kinfo_mem->nr_banks; i++ )
- {
- start = kinfo_mem->bank[i].start;
- end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
- res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
+ start = mem_banks[i]->bank[j].start;
+ end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
+ res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
PFN_DOWN(end - 1));
- if ( res )
- {
- printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
- start, end);
- goto out;
- }
- }
-
- /* Remove reserved-memory regions */
- for ( i = 0; i < reserved_mem->nr_banks; i++ )
- {
- start = reserved_mem->bank[i].start;
- end = reserved_mem->bank[i].start + reserved_mem->bank[i].size;
- res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
- PFN_DOWN(end - 1));
- if ( res )
- {
- printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
- start, end);
- goto out;
+ if ( res )
+ {
+ printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
+ start, end);
+ goto out;
+ }
}
- }

/* Remove grant table region */
if ( kinfo->gnttab_size )
--
2.34.1
Re: [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory [ In reply to ]
Hi Luca,

On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> The function find_unallocated_memory is using the same code to
> loop through 3 structure of the same type, in order to avoid
> code duplication, rework the code to have only one loop that
> goes through all the structures.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2:
> - Add comment in the loop inside find_unallocated_memory to
> improve readability
> v1:
> - new patch
> ---
> ---
> xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------
> 1 file changed, 25 insertions(+), 45 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 57cf92668ae6..269aaff4d067 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> static int __init find_unallocated_memory(const struct kernel_info *kinfo,
> struct membanks *ext_regions)
> {
> - const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
> - const struct membanks *mem = bootinfo_get_mem();
> - const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
> + const struct membanks *mem_banks[] = {
> + bootinfo_get_mem(),
> + kernel_info_get_mem_const(kinfo),
> + bootinfo_get_reserved_mem(),
> + };
> struct rangeset *unalloc_mem;
> paddr_t start, end;
> - unsigned int i;
> + unsigned int i, j;
> int res;
>
> dt_dprintk("Find unallocated memory for extended regions\n");
> @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
> if ( !unalloc_mem )
> return -ENOMEM;
>
> - /* Start with all available RAM */
> - for ( i = 0; i < mem->nr_banks; i++ )
> - {
> - start = mem->bank[i].start;
> - end = mem->bank[i].start + mem->bank[i].size;
> - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
> - PFN_DOWN(end - 1));
> - if ( res )
> + /*
> + * Exclude the following regions, in order:
> + * 1) Start with all available RAM
> + * 2) Remove RAM assigned to Dom0
> + * 3) Remove reserved memory
Given this commit and the previous code, I expect one call to rangeset_add_range() and
3 calls to rangeset_remove_range(). However ...
> + * The order comes from the initialization of the variable "mem_banks"
> + * above
> + */
> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
> + for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
> {
> - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
> - start, end);
> - goto out;
> - }
> - }
> -
> - /* Remove RAM assigned to Dom0 */
> - for ( i = 0; i < kinfo_mem->nr_banks; i++ )
> - {
> - start = kinfo_mem->bank[i].start;
> - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
> - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
> + start = mem_banks[i]->bank[j].start;
> + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
> + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
... here you always call rangeset_add_range() which is wrong. For direct mapped domain
you would e.g. register its RAM region as extended region.

~Michal
Re: [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory [ In reply to ]
> On 9 Apr 2024, at 14:38, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Hi Luca,
>
> On 09/04/2024 13:45, Luca Fancellu wrote:
>>
>>
>> The function find_unallocated_memory is using the same code to
>> loop through 3 structure of the same type, in order to avoid
>> code duplication, rework the code to have only one loop that
>> goes through all the structures.
>>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v2:
>> - Add comment in the loop inside find_unallocated_memory to
>> improve readability
>> v1:
>> - new patch
>> ---
>> ---
>> xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------
>> 1 file changed, 25 insertions(+), 45 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 57cf92668ae6..269aaff4d067 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>> static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>> struct membanks *ext_regions)
>> {
>> - const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
>> - const struct membanks *mem = bootinfo_get_mem();
>> - const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
>> + const struct membanks *mem_banks[] = {
>> + bootinfo_get_mem(),
>> + kernel_info_get_mem_const(kinfo),
>> + bootinfo_get_reserved_mem(),
>> + };
>> struct rangeset *unalloc_mem;
>> paddr_t start, end;
>> - unsigned int i;
>> + unsigned int i, j;
>> int res;
>>
>> dt_dprintk("Find unallocated memory for extended regions\n");
>> @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>> if ( !unalloc_mem )
>> return -ENOMEM;
>>
>> - /* Start with all available RAM */
>> - for ( i = 0; i < mem->nr_banks; i++ )
>> - {
>> - start = mem->bank[i].start;
>> - end = mem->bank[i].start + mem->bank[i].size;
>> - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
>> - PFN_DOWN(end - 1));
>> - if ( res )
>> + /*
>> + * Exclude the following regions, in order:
>> + * 1) Start with all available RAM
>> + * 2) Remove RAM assigned to Dom0
>> + * 3) Remove reserved memory
> Given this commit and the previous code, I expect one call to rangeset_add_range() and
> 3 calls to rangeset_remove_range(). However ...
>> + * The order comes from the initialization of the variable "mem_banks"
>> + * above
>> + */
>> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
>> + for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
>> {
>> - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
>> - start, end);
>> - goto out;
>> - }
>> - }
>> -
>> - /* Remove RAM assigned to Dom0 */
>> - for ( i = 0; i < kinfo_mem->nr_banks; i++ )
>> - {
>> - start = kinfo_mem->bank[i].start;
>> - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
>> - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
>> + start = mem_banks[i]->bank[j].start;
>> + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
>> + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
> ... here you always call rangeset_add_range() which is wrong. For direct mapped domain
> you would e.g. register its RAM region as extended region.

Right, I read it wrong initially, my mistake, here we are adding all available ram and later removing the dom0 regions
and reserved regions. Will fix

>
> ~Michal