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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2013-12-04 at 00:00 +0000, Julien Grall wrote:
> 
> 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.

Shouldn't almost all of this logic be part of the per arch
get_page_from_gfn?

> > +                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;
> >
>