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

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

On Wed, 4 Dec 2013 09:43:26 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:

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

Not sure. We are just checking few asserts in the remove path here.
get_page_from_gfn gets called from many non-relevant places.