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

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

On 12/03/2013 02:30 AM, Mukesh Rathor wrote:
> In this patch, a new function, xenmem_add_foreign_to_pmap(), is added

xenmem_add_foreign_to_p2m?

> 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.

Your remove path is very interesting for the ARM port. For now we are 
unable to unmap the foreign page because get_page() will always return 
NULL (as dom0 is not the owner).

I will give a try on ARM to see if it could resolve our problem.

> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>   xen/arch/x86/mm.c   |   88 ++++++++++++++++++++++++++++++++++++++++++++++----
>   xen/common/memory.c |   38 +++++++++++++++++++---
>   2 files changed, 114 insertions(+), 12 deletions(-)

[..]

> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 50b740f..d81df18 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;
> +        struct domain *d, *foreign_dom = NULL;
> +        p2m_type_t p2mt, tp;
>
>           if ( copy_from_guest(&xrfp, arg, 1) )
>               return -EFAULT;
> @@ -693,11 +695,37 @@ 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_pmap().

s/xenmem_add_foreign_pmap/xenmem_add_foreign_p2m/

> +         */
> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> +
> +        if ( page || p2m_is_foreign(p2mt) )
>           {

p2m_is_foreign doesn't exist on ARM. I plan to introduce p2m type in the 
future, so I think you can define p2m_is_foreign as 0 for now on ARM.

> -            guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
> -            put_page(page);
> +            if ( page )
> +                mfn = page_to_mfn(page);
> +            else
> +            {
> +                mfn = mfn_x(get_gfn_query(d, xrfp.gpfn, &tp));

get_gfn_query doesn't exist on ARM.

> +                foreign_dom = page_get_owner(mfn_to_page(mfn));
> +                ASSERT(is_pvh_domain(d));

On ARM, the assert will always be wrong.

> +                ASSERT(d != foreign_dom);
> +                ASSERT(p2m_is_foreign(tp));
> +            }
> +
> +            guest_physmap_remove_page(d, xrfp.gpfn, mfn, 0);
> +            if (page)
> +                put_page(page);
> +
> +            if ( p2m_is_foreign(p2mt) )
> +            {
> +                put_page(mfn_to_page(mfn));
> +                put_gfn(d, xrfp.gpfn);
> +            }
>           }
>           else
>               rc = -ENOENT;
>

-- 
Julien Grall