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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 20/03/2024 15:53, Luca Fancellu wrote:
> 
> 
>> On 20 Mar 2024, at 10:57, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>
>> Hi Luca,
>>
>> On 12/03/2024 14:03, 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>
>>> ---
>>> xen/arch/arm/domain_build.c | 62 ++++++++++---------------------------
>>> 1 file changed, 17 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index b254f252e7cb..d0f2ac6060eb 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(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(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,20 @@ 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 )
>>> -        {
>>> -            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),
>>> -                                    PFN_DOWN(end - 1));
>>> -        if ( res )
>>> +    for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
>>> +        for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
>> It might be a matter of personal opinion, but I would actually prefer the 
>> current code
>> that looks simpler/neater (the steps are clear) to me. I'd like to know 
>> other maintainers opinion.
> 
> Ok, I’ll wait the other maintainers then, I’m going to use this construct in 
> other part
> of the code to simplify and remove duplication so it would be important to 
> know if
> It’s desirable or not.
> 
> Maybe your opinion could change with a proper comment on top of the structure 
> and the loop,
> listing the operation performed in order?
> 
> 1) Start with all available RAM
> 2) Remove RAM assigned to Dom0
> 3) Remove reserved mem
> <later>
> 4) Remove static shared memory regions
Yes, that would help with the overall readability.

~Michal