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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 08/04/2022 23:59, Julien Grall wrote:
> 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.

There is another issue here. guest_physmap_add_pages() may use superpage 
mapping. The P2M code is currently assuming the foreing mapping will be 
using L3 mapping (4KB).
Do you need to use superpage mapping here?

Cheers,

--
Julien Grall