Wrap the code and logic that is calling assign_shared_memory
and map_regions_p2mt into a new function 'handle_shared_mem_bank',
it will become useful later when the code will allow the user to
don't pass the host physical address.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
 - add blank line, move owner_dom_io computation inside
   handle_shared_mem_bank in order to reduce args count, remove
   not needed BUGON(). (Michal)
---
 xen/arch/arm/static-shmem.c | 87 ++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 0afc86c43f85..8a14d120690c 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
     return 0;
 }
 
+static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
+                                         const char *role_str,
+                                         const struct membank *shm_bank)
+{
+    bool owner_dom_io = true;
+    paddr_t pbase, psize;
+    int ret;
+
+    pbase = shm_bank->start;
+    psize = shm_bank->size;
+
+    /*
+     * "role" property is optional and if it is defined explicitly,
+     * then the owner domain is not the default "dom_io" domain.
+     */
+    if ( role_str != NULL )
+        owner_dom_io = false;
+
+    /*
+     * 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)) ||
+         (!owner_dom_io && strcmp(role_str, "owner") == 0) )
+    {
+        /*
+         * We found the first borrower of the region, the owner was not
+         * specified, so they should be assigned to dom_io.
+         */
+        ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
+        if ( ret )
+            return ret;
+    }
+
+    if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
+    {
+        /* Set up P2M foreign mapping for borrower domain. */
+        ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
+                               _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
+        if ( ret )
+            return ret;
+    }
+
+    return 0;
+}
+
 int __init process_shm(struct domain *d, struct kernel_info *kinfo,
                        const struct dt_device_node *node)
 {
@@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
         paddr_t gbase, pbase, psize;
         int ret = 0;
         unsigned int i;
-        const char *role_str;
+        const char *role_str = NULL;
         const char *shm_id;
-        bool owner_dom_io = true;
 
         if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
             continue;
@@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
                 return -EINVAL;
             }
 
-        /*
-         * "role" property is optional and if it is defined explicitly,
-         * then the owner domain is not the default "dom_io" domain.
-         */
-        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
-            owner_dom_io = false;
+        /* "role" property is optional */
+        dt_property_read_string(shm_node, "role", &role_str);
 
-        /*
-         * 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)) ||
-             (!owner_dom_io && strcmp(role_str, "owner") == 0) )
-        {
-            /*
-             * We found the first borrower of the region, the owner was not
-             * specified, so they should be assigned to dom_io.
-             */
-            ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
-                                       boot_shm_bank);
-            if ( ret )
-                return ret;
-        }
-
-        if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
-        {
-            /* Set up P2M foreign mapping for borrower domain. */
-            ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
-                                   _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
-            if ( ret )
-                return ret;
-        }
+        ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
+        if ( ret )
+            return ret;
 
         /*
          * Record static shared memory region info for later setting
-- 
2.34.1

Hi Luca,

On 15/05/2024 16:26, Luca Fancellu wrote:
> 
> 
> Wrap the code and logic that is calling assign_shared_memory
> and map_regions_p2mt into a new function 'handle_shared_mem_bank',
> it will become useful later when the code will allow the user to
> don't pass the host physical address.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2 changes:
>  - add blank line, move owner_dom_io computation inside
>    handle_shared_mem_bank in order to reduce args count, remove
>    not needed BUGON(). (Michal)
> ---
>  xen/arch/arm/static-shmem.c | 87 ++++++++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 34 deletions(-)
> 
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 0afc86c43f85..8a14d120690c 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
>      return 0;
>  }
> 
> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> +                                         const char *role_str,
> +                                         const struct membank *shm_bank)
> +{
> +    bool owner_dom_io = true;
> +    paddr_t pbase, psize;
> +    int ret;
> +
> +    pbase = shm_bank->start;
> +    psize = shm_bank->size;
> +
> +    /*
> +     * "role" property is optional and if it is defined explicitly,
> +     * then the owner domain is not the default "dom_io" domain.
> +     */
> +    if ( role_str != NULL )
> +        owner_dom_io = false;
> +
> +    /*
> +     * 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)) ||
> +         (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> +    {
> +        /*
> +         * We found the first borrower of the region, the owner was not
> +         * specified, so they should be assigned to dom_io.
> +         */
> +        ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
> +        if ( ret )
> +            return ret;
> +    }
> +
> +    if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
> +    {
> +        /* Set up P2M foreign mapping for borrower domain. */
> +        ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
> +                               _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
> +        if ( ret )
> +            return ret;
> +    }
> +
> +    return 0;
> +}
> +
>  int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>                         const struct dt_device_node *node)
>  {
> @@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>          paddr_t gbase, pbase, psize;
>          int ret = 0;
>          unsigned int i;
> -        const char *role_str;
> +        const char *role_str = NULL;
>          const char *shm_id;
> -        bool owner_dom_io = true;
> 
>          if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
>              continue;
> @@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>                  return -EINVAL;
>              }
> 
> -        /*
> -         * "role" property is optional and if it is defined explicitly,
> -         * then the owner domain is not the default "dom_io" domain.
> -         */
> -        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
> -            owner_dom_io = false;
> +        /* "role" property is optional */
> +        dt_property_read_string(shm_node, "role", &role_str);
This now violates a MISRA rule saying that if a function returns a value, this value needs to be checked.
I think you should check if return value is non zero and if such, assign role_str NULL (thus removing such
assignment from a definition).

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Hi Michal,

>> 
>> -        /*
>> -         * "role" property is optional and if it is defined explicitly,
>> -         * then the owner domain is not the default "dom_io" domain.
>> -         */
>> -        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>> -            owner_dom_io = false;
>> +        /* "role" property is optional */
>> +        dt_property_read_string(shm_node, "role", &role_str);
> This now violates a MISRA rule saying that if a function returns a value, this value needs to be checked.
> I think you should check if return value is non zero and if such, assign role_str NULL (thus removing such
> assignment from a definition).

Sure, I’ll do it.

> 
> Other than that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> ~Michal

Cheers,
Luca