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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 09/01/2025 11:09, Luca Fancellu wrote:
> 
> 
> 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?
I can do that on commit but first, can you please answer what is the intended 
use of enum region_type?
At first I was expecting gnttab to be MEMORY as well. I don't see a difference 
between a region prepared by Xen
for domain to store gnttab vs extended regions. Both specify regions of unused 
address space. It's just that the region
for gnttab must always be present but extended regions don't have to.

~Michal