Re: [PATCH v2] xen/arm: Fully initialise struct membanks_hdr fields

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

> On 9 Jan 2025, at 10:00, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> Is this patch for 4.20? I would say so, therefore it should have "for-4.20" 
> tag and Oleksii as release manager
> should be CCed. Doing it now.

Thanks, I forgot the procedure

> 
>> ---
>> ---
>> xen/arch/arm/domain_build.c       | 13 ++++---------
>> xen/arch/arm/include/asm/kernel.h |  5 ++++-
>> xen/arch/arm/static-shmem.c       |  3 ++-
>> xen/include/xen/bootfdt.h         | 16 ++++++++++++++++
>> 4 files changed, 26 insertions(+), 11 deletions(-)
>> 
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index b072a16249fe..9e3132fb21d8 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1039,7 +1039,7 @@ void __init allocate_memory(struct domain *d, struct 
>> kernel_info *kinfo)
>>      */
>>     if ( is_hardware_domain(d) )
>>     {
>> -        struct membanks *gnttab = xzalloc_flex_struct(struct membanks, 
>> bank, 1);
>> +        struct membanks *gnttab = membanks_xzalloc(1, RESERVED_MEMORY);
>>         /*
>>          * Exclude the following regions:
>>          * 1) Remove reserved memory
>> @@ -1057,13 +1057,10 @@ void __init allocate_memory(struct domain *d, struct 
>> kernel_info *kinfo)
>>         gnttab->bank[0].start = kinfo->gnttab_start;
>>         gnttab->bank[0].size = kinfo->gnttab_size;
>> 
>> -        hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
>> -                                             NR_MEM_BANKS);
>> +        hwdom_free_mem = membanks_xzalloc(NR_MEM_BANKS, MEMORY);
>>         if ( !hwdom_free_mem )
>>             goto fail;
>> 
>> -        hwdom_free_mem->max_banks = NR_MEM_BANKS;
>> -
>>         if ( find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
>>                                      hwdom_free_mem, add_hwdom_free_regions) 
>> )
>>             goto fail;
>> @@ -1293,7 +1290,7 @@ static int __init find_host_extended_regions(const 
>> struct kernel_info *kinfo,
>>                                              struct membanks *ext_regions)
>> {
>>     int res;
>> -    struct membanks *gnttab = xzalloc_flex_struct(struct membanks, bank, 1);
>> +    struct membanks *gnttab = membanks_xzalloc(1, RESERVED_MEMORY);
>> 
>>     /*
>>      * Exclude the following regions:
>> @@ -1374,12 +1371,10 @@ int __init make_hypervisor_node(struct domain *d,
>>     }
>>     else
>>     {
>> -        ext_regions = xzalloc_flex_struct(struct membanks, bank, 
>> NR_MEM_BANKS);
>> +        ext_regions = membanks_xzalloc(NR_MEM_BANKS, RESERVED_MEMORY);
> I'm a bit confused what is the expectations behind using different types of 
> enum region_type, mostly because it can point to
> different address spaces depending on the context. Above, you marked gnttab 
> as RESERVED_MEMORY (I guess because this
> region has already been found - but in fact it is still unused) and 
> hwdom_free_mem as MEMORY. So I would at least expect
> ext_regions to be of MEMORY type as well. After all both hwdom_free_mem and 
> ext_regions contain
> banks of unused/free memory (although former lists host memory while latter 
> can also contain guest physical
> memory). Could you please clarify the intended use?

You are right, that should be MEMORY, my bad! Could it be something addressable 
on commit or should I send another one?

Cheers,
Luca