Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 11/03/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@xxxxxxx>
>
> This commits introduces a new helper guest_physmap_add_shm to set up shared
> memory foreign mapping for borrower domain.
>
> Firstly it should get and take reference of statically shared pages from
> owner dom_shared. Then it will setup P2M foreign memory map of these statically
> shared pages for borrower domain.
>
> This commits only considers owner domain is the default dom_shared, the
> other scenario will be covered in the following patches.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 52 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 984e70e5fc..8cee5ffbd1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct domain *d,
>       return ret;
>   }
>   
> +static int __init guest_physmap_add_shm(struct domain *od, struct domain *bd,
> +                                        unsigned long o_gfn,
> +                                        unsigned long b_gfn,
> +                                        unsigned long nr_gfns)
> +{
> +    struct page_info **pages = NULL;
> +    p2m_type_t p2mt, t;
> +    int ret = 0;
You don't need to initialize ret.

> +
> +    pages = xmalloc_array(struct page_info *, nr_gfns);
> +    if ( !pages )
> +        return -ENOMEM;
> +
> +    /*
> +     * Take reference of statically shared pages from owner domain.
> +     * Reference will be released when destroying shared memory region.
> +     */
> +    ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
> +    if ( ret )
> +    {
> +        ret = -EINVAL;
> +        goto fail_pages;
> +    }
> +
> +    if ( p2m_is_ram(p2mt) )
> +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
> +    else
> +    {
> +        ret = -EINVAL;
> +        goto fail_pages;
Where would we release the references?

> +    }
> +
> +    /* Set up guest foreign map. */
> +    ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
> +                                  nr_gfns, t);
A few things:
  - The beginning of the code assumes that the MFN may be discontiguous 
in the physical memory. But here, you are assuming they are contiguous. 
If you want the latter, then you should check the MFNs are contiguous. 
That said, I am not sure if this restriction is really necessary.
  - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So you 
need to revert it in case of failure.
Cheers,

--
Julien Grall