Re: [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory

[Michal Orzel <michal.orzel@xxxxxxx>]

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