Re: [PATCH v1 10/13] xen/arm: allocate static shared memory to a specific owner domain

[Stefano Stabellini <sstabellini@kernel.org>]

On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
> 
> If owner property is defined, then owner domain of a static shared memory
> region is not the default dom_shared anymore, but a specific domain.
> 
> This commit implements allocating static shared memory to a specific domain
> when owner property is defined.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>  xen/arch/arm/domain_build.c | 63 ++++++++++++++++++++++++++++---------
>  1 file changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d35f98ff9c..7ee4d33e0b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -872,6 +872,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_shared = true;
>  
>      dt_for_each_child_node(node, shm_node)
>      {
> @@ -899,6 +901,13 @@ static int __init process_shm(struct domain *d,
>          gbase = dt_read_number(cells, addr_cells);
>  
>          /* TODO: Consider owner domain is not the default dom_shared. */

We should remove this comment?


> +        /*
> +         * "role" property is optional and if it is defined explicitly,
> +         * so the owner domain is not the default "dom_shared" domain.
> +         */
> +        if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
> +            owner_dom_shared = false;
> +
>          /*
>           * Per shared memory region could be shared between multiple domains.
>           * In case re-allocating the same shared memory region, we use bitmask
> @@ -907,17 +916,38 @@ static int __init process_shm(struct domain *d,
>           */
>          if ( !test_bit(shm_id, shm_mask) )
>          {
> -            /*
> -             * Allocate statically shared pages to the default dom_shared.
> -             * Set up P2M, and dom_shared is a direct-map domain,
> -             * so GFN == PFN.
> -             */
> -            ret = allocate_shared_memory(dom_shared, addr_cells, size_cells,
> -                                         pbase, psize, pbase);
> -            if ( ret )
> -                return ret;
> -
> -            set_bit(shm_id, shm_mask);
> +            if ( !owner_dom_shared )
> +            {
> +                if ( strcmp(role_str, "owner") == 0 )
> +                {
> +                    /*
> +                     * Allocate statically shared pages to a specific owner
> +                     * domain.
> +                     */
> +                    ret = allocate_shared_memory(d, shm_id, addr_cells,
> +                                                 size_cells, pbase, psize,
> +                                                 gbase);
> +                    if ( ret )
> +                        return ret;
> +
> +                    set_bit(shm_id, shm_mask);
> +                }
> +            }
> +            else
> +            {
> +                /*
> +                 * Allocate statically shared pages to the default dom_shared.
> +                 * Set up P2M, and dom_shared is a direct-map domain,
> +                 * so GFN == PFN.
> +                 */
> +                ret = allocate_shared_memory(dom_shared, shm_id,
> +                                             addr_cells, size_cells, pbase,
> +                                             psize, pbase);
> +                if ( ret )
> +                    return ret;
> +
> +                set_bit(shm_id, shm_mask);
> +            }

These two chunks are identical if not for dom_shared / d. Can we just
do:

if ( owner_dom_shared )
  d = dom_shared;

on top? Then we can implement this only once.

>          }
>  
>          /*
> @@ -925,10 +955,13 @@ static int __init process_shm(struct domain *d,
>           * default dom_shared, so here we could just set up P2M foreign
>           * mapping for borrower domain immediately.
>           */
> -        ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
> -                                    PFN_DOWN(gbase), PFN_DOWN(psize));
> -        if ( ret )
> -            return ret;
> +        if ( owner_dom_shared )
> +        {
> +            ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
> +                                        PFN_DOWN(gbase), PFN_DOWN(psize));
> +            if ( ret )
> +                return ret;
> +        }

What happens if the borrower is specified before the owner in device
tree? I see the case is handle by the next patch. Maybe we can have at
least a comment here or in the commit message.


>  
>          /*
>           * Record static shared memory region info for later setting
> -- 
> 2.25.1
>