RE: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region

From: Penny Zheng <Penny.Zheng@arm.com>
To: Stewart Hildebrand <stewart.hildebrand@amd.com>,
	"xen-devel@lists.xenproject.org" <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
Date: Tue, 14 Feb 2023 09:56:17 +0000	[thread overview]
Message-ID: <AM0PR08MB453065914FD7E70FEF28782CF7A29@AM0PR08MB4530.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <b14af71d-bc74-a18e-18ae-42e7a0a2efd9@amd.com>

> -----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