Re: [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 09/04/2024 13:45, Luca Fancellu wrote:
> 
> 
> Currently the 'stuct meminfo' is defining a static defined array of
s/stuct/struct

> 'struct membank' of NR_MEM_BANKS elements, some feature like
s/feature/features

> shared memory don't require such amount of memory allocation but
> might want to reuse existing code to manipulate this kind of
> structure that is just as 'struct meminfo' but less bulky.
> 
> For this reason introduce a generic way to access this kind of
> structure using a new structure 'struct membanks', which implements
> all the fields needed by a structure related to memory banks
> without the need to specify at build time the size of the
> 'struct membank' array, using a flexible array member.
> 
> Modify 'struct meminfo' to implement the field related to the new
> introduced structure, given the change all usage of this structure
> are updated in this way:
>  - code accessing bootinfo.{mem,reserved_mem,acpi} field now uses
>    3 new introduced static inline helpers to access the new field
>    of 'struct meminfo' named 'common'.
>  - code accessing 'struct kernel_info *' member 'mem' now use the
>    new introduced macro 'kernel_info_get_mem(...)' to access the
>    new field of 'struct meminfo' named 'common'.

I would also mention introduction of KERNEL_INFO_INIT, BOOTINFO_INIT that 
should be used
from now onwards to initialize corresponding structs (.max_banks member).

> 
> Constify pointers where needed.
> 
> Suggested-by: Julien Grall <julien@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v2:
>  - Fixed typos in commit message and mention flexible array member
>  - Add static assert for struct membanks
>  - use static inline for the kernel_info structure instead of macro
>  - use xzalloc_flex_struct inside make_hypervisor_node instead of
>    xzalloc
>  - Fix trailing backslash style


[...]

> 
> +static inline struct membanks *kernel_info_get_mem(struct kernel_info *kinfo)
> +{
> +    return &kinfo->mem.common;
> +}
> +
> +static inline const struct membanks *
> +kernel_info_get_mem_const(const struct kernel_info *kinfo)
I'm not a fan of having 2 helpers named this way. Maybe macro (as in v1) would 
be better here.

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal