Re: [PATCH 03/11] xen/arm: Introduce a generic way to access memory bank structures

[Julien Grall <julien@xxxxxxx>]

On 19/03/2024 13:10, Michal Orzel wrote:
> Hi Luca,
>
> On 12/03/2024 14:03, Luca Fancellu wrote:
> > Currently the 'stuct meminfo' is defining a static defined array of
> > 'struct membank' of NR_MEM_BANKS elements, some feature like
> > 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 stucture 'struct membanks', which implements
> s/stucture/structure
>
> > 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.
> Might be beneficial here to mention the use of FAM.
I had to look it up online and there was no hit on Google. I guess you 
mean "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'.
> >
> > Constify pointers where needed.
> >
> > Suggested-by: Julien Grall <julien@xxxxxxx>
> > Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > ---
> >   xen/arch/arm/acpi/domain_build.c        |   6 +-
> >   xen/arch/arm/arm32/mmu/mm.c             |  44 +++++-----
> >   xen/arch/arm/arm64/mmu/mm.c             |   2 +-
> >   xen/arch/arm/bootfdt.c                  |  27 +++---
> >   xen/arch/arm/dom0less-build.c           |  18 ++--
> >   xen/arch/arm/domain_build.c             | 106 +++++++++++++-----------
> >   xen/arch/arm/efi/efi-boot.h             |   8 +-
> >   xen/arch/arm/efi/efi-dom0.c             |  13 +--
> >   xen/arch/arm/include/asm/domain_build.h |   2 +-
> >   xen/arch/arm/include/asm/kernel.h       |   9 ++
> >   xen/arch/arm/include/asm/setup.h        |  40 ++++++++-
> >   xen/arch/arm/include/asm/static-shmem.h |   4 +-
> >   xen/arch/arm/kernel.c                   |  12 +--
> >   xen/arch/arm/setup.c                    |  58 +++++++------
> >   xen/arch/arm/static-memory.c            |  27 +++---
> >   xen/arch/arm/static-shmem.c             |  34 ++++----
> >   16 files changed, 243 insertions(+), 167 deletions(-)
> Lots of mechanical changes but in general I like this approach.
> I'd like other maintainers to share their opinion.
I don't mind the churn so long we get a correct interface at the end. 
The current interface looks alright (I know I suggested it ;)).
> [...]
>
> > @@ -1157,10 +1163,12 @@ int __init make_hypervisor_node(struct domain *d,
> >       }
> >       else
> >       {
> > -        ext_regions = xzalloc(struct meminfo);
> > +        ext_regions = (struct membanks *)xzalloc(struct meminfo);
> You're making assumption that struct membanks is the first member of meminfo 
> but there's no sanity check.
> Why not xzalloc_flex_struct()?
>
> >           if ( !ext_regions )
> >               return -ENOMEM;
> [...]
> > diff --git a/xen/arch/arm/include/asm/kernel.h 
> > b/xen/arch/arm/include/asm/kernel.h
> > index 0a23e86c2d37..d28b843c01a9 100644
> > --- a/xen/arch/arm/include/asm/kernel.h
> > +++ b/xen/arch/arm/include/asm/kernel.h
> > @@ -78,6 +78,15 @@ struct kernel_info {
> >       };
> >   };
> >
> > +#define kernel_info_get_mem(kinfo) \
> > +    (&(kinfo)->mem.common)
> Why this is a macro but for bootinfo you use static inline helpers?
>
> > +
> > +#define KERNEL_INFO_INIT \
> NIT: in most places we prefer \ to be aligned (the same apply to other places)
>
> > +{ \
> > +    .mem.common.max_banks = NR_MEM_BANKS, \
> > +    .shm_mem.common.max_banks = NR_MEM_BANKS, \
> > +}
> > +
> >   /*
> >    * Probe the kernel to detemine its type and select a loader.
> >    *
> > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> > index d15a88d2e0d1..a3e1dc8fdb6c 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -56,8 +56,14 @@ struct membank {
> >   #endif
> >   };
> >
> > -struct meminfo {
> > +struct membanks {
> >       unsigned int nr_banks;
> > +    unsigned int max_banks;
> > +    struct membank bank[];
> > +};
> > +
> > +struct meminfo {
> > +    struct membanks common;
> You were supposed to make sure there is no extra padding here. I don't see any 
> check in this patch.
> I'd at least do sth like:
> BUILD_BUG_ON((offsetof(struct membanks, bank)) != (offsetof(struct meminfo, 
> bank)));
+1. You could also check that "struct membanks" is 8-byte aligned.

Cheers,

--
Julien Grall