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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, April 9, 2022 5:31 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand
> Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign
> mapping for borrower domain
> 
> 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?
> 

Right now, there is no user case on my side needing superpage mapping.

> Cheers,
> 
> --
> Julien Grall