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