From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vitaly Kuznetsov Subject: Re: [PATCH v7 04/10] xen: Introduce XEN_DOMCTL_soft_reset Date: Thu, 28 May 2015 15:53:36 +0200 Message-ID: <87zj4ooljz.fsf@vitty.brq.redhat.com> References: <1432740346-7887-1-git-send-email-vkuznets@redhat.com> <1432740346-7887-5-git-send-email-vkuznets@redhat.com> <20150528100642.GB24442@deinos.phlegethon.org> <87h9qwq5kf.fsf@vitty.brq.redhat.com> <20150528125237.GD24442@deinos.phlegethon.org> <878uc8q21s.fsf@vitty.brq.redhat.com> <20150528133739.GA40491@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YxyFn-00031C-RB for xen-devel@lists.xenproject.org; Thu, 28 May 2015 13:53:47 +0000 In-Reply-To: <20150528133739.GA40491@deinos.phlegethon.org> (Tim Deegan's message of "Thu, 28 May 2015 14:37:39 +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: Tim Deegan Cc: Wei Liu , Andrew Jones , Julien Grall , Keir Fraser , Ian Campbell , Stefano Stabellini , Andrew Cooper , Ian Jackson , Olaf Hering , David Vrabel , Jan Beulich , xen-devel@lists.xenproject.org, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org Tim Deegan writes: > At 15:11 +0200 on 28 May (1432825919), Vitaly Kuznetsov wrote: >> Tim Deegan writes: >> > At 13:56 +0200 on 28 May (1432821360), Vitaly Kuznetsov wrote: >> >> Tim Deegan writes: >> >> >> + while ( next_page && !is_xen_heap_page(next_page) && >> >> >> + page_to_mfn(next_page) == mfn + count ) >> >> > >> >> > What's the purpose of this second loop? It doesn't seem to be doing >> >> > anything that the outer loop couldn't. >> >> >> >> True. This second loops searches for a continuous region to preserve the >> >> order of mappings (when possible) >> > >> > Ah; I think this, like the PoD case, should the more detailed p2m >> > lookup to get the actual order of the current mapping. That should be >> > shorter and more readable, and probably more correct. >> >> If we bring get_gfn_type_access() call to the top level it becomes >> possible (and easy) but (if I'm not mistaken) we still need to walk >> through all pages of the mapping checking that each of them has the >> reqiuired count_info (so it is not mapped from other domain or xen >> itself). In case we meet a 'bad' page we'll have to split the mapping >> (and copy the page itself). > > Hmmm. Yes, we can only reassign one page at a time. I think that > will look cleaner if you split out the reassign-or-copy into a > separate function that takes a start + order and DTRT, and then having > the loop in this function handle one p2m entry (of whatever order) per > iteration. > > BTW having looked at how messy this is ending up, and how it's still > incomplete, I'd agree with Jan that resetting the domain state might > be a better approach. Even with the 'reset-everything' approach the function from this patch will still be required in some form as we'll still have to walk the p2m and each individual page checking it's count_info and replacing in case of need. At the same time we'll have lots of other hypervisor-related implications (tearing down everything) so I seriously doubt it's going to end up less messy (toolstack-related changes might go away though). -- Vitaly