Re: [V5 PATCH 6/7] pvh dom0: Add and remove foreign pages

[Julien Grall <julien.grall@linaro.org>]

On 12/05/2013 02:05 AM, Mukesh Rathor wrote:
> In this patch, a new function, xenmem_add_foreign_to_p2m(), is added
> to map pages from foreign guest into current dom0 for domU creation.
> Such pages are typed p2m_map_foreign. Also, support is added here to
> XENMEM_remove_from_physmap to remove such pages. Note, in the remove
> path, we must release the refcount that was taken during the map phase.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>   xen/arch/x86/mm.c         |   88 +++++++++++++++++++++++++++++++++++++++++----
>   xen/common/memory.c       |   40 ++++++++++++++++++--
>   xen/include/asm-arm/p2m.h |    2 +
>   3 files changed, 119 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e3da479..1a4d564 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2810,7 +2810,7 @@ static struct domain *get_pg_owner(domid_t domid)
>           goto out;
>       }
>
> -    if ( unlikely(paging_mode_translate(curr)) )
> +    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
>       {
>           MEM_LOG("Cannot mix foreign mappings with translated domains");
>           goto out;
> @@ -4520,9 +4520,75 @@ static int handle_iomem_range(unsigned long s, unsigned long e, void *p)
>       return 0;
>   }
>
> +/*
> + * Add frames from foreign domain to target 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_p2m(struct domain *tdom, unsigned long fgfn,
> +                                     unsigned long gpfn, struct domain *fdom)
> +{
> +    p2m_type_t p2mt, p2mt_prev;
> +    int rc = 0;
> +    unsigned long prev_mfn, mfn = 0;
> +    struct page_info *page = NULL;
> +
> +    if ( tdom == fdom || !tdom || !fdom || !is_pvh_domain(tdom) )
> +        return -EINVAL;
> +
> +    /* following will take a refcnt on the mfn */
> +    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
> +    if ( !page || !p2m_is_valid(p2mt) )
> +    {
> +        if ( page )
> +            put_page(page);
> +        return -EINVAL;
> +    }
> +    mfn = page_to_mfn(page);
> +
> +    /* Remove previously mapped page if it is present. */
> +    prev_mfn = mfn_x(get_gfn(tdom, 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(tdom, gpfn, prev_mfn, 0);
> +        else
> +            /* Normal domain memory is freed, to avoid leaking memory. */
> +            guest_remove_page(tdom, 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(tdom, gpfn, _mfn(mfn)) == 0 )
> +    {
> +        gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
> +                 "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
> +                 gpfn, mfn, fgfn, tdom->domain_id, fdom->domain_id);
> +        put_page(page);
> +        rc = -EINVAL;
> +    }
> +
> +    /*
> +     * We must do this put_gfn after set_foreign_p2m_entry so another cpu
> +     * doesn't populate the gpfn before us.
> +     */
> +    put_gfn(tdom, gpfn);
> +
> +    return rc;
> +}
> +
>   static int xenmem_add_to_physmap_once(
>       struct domain *d,
> -    const struct xen_add_to_physmap *xatp)
> +    const struct xen_add_to_physmap *xatp,
> +    struct domain *fdom)
>   {
>       struct page_info *page = NULL;
>       unsigned long gfn = 0; /* gcc ... */
> @@ -4581,6 +4647,13 @@ static int xenmem_add_to_physmap_once(
>               page = mfn_to_page(mfn);
>               break;
>           }
> +
> +        case XENMAPSPACE_gmfn_foreign:
> +        {
> +            rc = xenmem_add_foreign_to_p2m(d, xatp->idx, xatp->gpfn, fdom);
> +            return rc;
> +        }
> +
>           default:
>               break;
>       }
> @@ -4646,7 +4719,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>           start_xatp = *xatp;
>           while ( xatp->size > 0 )
>           {
> -            rc = xenmem_add_to_physmap_once(d, xatp);
> +            rc = xenmem_add_to_physmap_once(d, xatp, NULL);
>               if ( rc < 0 )
>                   return rc;
>
> @@ -4672,11 +4745,12 @@ static int xenmem_add_to_physmap(struct domain *d,
>           return rc;
>       }
>
> -    return xenmem_add_to_physmap_once(d, xatp);
> +    return xenmem_add_to_physmap_once(d, xatp, NULL);
>   }
>
>   static int xenmem_add_to_physmap_range(struct domain *d,
> -                                       struct xen_add_to_physmap_range *xatpr)
> +                                       struct xen_add_to_physmap_range *xatpr,
> +                                       struct domain *fdom)
>   {
>       /* Process entries in reverse order to allow continuations */
>       while ( xatpr->size > 0 )
> @@ -4693,7 +4767,7 @@ static int xenmem_add_to_physmap_range(struct domain *d,
>           xatp.space = xatpr->space;
>           xatp.idx = idx;
>           xatp.gpfn = gpfn;
> -        rc = xenmem_add_to_physmap_once(d, &xatp);
> +        rc = xenmem_add_to_physmap_once(d, &xatp, fdom);
>
>           if ( copy_to_guest_offset(xatpr->errs, xatpr->size-1, &rc, 1) )
>               return -EFAULT;
> @@ -4780,7 +4854,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>           }
>           rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d, fd);
>           if ( rc == 0 )
> -            rc = xenmem_add_to_physmap_range(d, &xatpr);
> +            rc = xenmem_add_to_physmap_range(d, &xatpr, fd);
>
>           rcu_unlock_domain(d);
>           if ( fd )
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index eb7b72b..ae11828 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -675,9 +675,11 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>
>       case XENMEM_remove_from_physmap:
>       {
> +        unsigned long mfn;
>           struct xen_remove_from_physmap xrfp;
>           struct page_info *page;
>           struct domain *d;
> +        p2m_type_t p2mt;
>
>           if ( copy_from_guest(&xrfp, arg, 1) )
>               return -EFAULT;
> @@ -693,11 +695,41 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>               return rc;
>           }
>
> -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> -        if ( page )
> +        /*
> +         * if PVH, the gfn could be mapped to a mfn from foreign domain by the
> +         * user space tool during domain creation. We need to check for that,
> +         * free it up from the p2m, and release refcnt on it. In such a case,
> +         * page would be NULL and the following call would not have refcnt'd
> +         * the page. See also xenmem_add_foreign_to_p2m().
> +         */
> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);

Actually, on debug build, get_page_from_gfn on ARM will crash Xen. For 
now we have an ASSERT to check if the type (p2m_type_t *) is NULL.

-- 
Julien Grall