[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: Luca Fancellu <luca.fancellu@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Michal Orzel <michal.orzel@xxxxxxx>
  • Date: Tue, 19 Mar 2024 14:10:08 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=arm.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • 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=dsmyBd0bP8uAZJ09PLh60OBCZMw+z+H0XvEyAGsSFqQ=; b=O8TC0lcsyvyYIeks+Rb8JVlDWJVrvyAbBWHlICfKqjVRcE2tNStLmbyhaFK0KuFUK/W5rhT0tp8CsB2UMinqdKNNSJW2WNWDhbKaLukBtbykKEAEiMgfezADEgV1nae0UjO3PTAitbqsSKIZmmCFtvRQlE95TPHjrcYdEeV5+ePxrldbpbRuL1Pw5qaAwvkGz1frmYIlBAvcBsRiEljB7OcmRxV80Y3fXNQAzWJCSK+2s8hULqco1aAMuJU0lsQWUm+zzd85XtYUXQv/kVPEW2Mujg7xX+L3SzidNrAXKT2cR0jvYttOl480I2aa9Cdc7jeak4vsw/0njhlsvOLgfw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=i0u3VfrBJUsLY7MVX3BuSPkl1ubtqGskhoWfCnbE2O3pu2hVnhUqyWxF7Bi7gAZn/QRuB0nbI6vBVO4AGZ08zl7TDOqj/PnC+1O6gYAK7sCAkKvnE4c/68LfOVErSJrnZ211sTxxUk/czyu5xnEhiyDYtgrRgeEI7iBvGeHSddYwKo7tK2LKO+zZhQPTeWBzOttko2ftMu1YbAmIXJZkCN4JDSwb8Fw7PuTeoWkhBkNxkm/X6VghXU4XNy2LaE++jgc7m6uCdhDoZX06np2S38EjeYwjH23UAXo2l8PvCNB33dPOipjsBSBGyfwlFchjgJXWbrpA+OuqYc2dcGoIKA==
  • Cc: 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:10:29 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

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