[PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter

Penny Zheng posted 13 patches 2 months, 2 weeks ago
[PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter
Posted by Penny Zheng 2 months, 2 weeks ago
Instead of using multiple function parameters to deliver various shm-related
info, like host physical address, SHMID, etc, and with the introduction
of new struct "shm_membank", we could switch to use "shm_membank" as
function parameter to replace them all, to make codes more clear and
tidy.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/arch/arm/domain_build.c | 46 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c0fd13f6ed..d2b9e60b5c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -751,40 +751,31 @@ static void __init assign_static_memory_11(struct domain *d,
 }
 
 #ifdef CONFIG_STATIC_SHM
-static int __init acquire_nr_borrower_domain(struct domain *d,
-                                             paddr_t pbase, paddr_t psize,
-                                             unsigned long *nr_borrowers)
+static struct shm_membank * __init acquire_shm_membank(const char *shm_id)
 {
     unsigned int 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.shm_mem.bank[bank].membank->start;
-        paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
-
-        if ( (pbase == bank_start) && (psize == bank_size) )
+        if ( strcmp(shm_id, bootinfo.shm_mem.bank[bank].shm_id) == 0 )
             break;
-    }
 
     if ( bank == bootinfo.shm_mem.nr_banks )
-        return -ENOENT;
-
-    *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
+        return NULL;
 
-    return 0;
+    return &bootinfo.shm_mem.bank[bank];
 }
 
 /*
  * This function checks whether the static shared memory region is
  * already allocated to dom_io.
  */
-static bool __init is_shm_allocated_to_domio(paddr_t pbase)
+static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
 {
     struct page_info *page;
     struct domain *d;
 
-    page = maddr_to_page(pbase);
+    page = maddr_to_page(shm_membank->membank->start);
     d = page_get_owner_and_reference(page);
     if ( d == NULL )
         return false;
@@ -835,14 +826,17 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
 }
 
 static int __init assign_shared_memory(struct domain *d,
-                                       uint32_t addr_cells, uint32_t size_cells,
-                                       paddr_t pbase, paddr_t psize,
+                                       struct shm_membank *shm_membank,
                                        paddr_t gbase)
 {
     mfn_t smfn;
     int ret = 0;
     unsigned long nr_pages, nr_borrowers, i;
     struct page_info *page;
+    paddr_t pbase, psize;
+
+    pbase = shm_membank->membank->start;
+    psize = shm_membank->membank->size;
 
     printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
            d, pbase, pbase + psize);
@@ -871,9 +865,7 @@ static int __init assign_shared_memory(struct domain *d,
      * Get the right amount of references per page, which is the number of
      * borrower domains.
      */
-    ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
-    if ( ret )
-        return ret;
+    nr_borrowers = shm_membank->nr_shm_borrowers;
 
     /*
      * Instead of letting borrower domain get a page ref, we add as many
@@ -941,6 +933,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         const char *role_str;
         const char *shm_id;
         bool owner_dom_io = true;
+        struct shm_membank *shm_membank;
 
         if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
             continue;
@@ -991,12 +984,20 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         }
         BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
 
+        shm_membank = acquire_shm_membank(shm_id);
+        if ( !shm_membank )
+        {
+            printk("%pd: failed to acquire %s shared memory bank\n",
+                   d, shm_id);
+            return -ENOENT;
+        }
+
         /*
          * DOMID_IO is a fake domain and is not described in the Device-Tree.
          * Therefore when the owner of the shared region is DOMID_IO, we will
          * only find the borrowers.
          */
-        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
+        if ( (owner_dom_io && !is_shm_allocated_to_domio(shm_membank)) ||
              (!owner_dom_io && strcmp(role_str, "owner") == 0) )
         {
             /*
@@ -1004,8 +1005,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
              * specified, so they should be assigned to dom_io.
              */
             ret = assign_shared_memory(owner_dom_io ? dom_io : d,
-                                       addr_cells, size_cells,
-                                       pbase, psize, gbase);
+                                       shm_membank, gbase);
             if ( ret )
                 return ret;
         }
-- 
2.25.1
Re: [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter
Posted by Julien Grall 3 weeks, 2 days ago
Hi Penny,

On 15/11/2022 02:52, Penny Zheng wrote:
> Instead of using multiple function parameters to deliver various shm-related
> info, like host physical address, SHMID, etc, and with the introduction
> of new struct "shm_membank", we could switch to use "shm_membank" as
> function parameter to replace them all, to make codes more clear and
> tidy.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 46 ++++++++++++++++++-------------------
>   1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c0fd13f6ed..d2b9e60b5c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -751,40 +751,31 @@ static void __init assign_static_memory_11(struct domain *d,
>   }
>   
>   #ifdef CONFIG_STATIC_SHM
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> -                                             paddr_t pbase, paddr_t psize,
> -                                             unsigned long *nr_borrowers)
> +static struct shm_membank * __init acquire_shm_membank(const char *shm_id)

You are returning bootinfo. AFAICT, nobody you modify it after it was 
populated. So can this be const?

Also, I think the function wants to be renamed because this is too 
similar to the function acquire_shared_memory_bank().

I would rename it to find_shared_memory_bank().

>   {
>       unsigned int 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.shm_mem.bank[bank].membank->start;
> -        paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
> -
> -        if ( (pbase == bank_start) && (psize == bank_size) )
> +        if ( strcmp(shm_id, bootinfo.shm_mem.bank[bank].shm_id) == 0 )
>               break;
> -    }
>   
>       if ( bank == bootinfo.shm_mem.nr_banks )
> -        return -ENOENT;
> -
> -    *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
> +        return NULL;
>   
> -    return 0;
> +    return &bootinfo.shm_mem.bank[bank];
>   }
>   
>   /*
>    * This function checks whether the static shared memory region is
>    * already allocated to dom_io.
>    */
> -static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> +static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)

AFAICT, the function will not modify shm_membank. So please use const.

>   {
>       struct page_info *page;
>       struct domain *d;
>   
> -    page = maddr_to_page(pbase);
> +    page = maddr_to_page(shm_membank->membank->start);
>       d = page_get_owner_and_reference(page);
>       if ( d == NULL )
>           return false;
> @@ -835,14 +826,17 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>   }
>   
>   static int __init assign_shared_memory(struct domain *d,
> -                                       uint32_t addr_cells, uint32_t size_cells,
> -                                       paddr_t pbase, paddr_t psize,
> +                                       struct shm_membank *shm_membank,

Same here.

>                                          paddr_t gbase)
>   {
>       mfn_t smfn;
>       int ret = 0;
>       unsigned long nr_pages, nr_borrowers, i;
>       struct page_info *page;
> +    paddr_t pbase, psize;
> +
> +    pbase = shm_membank->membank->start;
> +    psize = shm_membank->membank->size;
>   
>       printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
>              d, pbase, pbase + psize);
> @@ -871,9 +865,7 @@ static int __init assign_shared_memory(struct domain *d,
>        * Get the right amount of references per page, which is the number of
>        * borrower domains.
>        */
> -    ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> -    if ( ret )
> -        return ret;
> +    nr_borrowers = shm_membank->nr_shm_borrowers;
>   
>       /*
>        * Instead of letting borrower domain get a page ref, we add as many
> @@ -941,6 +933,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>           const char *role_str;
>           const char *shm_id;
>           bool owner_dom_io = true;
> +        struct shm_membank *shm_membank;

same here.

>   
>           if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
>               continue;
> @@ -991,12 +984,20 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>           }
>           BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
>   
> +        shm_membank = acquire_shm_membank(shm_id);
> +        if ( !shm_membank )
> +        {
> +            printk("%pd: failed to acquire %s shared memory bank\n",
> +                   d, shm_id);
> +            return -ENOENT;
> +        }
> +
>           /*
>            * DOMID_IO is a fake domain and is not described in the Device-Tree.
>            * Therefore when the owner of the shared region is DOMID_IO, we will
>            * only find the borrowers.
>            */
> -        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> +        if ( (owner_dom_io && !is_shm_allocated_to_domio(shm_membank)) ||
>                (!owner_dom_io && strcmp(role_str, "owner") == 0) )
>           {
>               /*
> @@ -1004,8 +1005,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>                * specified, so they should be assigned to dom_io.
>                */
>               ret = assign_shared_memory(owner_dom_io ? dom_io : d,
> -                                       addr_cells, size_cells,
> -                                       pbase, psize, gbase);
> +                                       shm_membank, gbase);
>               if ( ret )
>                   return ret;
>           }

Cheers,

-- 
Julien Grall
Re: [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter
Posted by Jeungwoo Yoo 2 months, 2 weeks ago
Hello,

Reading this patch, I found one place that can be improved.

On 11/15/22 03:52, Penny Zheng wrote:
> Instead of using multiple function parameters to deliver various shm-related
> info, like host physical address, SHMID, etc, and with the introduction
> of new struct "shm_membank", we could switch to use "shm_membank" as
> function parameter to replace them all, to make codes more clear and
> tidy.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 46 ++++++++++++++++++-------------------
>   1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c0fd13f6ed..d2b9e60b5c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -751,40 +751,31 @@ static void __init assign_static_memory_11(struct domain *d,
>   }
>
>   #ifdef CONFIG_STATIC_SHM
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> -                                             paddr_t pbase, paddr_t psize,
> -                                             unsigned long *nr_borrowers)
> +static struct shm_membank * __init acquire_shm_membank(const char *shm_id)
>   {
>       unsigned int 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.shm_mem.bank[bank].membank->start;
> -        paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
> -
> -        if ( (pbase == bank_start) && (psize == bank_size) )
> +        if ( strcmp(shm_id, bootinfo.shm_mem.bank[bank].shm_id) == 0 )

The target shared memory is found and the bank can be returned
directly here (return &bootinfo.shm_mem.bank[bank];).
Therefore, the out-of-bounds condition check can be removed below.

>               break;
> -    }
>
>       if ( bank == bootinfo.shm_mem.nr_banks )

This can be removed, but only return NULL because the target memory is
not found.


> -        return -ENOENT;
> -
> -    *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
> +        return NULL;
>
> -    return 0;
> +    return &bootinfo.shm_mem.bank[bank];
>   }
>
>   /*
>    * This function checks whether the static shared memory region is
>    * already allocated to dom_io.
>    */
> -static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> +static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
>   {
>       struct page_info *page;
>       struct domain *d;
>
> -    page = maddr_to_page(pbase);
> +    page = maddr_to_page(shm_membank->membank->start);
>       d = page_get_owner_and_reference(page);
>       if ( d == NULL )
>           return false;
> @@ -835,14 +826,17 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>   }
>
>   static int __init assign_shared_memory(struct domain *d,
> -                                       uint32_t addr_cells, uint32_t size_cells,
> -                                       paddr_t pbase, paddr_t psize,
> +                                       struct shm_membank *shm_membank,
>                                          paddr_t gbase)
>   {
>       mfn_t smfn;
>       int ret = 0;
>       unsigned long nr_pages, nr_borrowers, i;
>       struct page_info *page;
> +    paddr_t pbase, psize;
> +
> +    pbase = shm_membank->membank->start;
> +    psize = shm_membank->membank->size;
>
>       printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
>              d, pbase, pbase + psize);
> @@ -871,9 +865,7 @@ static int __init assign_shared_memory(struct domain *d,
>        * Get the right amount of references per page, which is the number of
>        * borrower domains.
>        */
> -    ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> -    if ( ret )
> -        return ret;
> +    nr_borrowers = shm_membank->nr_shm_borrowers;
>
>       /*
>        * Instead of letting borrower domain get a page ref, we add as many
> @@ -941,6 +933,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>           const char *role_str;
>           const char *shm_id;
>           bool owner_dom_io = true;
> +        struct shm_membank *shm_membank;
>
>           if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
>               continue;
> @@ -991,12 +984,20 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>           }
>           BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
>
> +        shm_membank = acquire_shm_membank(shm_id);
> +        if ( !shm_membank )
> +        {
> +            printk("%pd: failed to acquire %s shared memory bank\n",
> +                   d, shm_id);
> +            return -ENOENT;
> +        }
> +
>           /*
>            * DOMID_IO is a fake domain and is not described in the Device-Tree.
>            * Therefore when the owner of the shared region is DOMID_IO, we will
>            * only find the borrowers.
>            */
> -        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> +        if ( (owner_dom_io && !is_shm_allocated_to_domio(shm_membank)) ||
>                (!owner_dom_io && strcmp(role_str, "owner") == 0) )
>           {
>               /*
> @@ -1004,8 +1005,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>                * specified, so they should be assigned to dom_io.
>                */
>               ret = assign_shared_memory(owner_dom_io ? dom_io : d,
> -                                       addr_cells, size_cells,
> -                                       pbase, psize, gbase);
> +                                       shm_membank, gbase);
>               if ( ret )
>                   return ret;
>           }