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

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 09/01/2023 07:48, Penny Zheng wrote:
> > -----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
> > 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.
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,

--
Julien Grall