From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vitaly Kuznetsov Subject: Re: [PATCH RFC/WIPv2 1/6] Introduce XENMEM_transfer operation Date: Wed, 24 Sep 2014 17:13:02 +0200 Message-ID: <87a95p6oht.fsf@vitty.brq.redhat.com> References: <1411568455-27113-1-git-send-email-vkuznets@redhat.com> <1411568455-27113-2-git-send-email-vkuznets@redhat.com> <5422DE1D.9060305@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XWoFk-00072f-C0 for xen-devel@lists.xenproject.org; Wed, 24 Sep 2014 15:13:12 +0000 In-Reply-To: <5422DE1D.9060305@citrix.com> (Andrew Cooper's message of "Wed, 24 Sep 2014 16:07:09 +0100") List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: xen-devel@lists.xenproject.org, Andrew Jones , David Vrabel , Jan Beulich List-Id: xen-devel@lists.xenproject.org Andrew Cooper writes: > On 24/09/14 15:20, Vitaly Kuznetsov wrote: >> New operation reassigns pages from one domain to the other mapping them >> at exactly the same GFNs in the destination domain. Pages mapped more >> than once (e.g. granted pages) are being copied. >> >> Signed-off-by: Vitaly Kuznetsov >> --- >> xen/common/memory.c | 178 ++++++++++++++++++++++++++++++++++++++++++++ >> xen/include/public/memory.h | 32 +++++++- >> 2 files changed, 209 insertions(+), 1 deletion(-) >> >> diff --git a/xen/common/memory.c b/xen/common/memory.c >> index 2e3225d..653e117 100644 >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -578,6 +578,180 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) >> return rc; >> } >> >> +static long memory_transfer(XEN_GUEST_HANDLE_PARAM(xen_memory_transfer_t) arg) >> +{ >> + long rc = 0; >> + struct xen_memory_transfer trans; >> + struct domain *source_d, *dest_d; >> + unsigned long mfn, gmfn, last_gmfn; >> + p2m_type_t p2mt; >> + struct page_info *page, *new_page; >> + char *sp, *dp; >> + int copying; >> + >> + if ( copy_from_guest(&trans, arg, 1) ) >> + return -EFAULT; >> + >> + source_d = rcu_lock_domain_by_any_id(trans.source_domid); >> + if ( source_d == NULL ) >> + { >> + rc = -ESRCH; >> + goto fail_early; >> + } >> + >> + if ( source_d->is_dying ) >> + { >> + rc = -EINVAL; >> + rcu_unlock_domain(source_d); >> + goto fail_early; >> + } >> + >> + dest_d = rcu_lock_domain_by_any_id(trans.dest_domid); >> + if ( dest_d == NULL ) >> + { >> + rc = -ESRCH; >> + rcu_unlock_domain(source_d); >> + goto fail_early; >> + } >> + >> + if ( dest_d->is_dying ) >> + { >> + rc = -EINVAL; >> + goto fail; >> + } >> + >> + last_gmfn = trans.gmfn_start + trans.gmfn_count; >> + for ( gmfn = trans.gmfn_start; gmfn < last_gmfn; gmfn++ ) >> + { >> + page = get_page_from_gfn(source_d, gmfn, &p2mt, 0); >> + if ( !page ) >> + { >> + continue; >> + } >> + >> + mfn = page_to_mfn(page); >> + if ( !mfn_valid(mfn) ) >> + { >> + put_page(page); >> + continue; >> + } >> + >> + copying = 0; >> + >> + if ( is_xen_heap_mfn(mfn) ) >> + { >> + put_page(page); >> + continue; >> + } >> + >> + /* Page table always worth copying */ >> + if ( (page->u.inuse.type_info & PGT_l4_page_table) || >> + (page->u.inuse.type_info & PGT_l3_page_table) || >> + (page->u.inuse.type_info & PGT_l2_page_table) || >> + (page->u.inuse.type_info & PGT_l1_page_table) ) >> + copying = 1; > > How can copying pagetables like this ever work? You will end up with an > L4 belonging to the new domain pointing to L3's owned by the old domain. > > Even if you change the ownership of the pages pointed to by the L1's, as > soon as the old domain is torn down, the new domains pagetables will be > freed heap pages. Yes, I'm aware it is broken and that's actually why I sent this RFC - in my PATCH 0/6 letter the main question was: what's the best approach here with regards to PV? If we want to avoid copying and updating this pages we can do it while killing the original domain (so instead of this _transfer op we'll have special 'domain kill' op). Thanks, > > ~Andrew -- Vitaly