[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 03/11] xen/arm: Introduce a generic way to access memory bank structures
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. > > 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. [...] > @@ -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))); ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |