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

Penny Zheng posted 13 patches 2 months, 2 weeks ago
[PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
Posted by Penny Zheng 2 months, 2 weeks ago
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;
+    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
-- 
2.25.1
Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
Posted by Julien Grall 3 weeks, 2 days ago
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,

-- 
Julien Grall
RE: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
Posted by Penny Zheng 3 weeks, 1 day ago

> -----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
Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
Posted by Julien Grall 3 weeks, 1 day ago
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,

-- 
Julien Grall