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

[Michal Orzel <michal.orzel@xxxxxxx>]

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.

On 09/01/2025 10:28, Luca Fancellu wrote:
> 
> 
> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
> but forgot to update the 'struct kernel_info' initialiser, while
> it doesn't lead to failures because the field is not currently
> used while managing kernel_info structures, it's good to have it
> for completeness.
> 
> There are other instance of structures using 'struct membanks_hdr'
> that are dynamically allocated and don't fully initialise these
> fields, provide a static inline helper for that.
> 
> Fixes: a14593e3995a ("xen/device-tree: Allow region overlapping with 
> /memreserve/ ranges")
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> Changes from v1:
>  - Changed commit title and body msg
>  - initialised max_banks and type for all structures using 'struct 
> membanks_hdr'
> 
> I didn't get why the fixes tag is wrong, but please feel free to
> correct it on commit or suggest the good one
It is ok.

> ---
> ---
>  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?

~Michal