| Message ID | 20240312130331.78418-5-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt1 | expand |
Hi Luca, On 12/03/2024 14:03, Luca Fancellu wrote: > > > The user of shm_mem member of the 'struct kernel_info' is only > the code managing the static shared memory feature, which can be > compiled out using CONFIG_STATIC_SHM, so in case the feature is > not requested, that member won't be used and will waste memory > space. > > To address this issue, protect the member with the Kconfig parameter > and modify the signature of the only function using it to remove > any reference to the member from outside the static-shmem module. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> NIT: I always wonder why we have hundreds of functions taking both struct domain and struct kernel_info as arguments if the latter has the former as its member. As you are revisiting the function and modifying parameter list, you could take the opportunity to change it. But you don't have to. ~Michal
> On 19 Mar 2024, at 14:58, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Luca, > > On 12/03/2024 14:03, Luca Fancellu wrote: >> >> >> The user of shm_mem member of the 'struct kernel_info' is only >> the code managing the static shared memory feature, which can be >> compiled out using CONFIG_STATIC_SHM, so in case the feature is >> not requested, that member won't be used and will waste memory >> space. >> >> To address this issue, protect the member with the Kconfig parameter >> and modify the signature of the only function using it to remove >> any reference to the member from outside the static-shmem module. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > > NIT: I always wonder why we have hundreds of functions taking both struct domain and > struct kernel_info as arguments if the latter has the former as its member. As you are > revisiting the function and modifying parameter list, you could take the opportunity > to change it. But you don't have to. You are right, can I do this modification as part of patch 3 and this one? Also, can I keep your R-by here when doing this change? > > ~Michal >
Hi Luca, On 04/04/2024 11:27, Luca Fancellu wrote: > > >> On 19 Mar 2024, at 14:58, Michal Orzel <michal.orzel@amd.com> wrote: >> >> Hi Luca, >> >> On 12/03/2024 14:03, Luca Fancellu wrote: >>> >>> >>> The user of shm_mem member of the 'struct kernel_info' is only >>> the code managing the static shared memory feature, which can be >>> compiled out using CONFIG_STATIC_SHM, so in case the feature is >>> not requested, that member won't be used and will waste memory >>> space. >>> >>> To address this issue, protect the member with the Kconfig parameter >>> and modify the signature of the only function using it to remove >>> any reference to the member from outside the static-shmem module. >>> >>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> Reviewed-by: Michal Orzel <michal.orzel@amd.com> >> >> NIT: I always wonder why we have hundreds of functions taking both struct domain and >> struct kernel_info as arguments if the latter has the former as its member. As you are >> revisiting the function and modifying parameter list, you could take the opportunity >> to change it. But you don't have to. > > You are right, can I do this modification as part of patch 3 and this one? Also, can I keep your R-by > here when doing this change? You can do this as part of patch 3 (afaict there will be no need to modify the argument list in patch 4) and you can keep my Rb. ~Michal
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 0165da6f2986..fe2a771d4984 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -647,8 +647,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; - ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, - &kinfo->shm_mem.common); + ret = make_resv_memory_node(d, kinfo, addrcells, sizecells); if ( ret ) goto err; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 62fcdfbdaff2..b254f252e7cb 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1780,8 +1780,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return res; } - res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, - &kinfo->shm_mem.common); + res = make_resv_memory_node(d, kinfo, addrcells, sizecells); if ( res ) return res; } diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index d28b843c01a9..5785da985ccf 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -39,7 +39,9 @@ struct kernel_info { void *fdt; /* flat device tree */ paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ struct meminfo mem; +#ifdef CONFIG_STATIC_SHM struct meminfo shm_mem; +#endif /* kernel entry point */ paddr_t entry; @@ -81,10 +83,16 @@ struct kernel_info { #define kernel_info_get_mem(kinfo) \ (&(kinfo)->mem.common) +#ifdef CONFIG_STATIC_SHM +#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_MEM_BANKS, +#else +#define KERNEL_INFO_SHM_MEM_INIT +#endif + #define KERNEL_INFO_INIT \ { \ .mem.common.max_banks = NR_MEM_BANKS, \ - .shm_mem.common.max_banks = NR_MEM_BANKS, \ + KERNEL_INFO_SHM_MEM_INIT \ } /* diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h index cc6b414ed79b..108cedb55a9f 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -7,8 +7,9 @@ #ifdef CONFIG_STATIC_SHM -int make_resv_memory_node(const struct domain *d, void *fdt, int addrcells, - int sizecells, const struct membanks *mem); +int make_resv_memory_node(const struct domain *d, + const struct kernel_info *kinfo, int addrcells, + int sizecells); int process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node); @@ -26,9 +27,9 @@ int process_shm_node(const void *fdt, int node, uint32_t address_cells, #else /* !CONFIG_STATIC_SHM */ -static inline int make_resv_memory_node(const struct domain *d, void *fdt, - int addrcells, int sizecells, - const struct membanks *mem) +static inline int make_resv_memory_node(const struct domain *d, + const struct kernel_info *kinfo, + int addrcells, int sizecells) { return 0; } diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index fdc3bccde402..8b7da952be6e 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -490,13 +490,15 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, return 0; } -int __init make_resv_memory_node(const struct domain *d, void *fdt, - int addrcells, int sizecells, - const struct membanks *mem) +int __init make_resv_memory_node(const struct domain *d, + const struct kernel_info *kinfo, + int addrcells, int sizecells) { int res = 0; /* Placeholder for reserved-memory\0 */ const char resvbuf[16] = "reserved-memory"; + const struct membanks *mem = &kinfo->shm_mem.common; + void *fdt = kinfo->fdt; if ( mem->nr_banks == 0 ) /* No shared memory provided. */
The user of shm_mem member of the 'struct kernel_info' is only the code managing the static shared memory feature, which can be compiled out using CONFIG_STATIC_SHM, so in case the feature is not requested, that member won't be used and will waste memory space. To address this issue, protect the member with the Kconfig parameter and modify the signature of the only function using it to remove any reference to the member from outside the static-shmem module. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/dom0less-build.c | 3 +-- xen/arch/arm/domain_build.c | 3 +-- xen/arch/arm/include/asm/kernel.h | 10 +++++++++- xen/arch/arm/include/asm/static-shmem.h | 11 ++++++----- xen/arch/arm/static-shmem.c | 8 +++++--- 5 files changed, 22 insertions(+), 13 deletions(-)