From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Xen-Devel] [PATCH] [GSOC14] refactored mempaging code from xenpaging to libxc. Date: Thu, 12 Jun 2014 15:46:09 +0100 Message-ID: <5399BD31.506@citrix.com> References: <1402565030-22004-1-git-send-email-myselfdushyantbehl@gmail.com> <5399A984.6010300@citrix.com> <5399B529.7030608@citrix.com> <1402582909.26678.12.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: <1402582909.26678.12.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: dave.scott@eu.citrix.com, xen-devel@lists.xen.org, Dushyant Behl , Andres Lagar Cavilla , stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 12/06/14 15:21, Ian Campbell wrote: > On Thu, 2014-06-12 at 15:11 +0100, Andrew Cooper wrote: > > >>> >>> > + if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB ) >>> >>> >>> What is this check for? >>> foreign_batch has the old semantics of setting the MSB of the pfn in >>> error. Maybe use map_foreign_bulk with cleaner error reporting? >>> >> Eww. That is a nasty interface, and will completely break 32bit >> toolstacks on machines with hotplug regions above the 8TB boundary. >> >> Please do use map_foreign_bulk(). > The stated purpose of this patch is code motion. Please don't encourage > people to also make functional changes in such patches. > > If there is to be any cleanups/improvements then they should be done > later, but that won't affect the acceptance of this patch (assuming it > really is just motion, I've not checked in detail). > > Ian. > Refactoring is not necessarily code motion, although in this case it does appear to be almost exclusively motion. However, it is motion of code from an example utility into a main library, and I do not feel it is appropriate to be moving code with bugs like this into libxc. (Perhaps I am overly jaded by the amount of fixing up the migration code needed in areas like this) Two solutions come to mind: either change this patch to be "implement xenpaging initialisation in libxenguest" and fix the code up as it goes in, or fix it up in xenpaging and move it as a second patch. ~Andrew