Re: [PATCH 14/18 V2]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 21 Mar 2013 17:41:46 +0000
Tim Deegan <tim@xen.org> wrote:

> At 17:58 -0700 on 15 Mar (1363370311), Mukesh Rathor wrote:
> >  In this patch, a new function, xenmem_add_foreign_to_pmap(), is
> > added to map pages from foreign guest into current dom0 for domU
> > creation. Also, allow XENMEM_remove_from_physmap to remove
> > p2m_map_foreign pages. Note, in this path, we must release the
> > refcount that was taken during the map phase.
> > 
> > Changes in V2:
> >   - Move the XENMEM_remove_from_physmap changes here instead of
> > prev patch
> >   - Move grant changes from this to one of the next patches.
> >   - In xenmem_add_foreign_to_pmap(), do locked get_gfn
> >   - Fail the mappings for qemu mapping pages for memory not there.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  xen/arch/x86/mm.c   |   74
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > xen/common/memory.c |   44 +++++++++++++++++++++++++++--- 2 files
> > changed, 110 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 6603752..dbac811 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4266,6 +4266,66 @@ static int handle_iomem_range(unsigned long
> > s, unsigned long e, void *p) return 0;
> >  }
> >  
> > +/* 
> > + * Add frames from foreign domain to current domain's physmap.
> > Similar to 
> > + * XENMAPSPACE_gmfn but the frame is foreign being mapped into
> > current,
> > + * and is not removed from foreign domain. 
> > + * Usage: libxl on pvh dom0 creating a guest and doing
> > privcmd_ioctl_mmap.
> > + * Side Effect: the mfn for fgfn will be refcounted so it is not
> > lost
> > + *              while mapped here. The refcnt is released in
> > do_memory_op() 
> > + *              via XENMEM_remove_from_physmap.
> > + * Returns: 0 ==> success
> > + */
> > +static int xenmem_add_foreign_to_pmap(domid_t foreign_domid, 
> > +                                      unsigned long fgfn, unsigned
> > long gpfn) +{
> > +    p2m_type_t p2mt, p2mt_prev;
> > +    int rc = -EINVAL;
> > +    unsigned long prev_mfn, mfn = 0;
> > +    struct domain *fdom, *currd = current->domain;
> > +
> > +    if ( (fdom = get_pg_owner(foreign_domid)) == NULL )
> > +        return -EPERM;
> > +
> > +    mfn = mfn_x(get_gfn_query(fdom, fgfn, &p2mt));
> > +    if ( !mfn_valid(mfn) || !p2m_is_valid(p2mt) )
> > +        goto out_rc;
> > +
> > +    if ( !get_page_from_pagenr(mfn, fdom) )
> > +        goto out_rc;
> 
> I think you can use get_page_from_gfn() here instead of doing the
> translation and the get_page() by hand.  That way you don't need to
> worry about the put_gfn().  Which is just as well, as otherwise the
> second get_gfn() might deadlock if fdom == currd.

please see below.

> > +    /* Remove previously mapped page if it is present. */
> > +    prev_mfn = mfn_x(get_gfn(currd, gpfn, &p2mt_prev));
> > +    if ( mfn_valid(prev_mfn) )
> > +    {
> > +        if ( is_xen_heap_mfn(prev_mfn) )
> > +            /* Xen heap frames are simply unhooked from this phys
> > slot */
> > +            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
> > +        else
> > +            /* Normal domain memory is freed, to avoid leaking
> > memory. */
> > +            guest_remove_page(currd, gpfn);
> > +    }
> > +    put_gfn(currd, gpfn);
> 
> Again, without the p2m lock held, another CPU could populate gpfn
> between here and set_foreign_p2m_entry().

Ok, I suppose I should move the put_gfn. Here's what I got now:


/* 
 * Add frames from foreign domain to current domain's physmap. Similar to 
 * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
 * and is not removed from foreign domain. 
 * Usage: libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap.
 * Side Effect: the mfn for fgfn will be refcounted so it is not lost
 *              while mapped here. The refcnt is released in do_memory_op() 
 *              via XENMEM_remove_from_physmap.
 * Returns: 0 ==> success
 */
static int xenmem_add_foreign_to_pmap(domid_t foreign_domid, 
                                      unsigned long fgfn, unsigned long gpfn)
{
    p2m_type_t p2mt, p2mt_prev;
    int rc = -EINVAL;
    unsigned long prev_mfn, mfn = 0;
    struct domain *fdom, *currd = current->domain;
    struct page_info *page = NULL;

    if ( currd->domain_id == foreign_domid || foreign_domid == DOMID_SELF )
        return -EINVAL;

    if ( !IS_PRIV(currd) || (fdom = get_pg_owner(foreign_domid)) == NULL )
        return -EPERM;

    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
    if ( !page || !p2m_is_valid(p2mt) )
    {
        if ( page )
            put_page(page);
        put_pg_owner(fdom);
        return rc;
    }

    /* Remove previously mapped page if it is present. */
    prev_mfn = mfn_x(get_gfn(currd, gpfn, &p2mt_prev));
    if ( mfn_valid(prev_mfn) )
    {
        if ( is_xen_heap_mfn(prev_mfn) )
            /* Xen heap frames are simply unhooked from this phys slot */
            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
        else
            /* Normal domain memory is freed, to avoid leaking memory. */
            guest_remove_page(currd, gpfn);
    }

    /*
     * Create the new mapping. Can't use guest_physmap_add_page() because it 
     * will update the m2p table which will result in  mfn -> gpfn of dom0 
     * and not fgfn of domU.
     */
    if ( set_foreign_p2m_entry(currd, gpfn, _mfn(mfn)) == 0 ) 
    {
        dprintk(XENLOG_WARNING,
                "guest_physmap_add_page failed. gpfn:%lx mfn:%lx fgfn:%lx\n", 
                gpfn, mfn, fgfn);
        put_page(page);
    } 
    else
        rc = 0;

    /*
     * We must do this put_gfn after set_foreign_p2m_entry so another cpu
     * doesn't populate the gpfn before us.
     */
    put_gfn(currd, gpfn);

    put_pg_owner(fdom);
    return rc;
}