If owner property is defined, then owner domain of a static shared memory
region is not the default dom_io anymore, but a specific domain.

This commit implements allocating static shared memory to a specific domain
when owner property is defined.

Coding flow for dealing borrower domain will be introduced later in the
following commits.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v5 change:
- no change
---
v4 change:
- no changes
---
v3 change:
- simplify the code since o_gbase is not used if the domain is dom_io
---
v2 change:
- P2M mapping is restricted to normal domain
- in-code comment fix
---
 xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 91a5ace851..d4fd64e2bd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -805,9 +805,11 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
  */
 static int __init allocate_shared_memory(struct domain *d,
                                          u32 addr_cells, u32 size_cells,
-                                         paddr_t pbase, paddr_t psize)
+                                         paddr_t pbase, paddr_t psize,
+                                         paddr_t gbase)
 {
     mfn_t smfn;
+    int ret = 0;
 
     dprintk(XENLOG_INFO,
             "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
@@ -822,8 +824,18 @@ static int __init allocate_shared_memory(struct domain *d,
      * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
      * It sees RAM 1:1 and we do not need to create P2M mapping for it
      */
-    ASSERT(d == dom_io);
-    return 0;
+    if ( d != dom_io )
+    {
+        ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize));
+        if ( ret )
+        {
+            printk(XENLOG_ERR
+                   "Failed to map shared memory to %pd.\n", d);
+            return ret;
+        }
+    }
+
+    return ret;
 }
 
 static int __init process_shm(struct domain *d,
@@ -836,6 +848,8 @@ static int __init process_shm(struct domain *d,
     u32 shm_id;
     u32 addr_cells, size_cells;
     paddr_t gbase, pbase, psize;
+    const char *role_str;
+    bool owner_dom_io = true;
 
     dt_for_each_child_node(node, shm_node)
     {
@@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d,
         ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
         gbase = dt_read_number(cells, addr_cells);
 
-        /* TODO: Consider owner domain is not the default dom_io. */
+        /*
+         * "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;
+
         /*
          * Per static shared memory region could be shared between multiple
          * domains.
-         * In case re-allocating the same shared memory region, we check
-         * if it is already allocated to the default owner dom_io before
-         * the actual allocation.
+         * So when owner domain is the default dom_io, in case re-allocating
+         * the same shared memory region, we check if it is already allocated
+         * to the default owner dom_io before the actual allocation.
          */
-        if ( !is_shm_allocated_to_domio(pbase) )
+        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
+             (!owner_dom_io && strcmp(role_str, "owner") == 0) )
         {
-            /* Allocate statically shared pages to the default owner dom_io. */
-            ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
-                                         pbase, psize);
+            /* Allocate statically shared pages to the owner domain. */
+            ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
+                                         addr_cells, size_cells,
+                                         pbase, psize, gbase);
             if ( ret )
                 return ret;
         }
-- 
2.25.1

Hi Penny,

On 20/06/2022 06:11, Penny Zheng wrote:
> If owner property is defined, then owner domain of a static shared memory
> region is not the default dom_io anymore, but a specific domain.
> 
> This commit implements allocating static shared memory to a specific domain
> when owner property is defined.
> 
> Coding flow for dealing borrower domain will be introduced later in the
> following commits.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v5 change:
> - no change
> ---
> v4 change:
> - no changes
> ---
> v3 change:
> - simplify the code since o_gbase is not used if the domain is dom_io
> ---
> v2 change:
> - P2M mapping is restricted to normal domain
> - in-code comment fix
> ---
>   xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++----------
>   1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 91a5ace851..d4fd64e2bd 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -805,9 +805,11 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>    */
>   static int __init allocate_shared_memory(struct domain *d,
>                                            u32 addr_cells, u32 size_cells,
> -                                         paddr_t pbase, paddr_t psize)
> +                                         paddr_t pbase, paddr_t psize,
> +                                         paddr_t gbase)
>   {
>       mfn_t smfn;
> +    int ret = 0;
>   
>       dprintk(XENLOG_INFO,
>               "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> @@ -822,8 +824,18 @@ static int __init allocate_shared_memory(struct domain *d,
>        * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
>        * It sees RAM 1:1 and we do not need to create P2M mapping for it
>        */
> -    ASSERT(d == dom_io);
> -    return 0;
> +    if ( d != dom_io )
> +    {
> +        ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize));

Coding style: this line is over 80 characters. And...

> +        if ( ret )
> +        {
> +            printk(XENLOG_ERR
> +                   "Failed to map shared memory to %pd.\n", d);

... this line could be merged with the previous one.

> +            return ret;
> +        }
> +    }
> +
> +    return ret;
>   }
>   
>   static int __init process_shm(struct domain *d,
> @@ -836,6 +848,8 @@ static int __init process_shm(struct domain *d,
>       u32 shm_id;
>       u32 addr_cells, size_cells;
>       paddr_t gbase, pbase, psize;
> +    const char *role_str;
> +    bool owner_dom_io = true;

I think it would be best if role_str and owner_dom_io are defined within 
the loop. Same goes for all the other declarations.

>   
>       dt_for_each_child_node(node, shm_node)
>       {
> @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d,
>           ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
>           gbase = dt_read_number(cells, addr_cells);
>   
> -        /* TODO: Consider owner domain is not the default dom_io. */
> +        /*
> +         * "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
IIUC, the role is per-region. However, owner_dom_io is first initialized 
to false outside the loop. Therefore, the variable may not be correct on 
the next region.

So I think you want to write:

owner_dom_io = !dt_property_read_string(...);

This can also be avoided if you reduce the scope of the variable (it is 
meant to only be used in the loop).

> +
>           /*
>            * Per static shared memory region could be shared between multiple
>            * domains.
> -         * In case re-allocating the same shared memory region, we check
> -         * if it is already allocated to the default owner dom_io before
> -         * the actual allocation.
> +         * So when owner domain is the default dom_io, in case re-allocating
> +         * the same shared memory region, we check if it is already allocated
> +         * to the default owner dom_io before the actual allocation.
>            */
> -        if ( !is_shm_allocated_to_domio(pbase) )
> +        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> +             (!owner_dom_io && strcmp(role_str, "owner") == 0) )
>           {
> -            /* Allocate statically shared pages to the default owner dom_io. */
> -            ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> -                                         pbase, psize);
> +            /* Allocate statically shared pages to the owner domain. */
> +            ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
> +                                         addr_cells, size_cells,
> +                                         pbase, psize, gbase);
>               if ( ret )
>                   return ret;
>           }

Cheers,

-- 
Julien Grall

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 3:07 AM
> 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 v5 3/8] xen/arm: allocate static shared memory to a
> specific owner domain
> 
> Hi Penny,
> 
> On 20/06/2022 06:11, Penny Zheng wrote:
> > If owner property is defined, then owner domain of a static shared
> > memory region is not the default dom_io anymore, but a specific domain.
> >
> > This commit implements allocating static shared memory to a specific
> > domain when owner property is defined.
> >
> > Coding flow for dealing borrower domain will be introduced later in
> > the following commits.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > v5 change:
> > - no change
> > ---
> > v4 change:
> > - no changes
> > ---
> > v3 change:
> > - simplify the code since o_gbase is not used if the domain is dom_io
> > ---
> > v2 change:
> > - P2M mapping is restricted to normal domain
> > - in-code comment fix
> > ---
> >   xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++--------
> --
> >   1 file changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 91a5ace851..d4fd64e2bd 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -805,9 +805,11 @@ static mfn_t __init
> acquire_shared_memory_bank(struct domain *d,
> >    */
> >   static int __init allocate_shared_memory(struct domain *d,
> >                                            u32 addr_cells, u32 size_cells,
> > -                                         paddr_t pbase, paddr_t psize)
> > +                                         paddr_t pbase, paddr_t psize,
> > +                                         paddr_t gbase)
> >   {
> >       mfn_t smfn;
> > +    int ret = 0;
> >
> >       dprintk(XENLOG_INFO,
> >               "Allocate static shared memory BANK
> > %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -822,8 +824,18 @@ static int __init
> allocate_shared_memory(struct domain *d,
> >        * DOMID_IO is the domain, like DOMID_XEN, that is not auto-
> translated.
> >        * It sees RAM 1:1 and we do not need to create P2M mapping for it
> >        */
> > -    ASSERT(d == dom_io);
> > -    return 0;
> > +    if ( d != dom_io )
> > +    {
> > +        ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn,
> > + PFN_DOWN(psize));
> 
> Coding style: this line is over 80 characters. And...
> 
> > +        if ( ret )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "Failed to map shared memory to %pd.\n", d);
> 
> ... this line could be merged with the previous one.
> 
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    return ret;
> >   }
> >
> >   static int __init process_shm(struct domain *d, @@ -836,6 +848,8 @@
> > static int __init process_shm(struct domain *d,
> >       u32 shm_id;
> >       u32 addr_cells, size_cells;
> >       paddr_t gbase, pbase, psize;
> > +    const char *role_str;
> > +    bool owner_dom_io = true;
> 
> I think it would be best if role_str and owner_dom_io are defined within the
> loop. Same goes for all the other declarations.
> 
> >
> >       dt_for_each_child_node(node, shm_node)
> >       {
> > @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d,
> >           ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> PAGE_SIZE));
> >           gbase = dt_read_number(cells, addr_cells);
> >
> > -        /* TODO: Consider owner domain is not the default dom_io. */
> > +        /*
> > +         * "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
> IIUC, the role is per-region. However, owner_dom_io is first initialized to
> false outside the loop. Therefore, the variable may not be correct on the next
> region.
> 
> So I think you want to write:
> 
> owner_dom_io = !dt_property_read_string(...);
> 
> This can also be avoided if you reduce the scope of the variable (it is meant
> to only be used in the loop).
> 

Yes, it is a bug, thx!!! I'll reduce the scope.

> > +
> >           /*
> >            * Per static shared memory region could be shared between multiple
> >            * domains.
> > -         * In case re-allocating the same shared memory region, we check
> > -         * if it is already allocated to the default owner dom_io before
> > -         * the actual allocation.
> > +         * So when owner domain is the default dom_io, in case re-allocating
> > +         * the same shared memory region, we check if it is already allocated
> > +         * to the default owner dom_io before the actual allocation.
> >            */
> > -        if ( !is_shm_allocated_to_domio(pbase) )
> > +        if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> > +             (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> >           {
> > -            /* Allocate statically shared pages to the default owner dom_io. */
> > -            ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> > -                                         pbase, psize);
> > +            /* Allocate statically shared pages to the owner domain. */
> > +            ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
> > +                                         addr_cells, size_cells,
> > +                                         pbase, psize, gbase);
> >               if ( ret )
> >                   return ret;
> >           }
> 
> Cheers,
> 
> --
> Julien Grall