Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 23/04/2024 10:25, 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@xxxxxxx>
> ---
>  xen/arch/arm/static-shmem.c | 71 +++++++++++++++++++++++--------------
>  1 file changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index f6cf74e58a83..24e40495a481 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -185,6 +185,47 @@ append_shm_bank_to_domain(struct shared_meminfo 
> *kinfo_shm_mem, paddr_t start,
>      return 0;
>  }
> 
> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> +                                         bool owner_dom_io,
> +                                         const char *role_str,
> +                                         const struct membank *shm_bank)
> +{
> +    paddr_t pbase, psize;
> +    int ret;
> +
> +    BUG_ON(!shm_bank);
not needed

> +
> +    pbase = shm_bank->start;
> +    psize = shm_bank->size;
please add empty line here

> +    /*
> +     * 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)
>  {
> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct 
> kernel_info *kinfo,
>          if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>              owner_dom_io = false;
Looking at owner_dom_io, why don't you move parsing role and setting 
owner_dom_io accordingly to handle_shared_mem_bank()?

~Michal