Hi Luca, On 20/03/2024 15:53, Luca Fancellu wrote: > > >> On 20 Mar 2024, at 10:57, Michal Orzel <[email protected]> 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 <[email protected]> >>> --- >>> 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
