From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [V4 PATCH 6/7] pvh dom0: Add and remove foreign pages Date: Wed, 4 Dec 2013 09:43:26 +0000 Message-ID: <1386150206.15530.3.camel@kazak.uk.xensource.com> References: <1386037848-27891-1-git-send-email-mukesh.rathor@oracle.com> <1386037848-27891-7-git-send-email-mukesh.rathor@oracle.com> <529E70B6.7060002@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <529E70B6.7060002@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: Xen-devel@lists.xensource.com, Stefano Stabellini , george.dunlap@eu.citrix.com, tim@xen.org, keir.xen@gmail.com, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org 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 > > --- > > 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; > > >