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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Sunday, January 8, 2023 7:44 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> 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@xxxxxxx>
> > ---
> >   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