Re: [PATCH] xen/arm: Initialise memory type for struct kernel_info

[Michal Orzel <michal.orzel@xxxxxxx>]

On 20/12/2024 16:19, 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.
The last part "good to have it" does not sound like we need a Fixes tag.
I'm in two minds here. At the moment we do have some consistency because
we use and therefore initialize .type member only for bootinfo related 
structures.

You suggest to expand this also to kernel_info. But what about other places 
using
struct membanks that, after all, is a useful generic structure? One example you 
can find
is static struct shm_heap_banks in static-shmem.c for which you do not set a 
type. Another
example is allocate_memory() or find_host_extended_regions() where we have to 
convert gnttab
region into struct membanks and there is no need to use the type at all. So, do 
you suggest
we should initialize (explicitly) .type only for *meminfo based structures or 
all the structures
using struct membanks?

> 
> Fixes: a14593e3995a ("xen/device-tree: Allow region overlapping with 
> /memreserve/ ranges")
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>  xen/arch/arm/include/asm/kernel.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/include/asm/kernel.h 
> b/xen/arch/arm/include/asm/kernel.h
> index 7e6e3c82a477..de3f945ae54c 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -92,7 +92,9 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
>  }
> 
>  #ifdef CONFIG_STATIC_SHM
> -#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_SHMEM_BANKS,
> +#define KERNEL_INFO_SHM_MEM_INIT                \
> +    .shm_mem.common.max_banks = NR_SHMEM_BANKS, \
> +    .shm_mem.common.type = STATIC_SHARED_MEMORY,
>  #else
>  #define KERNEL_INFO_SHM_MEM_INIT
>  #endif
> @@ -100,6 +102,7 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
>  #define KERNEL_INFO_INIT                        \
>  {                                               \
>      .mem.common.max_banks = NR_MEM_BANKS,       \
> +    .mem.common.type = MEMORY,                  \
>      KERNEL_INFO_SHM_MEM_INIT                    \
>  }
> 
> --
> 2.34.1
> 

~Michal