> On 19 Mar 2024, at 13:10, Michal Orzel <[email protected]> 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 <[email protected]>
>> Signed-off-by: Luca Fancellu <[email protected]>
>> ---
>> 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
