| Message ID | 20221115025235.1378931-2-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Follow-up static shared memory | expand |
Hi Penny, On 15/11/2022 02:52, Penny Zheng wrote: > This commit re-arranges the static shared memory regions into a separate array > shm_meminfo. And static shared memory region now would have its own structure > 'shm_membank' to hold all shm-related members, like shm_id, etc, and a pointer > to the normal memory bank 'membank'. This will avoid continuing to grow > 'membank'. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------ > xen/arch/arm/domain_build.c | 35 ++++++++++++++++----------- > xen/arch/arm/include/asm/kernel.h | 2 +- > xen/arch/arm/include/asm/setup.h | 16 +++++++++---- > 4 files changed, 59 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 6014c0f852..ccf281cd37 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, int node, > const __be32 *cell; > paddr_t paddr, gaddr, size; > struct meminfo *mem = &bootinfo.reserved_mem; After this patch, 'mem' is barely going to be used. So I would recommend to remove it or restrict the scope. This will make easier to confirm that most of the use of 'mem' have been replaced with 'shm_mem' and reduce the risk of confusion between the two (the name are quite similar). [...] > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index bd30d3798c..c0fd13f6ed 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -757,20 +757,20 @@ static int __init acquire_nr_borrower_domain(struct domain *d, > { > unsigned int bank; > > - /* Iterate reserved memory to find requested shm bank. */ > - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ ) > + /* Iterate static shared memory to find requested shm bank. */ > + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ ) > { > - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start; > - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size; > + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start; > + paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size; I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be removed. But it looks like there was none. I guess that was a mistake in the existing code? > > if ( (pbase == bank_start) && (psize == bank_size) ) > break; > } > > - if ( bank == bootinfo.reserved_mem.nr_banks ) > + if ( bank == bootinfo.shm_mem.nr_banks ) > return -ENOENT; > > - *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers; > + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers; > > return 0; > } > @@ -907,11 +907,18 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo, > paddr_t start, paddr_t size, > const char *shm_id) > { > + struct membank *membank; > + > if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS ) > return -ENOMEM; > > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start; > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size; > + membank = xmalloc_bytes(sizeof(struct membank)); You allocate memory but never free it. However, I think it would be better to avoid the dynamic allocation. So I would consider to not use the structure shm_membank and instead create a specific one for the domain construction. > + if ( membank == NULL ) > + return -ENOMEM; > + > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank; > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start; > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size; The last two could be replaced with: membank->start = start; membank->size = size; This would make the code more readable. Also, while you are modifying the code, I would consider to introduce a local variable that points to kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks]. [...] > struct meminfo { > @@ -61,6 +57,17 @@ struct meminfo { > struct membank bank[NR_MEM_BANKS]; > }; > > +struct shm_membank { > + char shm_id[MAX_SHM_ID_LENGTH]; > + unsigned int nr_shm_borrowers; > + struct membank *membank; After the change I suggest above, I would expect that the field of membank will not be updated. So I would add const here. Cheers,
> -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Sunday, January 8, 2023 7:44 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory > region > > Hi Penny, > Hi Julien > On 15/11/2022 02:52, Penny Zheng wrote: > > This commit re-arranges the static shared memory regions into a > > separate array shm_meminfo. And static shared memory region now > would > > have its own structure 'shm_membank' to hold all shm-related members, > > like shm_id, etc, and a pointer to the normal memory bank 'membank'. > > This will avoid continuing to grow 'membank'. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------ > > xen/arch/arm/domain_build.c | 35 ++++++++++++++++----------- > > xen/arch/arm/include/asm/kernel.h | 2 +- > > xen/arch/arm/include/asm/setup.h | 16 +++++++++---- > > 4 files changed, 59 insertions(+), 34 deletions(-) > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index > > 6014c0f852..ccf281cd37 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, > int node, > > const __be32 *cell; > > paddr_t paddr, gaddr, size; > > struct meminfo *mem = &bootinfo.reserved_mem; > > After this patch, 'mem' is barely going to be used. So I would recommend to > remove it or restrict the scope. > Hope I understand correctly, you are saying that all static shared memory bank will be described as "struct shm_membank". That's right. However when host address is provided, we still need an instance of "struct membank" to refer to in "bootinfo.reserved_mem". Only in this way, it could be initialized properly in as static pages. That's why I put a "struct membank*" pointer in "struct shm_membank" to refer to the same object. If I removed instance in bootinfo.reserved_mem, a few more path needs to be updated, like Init_staticmem_pages, dt_unreserved_regions, etc > This will make easier to confirm that most of the use of 'mem' have been > replaced with 'shm_mem' and reduce the risk of confusion between the two > (the name are quite similar). > > [...] > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index bd30d3798c..c0fd13f6ed 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -757,20 +757,20 @@ static int __init > acquire_nr_borrower_domain(struct domain *d, > > { > > unsigned int bank; > > > > - /* Iterate reserved memory to find requested shm bank. */ > > - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ ) > > + /* Iterate static shared memory to find requested shm bank. */ > > + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ ) > > { > > - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start; > > - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size; > > + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank- > >start; > > + paddr_t bank_size = > > + bootinfo.shm_mem.bank[bank].membank->size; > > I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be > removed. But it looks like there was none. I guess that was a mistake in the > existing code? > Oh, you're right, the type shall also be checked. > > > > if ( (pbase == bank_start) && (psize == bank_size) ) > > break; > > } > > > > - if ( bank == bootinfo.reserved_mem.nr_banks ) > > + if ( bank == bootinfo.shm_mem.nr_banks ) > > return -ENOENT; > > > > - *nr_borrowers = > bootinfo.reserved_mem.bank[bank].nr_shm_borrowers; > > + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers; > > > > return 0; > > } > > @@ -907,11 +907,18 @@ static int __init > append_shm_bank_to_domain(struct kernel_info *kinfo, > > paddr_t start, paddr_t size, > > const char *shm_id) > > { > > + struct membank *membank; > > + > > if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS ) > > return -ENOMEM; > > > > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start; > > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size; > > + membank = xmalloc_bytes(sizeof(struct membank)); > > You allocate memory but never free it. However, I think it would be better to > avoid the dynamic allocation. So I would consider to not use the structure > shm_membank and instead create a specific one for the domain construction. > True, a local variable "struct meminfo shm_banks" could be introduced only for domain construction in function construct_domU > > + if ( membank == NULL ) > > + return -ENOMEM; > > + > > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = > membank; > > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = > start; > > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = > > + size; > > The last two could be replaced with: > > membank->start = start; > membank->size = size; > > This would make the code more readable. Also, while you are modifying the > code, I would consider to introduce a local variable that points to > kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks]. > > [...] > > > struct meminfo { > > @@ -61,6 +57,17 @@ struct meminfo { > > struct membank bank[NR_MEM_BANKS]; > > }; > > > > +struct shm_membank { > > + char shm_id[MAX_SHM_ID_LENGTH]; > > + unsigned int nr_shm_borrowers; > > + struct membank *membank; > > After the change I suggest above, I would expect that the field of > membank will not be updated. So I would add const here. > > Cheers, > > -- > Julien Grall
Hi Penny, On 09/01/2023 07:48, Penny Zheng wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Sunday, January 8, 2023 7:44 PM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory >> region >> On 15/11/2022 02:52, Penny Zheng wrote: >>> This commit re-arranges the static shared memory regions into a >>> separate array shm_meminfo. And static shared memory region now >> would >>> have its own structure 'shm_membank' to hold all shm-related members, >>> like shm_id, etc, and a pointer to the normal memory bank 'membank'. >>> This will avoid continuing to grow 'membank'. >>> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> --- >>> xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------ >>> xen/arch/arm/domain_build.c | 35 ++++++++++++++++----------- >>> xen/arch/arm/include/asm/kernel.h | 2 +- >>> xen/arch/arm/include/asm/setup.h | 16 +++++++++---- >>> 4 files changed, 59 insertions(+), 34 deletions(-) >>> >>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index >>> 6014c0f852..ccf281cd37 100644 >>> --- a/xen/arch/arm/bootfdt.c >>> +++ b/xen/arch/arm/bootfdt.c >>> @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, >> int node, >>> const __be32 *cell; >>> paddr_t paddr, gaddr, size; >>> struct meminfo *mem = &bootinfo.reserved_mem; >> >> After this patch, 'mem' is barely going to be used. So I would recommend to >> remove it or restrict the scope. >> > > Hope I understand correctly, you are saying that all static shared memory bank will be > described as "struct shm_membank". That's right. > However when host address is provided, we still need an instance of "struct membank" > to refer to in "bootinfo.reserved_mem". Only in this way, it could be initialized properly in > as static pages. > That's why I put a "struct membank*" pointer in "struct shm_membank" to refer to the > same object. I wasn't talking about the field in "struct shm_membank". Instead, I was referring to the local variable: struct meminfo *mem = &bootinfo.reserved_mem; AFAICT, the only use after this patch is when you add a new bank in shm_mem. So you could restrict the scope of the local variable. > If I removed instance in bootinfo.reserved_mem, a few more path needs to be updated, like > Init_staticmem_pages, dt_unreserved_regions, etc > >> This will make easier to confirm that most of the use of 'mem' have been >> replaced with 'shm_mem' and reduce the risk of confusion between the two >> (the name are quite similar). >> >> [...] >> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index bd30d3798c..c0fd13f6ed 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -757,20 +757,20 @@ static int __init >> acquire_nr_borrower_domain(struct domain *d, >>> { >>> unsigned int bank; >>> >>> - /* Iterate reserved memory to find requested shm bank. */ >>> - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ ) >>> + /* Iterate static shared memory to find requested shm bank. */ >>> + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ ) >>> { >>> - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start; >>> - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size; >>> + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank- >>> start; >>> + paddr_t bank_size = >>> + bootinfo.shm_mem.bank[bank].membank->size; >> >> I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be >> removed. But it looks like there was none. I guess that was a mistake in the >> existing code? >> > > Oh, you're right, the type shall also be checked. Just to clarify, with this patch you don't need to check the type. I was pointing out a latent error in the existing code. > >>> >>> if ( (pbase == bank_start) && (psize == bank_size) ) >>> break; >>> } >>> >>> - if ( bank == bootinfo.reserved_mem.nr_banks ) >>> + if ( bank == bootinfo.shm_mem.nr_banks ) >>> return -ENOENT; >>> >>> - *nr_borrowers = >> bootinfo.reserved_mem.bank[bank].nr_shm_borrowers; >>> + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers; >>> >>> return 0; >>> } >>> @@ -907,11 +907,18 @@ static int __init >> append_shm_bank_to_domain(struct kernel_info *kinfo, >>> paddr_t start, paddr_t size, >>> const char *shm_id) >>> { >>> + struct membank *membank; >>> + >>> if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS ) >>> return -ENOMEM; >>> >>> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start; >>> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size; >>> + membank = xmalloc_bytes(sizeof(struct membank)); >> >> You allocate memory but never free it. However, I think it would be better to >> avoid the dynamic allocation. So I would consider to not use the structure >> shm_membank and instead create a specific one for the domain construction. >> > > True, a local variable "struct meminfo shm_banks" could be introduced only > for domain construction in function construct_domU Hmmm... I didn't suggest to introduce a local variable. I would still much prefer if we keep using 'kinfo' because we keep all the domain building information in one place. So ``struct meninfo`` should want to be defined in ``kinfo``. Cheers,
Hi Penny, On 11/14/22 21:52, Penny Zheng wrote: > ... > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index fdbf68aadc..2d4ae0f00a 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -50,10 +50,6 @@ struct membank { > paddr_t start; > paddr_t size; > enum membank_type type; > -#ifdef CONFIG_STATIC_SHM > - char shm_id[MAX_SHM_ID_LENGTH]; > - unsigned int nr_shm_borrowers; > -#endif > }; > > struct meminfo { > @@ -61,6 +57,17 @@ struct meminfo { > struct membank bank[NR_MEM_BANKS]; > }; > > +struct shm_membank { > + char shm_id[MAX_SHM_ID_LENGTH]; > + unsigned int nr_shm_borrowers; > + struct membank *membank; > +}; > + > +struct shm_meminfo { > + unsigned int nr_banks; > + struct shm_membank bank[NR_MEM_BANKS]; > +}; > + > /* > * The domU flag is set for kernels and ramdisks of "xen,domain" nodes. > * The purpose of the domU flag is to avoid getting confused in > @@ -105,6 +112,7 @@ struct bootinfo { > struct meminfo acpi; > #endif > bool static_heap; > + struct shm_meminfo shm_mem; > }; > > struct map_range_data The reduction in the sizeof struct membank is a welcome improvement, in my opinion, because it is multiplied by NR_MEM_BANKS, and IIRC we only have 32k of boot stack to play with. Before this patch: sizeof(struct kernel_info): 20648 sizeof(struct meminfo): 10248 sizeof(struct shm_meminfo): 10248 When building with EXTRA_CFLAGS_XEN_CORE="Wstack-usage=4096 -Wno-error=stack-usage=": arch/arm/domain_build.c: In function ‘construct_domU’: arch/arm/domain_build.c:3747:19: warning: stack usage is 20720 bytes [-Wstack-usage=] 3747 | static int __init construct_domU(struct domain *d, | ^~~~~~~~~~~~~~ arch/arm/domain_build.c: In function ‘construct_dom0’: arch/arm/domain_build.c:3979:19: warning: stack usage is 20688 bytes [-Wstack-usage=] 3979 | static int __init construct_dom0(struct domain *d) | ^~~~~~~~~~~~~~ After this patch: sizeof(struct kernel_info): 14504 sizeof(struct meminfo): 6152 sizeof(struct shm_meminfo): 8200 arch/arm/domain_build.c: In function ‘construct_domU’: arch/arm/domain_build.c:3754:19: warning: stack usage is 14576 bytes [-Wstack-usage=] 3754 | static int __init construct_domU(struct domain *d, | ^~~~~~~~~~~~~~ arch/arm/domain_build.c: In function ‘construct_dom0’: arch/arm/domain_build.c:3986:19: warning: stack usage is 14544 bytes [-Wstack-usage=] 3986 | static int __init construct_dom0(struct domain *d) | ^~~~~~~~~~~~~~ A later patch in this series will increase it again slightly. Note that I'm not expecting this series to address these particular warnings, I'm merely pointing out the (positive) impact of the change.
> -----Original Message----- > From: Stewart Hildebrand <stewart.hildebrand@amd.com> > Sent: Wednesday, February 8, 2023 4:55 AM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory > region > > Hi Penny, > Hi, Stewart Sorry for the late response, got sidetracked by a few internal projects > On 11/14/22 21:52, Penny Zheng wrote: > > ... > > diff --git a/xen/arch/arm/include/asm/setup.h > > b/xen/arch/arm/include/asm/setup.h > > index fdbf68aadc..2d4ae0f00a 100644 > > --- a/xen/arch/arm/include/asm/setup.h > > +++ b/xen/arch/arm/include/asm/setup.h > > @@ -50,10 +50,6 @@ struct membank { > > paddr_t start; > > paddr_t size; > > enum membank_type type; > > -#ifdef CONFIG_STATIC_SHM > > - char shm_id[MAX_SHM_ID_LENGTH]; > > - unsigned int nr_shm_borrowers; > > -#endif > > }; > > > > struct meminfo { > > @@ -61,6 +57,17 @@ struct meminfo { > > struct membank bank[NR_MEM_BANKS]; }; > > > > +struct shm_membank { > > + char shm_id[MAX_SHM_ID_LENGTH]; > > + unsigned int nr_shm_borrowers; > > + struct membank *membank; > > +}; > > + > > +struct shm_meminfo { > > + unsigned int nr_banks; > > + struct shm_membank bank[NR_MEM_BANKS]; }; > > + > > /* > > * The domU flag is set for kernels and ramdisks of "xen,domain" nodes. > > * The purpose of the domU flag is to avoid getting confused in @@ > > -105,6 +112,7 @@ struct bootinfo { > > struct meminfo acpi; > > #endif > > bool static_heap; > > + struct shm_meminfo shm_mem; > > }; > > > > struct map_range_data > > The reduction in the sizeof struct membank is a welcome improvement, in > my opinion, because it is multiplied by NR_MEM_BANKS, and IIRC we only > have 32k of boot stack to play with. > > Before this patch: > sizeof(struct kernel_info): 20648 > sizeof(struct meminfo): 10248 > sizeof(struct shm_meminfo): 10248 > > When building with EXTRA_CFLAGS_XEN_CORE="Wstack-usage=4096 -Wno- > error=stack-usage=": Learnt! Thx! > arch/arm/domain_build.c: In function ‘construct_domU’: > arch/arm/domain_build.c:3747:19: warning: stack usage is 20720 bytes [- > Wstack-usage=] > 3747 | static int __init construct_domU(struct domain *d, > | ^~~~~~~~~~~~~~ > arch/arm/domain_build.c: In function ‘construct_dom0’: > arch/arm/domain_build.c:3979:19: warning: stack usage is 20688 bytes [- > Wstack-usage=] > 3979 | static int __init construct_dom0(struct domain *d) > | ^~~~~~~~~~~~~~ > > > > After this patch: > sizeof(struct kernel_info): 14504 > sizeof(struct meminfo): 6152 > sizeof(struct shm_meminfo): 8200 > > arch/arm/domain_build.c: In function ‘construct_domU’: > arch/arm/domain_build.c:3754:19: warning: stack usage is 14576 bytes [- > Wstack-usage=] > 3754 | static int __init construct_domU(struct domain *d, > | ^~~~~~~~~~~~~~ > arch/arm/domain_build.c: In function ‘construct_dom0’: > arch/arm/domain_build.c:3986:19: warning: stack usage is 14544 bytes [- > Wstack-usage=] > 3986 | static int __init construct_dom0(struct domain *d) > | ^~~~~~~~~~~~~~ > > A later patch in this series will increase it again slightly. Note that I'm not > expecting this series to address these particular warnings, I'm merely > pointing out the (positive) impact of the change. I agreed that NR_MEM_BANKS could be a large multiplier, and if we make "struct shm_meminfo" like "struct meminfo", to have a array of NR_MEM_BANKS items, it will make Xen binary exceed 20MB... We have discussed it here[1]. However, I'm afraid that dynamic allocation is also not a preferred option here, where to free the data could be a problem. So in next serie, which will come very soon, I'll introduce: ``` #define NR_SHM_BANKS 16 /* * A static shared memory node could be banked with a statically * configured host memory bank, or a set of arbitrary host memory * banks allocated from heap. */ struct shm_meminfo { unsigned int nr_banks; struct membank bank[NR_SHM_BANKS]; paddr_t tot_size; }; ``` Taking your previous instructions, compiling with "EXTRA_CFLAGS_XEN_CORE="-Wstack-usage=4096 -Wno-error=stack-usage=", boot stack usage for "construct_domU" and " construct_dom0" will be like: " arch/arm/domain_build.c: In function ‘construct_domU’: arch/arm/domain_build.c:4127:19: warning: stack usage is 16800 bytes [-Wstack-usage=] 4127 | static int __init construct_domU(struct domain *d, | ^~~~~~~~~~~~~~ arch/arm/domain_build.c: In function ‘construct_dom0’: arch/arm/domain_build.c:4359:19: warning: stack usage is 16640 bytes [-Wstack-usage=] 4359 | static int __init construct_dom0(struct domain *d) | ^~~~~~~~~~~~~~ " [1] https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg00449.html Cheers, --- Penny Zheng
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 6014c0f852..ccf281cd37 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, int node, const __be32 *cell; paddr_t paddr, gaddr, size; struct meminfo *mem = &bootinfo.reserved_mem; + struct shm_meminfo *shm_mem = &bootinfo.shm_mem; unsigned int i; int len; bool owner = false; @@ -455,17 +456,21 @@ static int __init process_shm_node(const void *fdt, int node, return -EINVAL; } - for ( i = 0; i < mem->nr_banks; i++ ) + for ( i = 0; i < shm_mem->nr_banks; i++ ) { + paddr_t bank_start = shm_mem->bank[i].membank->start; + paddr_t bank_size = shm_mem->bank[i].membank->size; + /* * Meet the following check: * 1) The shm ID matches and the region exactly match * 2) The shm ID doesn't match and the region doesn't overlap * with an existing one */ - if ( paddr == mem->bank[i].start && size == mem->bank[i].size ) + if ( paddr == bank_start && size == bank_size ) { - if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 ) + if ( strncmp(shm_id, + shm_mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 ) break; else { @@ -477,17 +482,17 @@ static int __init process_shm_node(const void *fdt, int node, else { paddr_t end = paddr + size; - paddr_t bank_end = mem->bank[i].start + mem->bank[i].size; + paddr_t bank_end = bank_start + bank_size; - if ( (end <= paddr) || (bank_end <= mem->bank[i].start) ) + if ( (end <= paddr) || (bank_end <= bank_start) ) { printk("fdt: static shared memory region %s overflow\n", shm_id); return -EINVAL; } - if ( (end <= mem->bank[i].start) || (paddr >= bank_end) ) + if ( (end <= bank_start) || (paddr >= bank_end) ) { - if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) + if ( strcmp(shm_id, shm_mem->bank[i].shm_id) != 0 ) continue; else { @@ -499,22 +504,27 @@ static int __init process_shm_node(const void *fdt, int node, else { printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n", - mem->bank[i].start, bank_end); + bank_start, bank_end); return -EINVAL; } } } - if ( i == mem->nr_banks ) + if ( i == shm_mem->nr_banks ) { - if ( i < NR_MEM_BANKS ) + if ( (i < NR_MEM_BANKS) && (mem->nr_banks < NR_MEM_BANKS) ) { /* Static shared memory shall be reserved from any other use. */ - safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id); - mem->bank[mem->nr_banks].start = paddr; - mem->bank[mem->nr_banks].size = size; - mem->bank[mem->nr_banks].type = MEMBANK_STATIC_DOMAIN; + struct membank *membank = &mem->bank[mem->nr_banks]; + + membank->start = paddr; + membank->size = size; + membank->type = MEMBANK_STATIC_DOMAIN; mem->nr_banks++; + + safe_strcpy(shm_mem->bank[i].shm_id, shm_id); + shm_mem->bank[i].membank = membank; + shm_mem->nr_banks++; } else { @@ -527,7 +537,7 @@ static int __init process_shm_node(const void *fdt, int node, * to calculate the reference count. */ if ( !owner ) - mem->bank[i].nr_shm_borrowers++; + shm_mem->bank[i].nr_shm_borrowers++; return 0; } diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index bd30d3798c..c0fd13f6ed 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -757,20 +757,20 @@ static int __init acquire_nr_borrower_domain(struct domain *d, { unsigned int bank; - /* Iterate reserved memory to find requested shm bank. */ - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ ) + /* Iterate static shared memory to find requested shm bank. */ + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ ) { - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start; - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size; + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start; + paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size; if ( (pbase == bank_start) && (psize == bank_size) ) break; } - if ( bank == bootinfo.reserved_mem.nr_banks ) + if ( bank == bootinfo.shm_mem.nr_banks ) return -ENOENT; - *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers; + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers; return 0; } @@ -907,11 +907,18 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start, paddr_t size, const char *shm_id) { + struct membank *membank; + if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS ) return -ENOMEM; - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start; - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size; + membank = xmalloc_bytes(sizeof(struct membank)); + if ( membank == NULL ) + return -ENOMEM; + + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank; + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start; + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size; safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id); kinfo->shm_mem.nr_banks++; @@ -1355,7 +1362,7 @@ static int __init make_memory_node(const struct domain *d, static int __init make_shm_memory_node(const struct domain *d, void *fdt, int addrcells, int sizecells, - const struct meminfo *mem) + const struct shm_meminfo *mem) { unsigned int i = 0; int res = 0; @@ -1372,8 +1379,8 @@ static int __init make_shm_memory_node(const struct domain *d, for ( ; i < mem->nr_banks; i++ ) { - uint64_t start = mem->bank[i].start; - uint64_t size = mem->bank[i].size; + uint64_t start = mem->bank[i].membank->start; + uint64_t size = mem->bank[i].membank->size; /* Placeholder for xen-shmem@ + a 64-bit number + \0 */ char buf[27]; const char compat[] = "xen,shared-memory-v1"; @@ -1382,7 +1389,7 @@ static int __init make_shm_memory_node(const struct domain *d, __be32 *cells; unsigned int len = (addrcells + sizecells) * sizeof(__be32); - snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start); + snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, start); res = fdt_begin_node(fdt, buf); if ( res ) return res; @@ -1426,7 +1433,7 @@ static int __init make_shm_memory_node(const struct domain *d, static int __init make_shm_memory_node(const struct domain *d, void *fdt, int addrcells, int sizecells, - const struct meminfo *mem) + const struct shm_meminfo *mem) { ASSERT_UNREACHABLE(); return -EOPNOTSUPP; @@ -1436,7 +1443,7 @@ static int __init make_shm_memory_node(const struct domain *d, static int __init make_resv_memory_node(const struct domain *d, void *fdt, int addrcells, int sizecells, - const struct meminfo *mem) + const struct shm_meminfo *mem) { int res = 0; /* Placeholder for reserved-memory\0 */ diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index 5bb30c3f2f..f47ba9d619 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -38,7 +38,7 @@ struct kernel_info { void *fdt; /* flat device tree */ paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ struct meminfo mem; - struct meminfo shm_mem; + struct shm_meminfo shm_mem; /* kernel entry point */ paddr_t entry; diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index fdbf68aadc..2d4ae0f00a 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -50,10 +50,6 @@ struct membank { paddr_t start; paddr_t size; enum membank_type type; -#ifdef CONFIG_STATIC_SHM - char shm_id[MAX_SHM_ID_LENGTH]; - unsigned int nr_shm_borrowers; -#endif }; struct meminfo { @@ -61,6 +57,17 @@ struct meminfo { struct membank bank[NR_MEM_BANKS]; }; +struct shm_membank { + char shm_id[MAX_SHM_ID_LENGTH]; + unsigned int nr_shm_borrowers; + struct membank *membank; +}; + +struct shm_meminfo { + unsigned int nr_banks; + struct shm_membank bank[NR_MEM_BANKS]; +}; + /* * The domU flag is set for kernels and ramdisks of "xen,domain" nodes. * The purpose of the domU flag is to avoid getting confused in @@ -105,6 +112,7 @@ struct bootinfo { struct meminfo acpi; #endif bool static_heap; + struct shm_meminfo shm_mem; }; struct map_range_data
This commit re-arranges the static shared memory regions into a separate array shm_meminfo. And static shared memory region now would have its own structure 'shm_membank' to hold all shm-related members, like shm_id, etc, and a pointer to the normal memory bank 'membank'. This will avoid continuing to grow 'membank'. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------ xen/arch/arm/domain_build.c | 35 ++++++++++++++++----------- xen/arch/arm/include/asm/kernel.h | 2 +- xen/arch/arm/include/asm/setup.h | 16 +++++++++---- 4 files changed, 59 insertions(+), 34 deletions(-)