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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 9 Apr 2024, at 14:38, Michal Orzel <michal.orzel@xxxxxxx> 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@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.

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