[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
> On 19 Mar 2024, at 13:10, Michal Orzel <michal.orzel@xxxxxxx> wrote: > > Hi Luca, Hi Michal, Thanks for having a look > > 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()? I’ll use that, as well as the check you suggested below > >> 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? So I think I started using static inline helpers but I had to have one that didn’t remove the const qualifier, and it was used only once. Anyway if it is acceptable I will go for static inline also here. > >> + >> +#define KERNEL_INFO_INIT \ > NIT: in most places we prefer \ to be aligned (the same apply to other places) kk > >> +{ \ >> + .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))); I’ll add this check, thanks! > > ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |