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