[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


  • To: Michal Orzel <michal.orzel@xxxxxxx>
  • From: Luca Fancellu <Luca.Fancellu@xxxxxxx>
  • Date: Tue, 19 Mar 2024 13:30:23 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=oE4p0YzeWWuQDURuNHJmYDSPA+fd7dpTX3r/gdvxMKE=; b=nlux7D3PWykcZgV02WJIGKjV9qChHg37S7xjrzPudX5F3vpWNrh4w3gc0srfeDtZcbbgYgE7e8NwOxeRCQTbZIuQynUBily7vqfmBDRhDBEdW/HdQY+Mk/n7IEFKo92LUdBDDl1hzikr+Zky2e4VHfgdhnwTQJiopVP8W1erP9Cw6XBx7zm9bD7ZXkly5TXkUi5qgBKqdAdtHwRDyoyhfIRQMnIwU6GOiyrZ/dEsq0PhWDJHU3Wp8fXwTv4jiDlCP/tekQU59uD1/khTn5qfjsZQjA/K6YctJcKXCFcnfzvNd7nGlRLhERh3ibOfznEmJ36W1WU/Qp5de6QukG7b+g==
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=oE4p0YzeWWuQDURuNHJmYDSPA+fd7dpTX3r/gdvxMKE=; b=Ngi/yRFkszOeEjdFp+/oLdETdSzsIA7DiL/t4t2t8F+T8Z9yMnLQsXBR2xpsMfBSAAwr5rr/F+/BPmFRJX/dLsZL9uJUUim4rcpZQyRxmOYr8upLsq2MkwkKRxOqqqhNoi0rE3uA2tZCRG77QMlfaYDSRqabor7Yd0LvFp3804/ZsaMvMeJ+aFev0sa5n0CbOEsx0ihcAGeO37Zwh7+hg+Thv6SGPc2VZ+yG+WuyGm8kI0+aUdMu5St6Gk9vBFYO5Cvqk01OvhZBDPG9h/rbl2+cEp/1a76gZT+nHFUchiPcz+OtFbbp1ztDTjHL3ZnB7lgklrBdZR4keiynEQXWlQ==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=U94yqitzZC2l8sbz41cgdA85r6zP6xzRPMRllIwXhip7qbEz+0y4q2sSzYsAPoahkFNEC69tesXTZPCEcJdZO42p8XDbbsUEyVTINPsctGNoI0StkRYtQFlQJeRwBYOTE9/VTotax8Udt157GAtC+6kbg88kp6+ro3ZA6u0GSnmeZ+T0xurwUyr1MIO7gQBQfMltkuSCf1O3mkH+df/PpRXvZw8GvikYoAXU7EUdLZ5iHP1DBwGp02HMwEY5OS5I33IroqorMERrptWj7jgDn2um9Gz7ZxRR/khjnMEcEJ4qkyFi/VpXkb+aayX1a1kYf1eizR6T/RWsMz0J+zwRfw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZaEbn72pNn1Jhp+B6rqHo7n/09ZiWR6UIOJ30LrXtJyW0CzV/srp/QVfcnXPy3lZUouovKWqsVGQYTnbDzvVQF6elizOF/+A1q9cHH8bpo+eLEyeK/irkt1dGyFuuzLXJ1GidpsuiuAib7Lq+jsWQGKtyc+eNlkcSGWnB0YfFtrmfu9IwurI5R9JOB2ifZgD3MlaIZVXkp+Jh67fzF6abJEh8Csxv8eF9Afl2zkCZqivC6Wwa0gBvo24n02Sb9soACEPxiBSQwPFAPI1Uh9XsMSLHyRR1IRi7Jnc5KeCM0ROOK+zkSLRkmbmn1NyQGJ6Wgjvja6YWNFPjFBFotDtbw==
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Tue, 19 Mar 2024 13:30:50 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Thread-index: AQHadH3mFY6wZkuH8kG5zjUlnZxCHbE/FIwAgAAFnIA=
  • Thread-topic: [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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.