From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH] Paging and memory sharing for HVM guests Date: Thu, 17 Dec 2009 11:38:45 -0500 Message-ID: <20091217163845.GA26398@phenom.dumpdata.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Grzegorz Milos Cc: Patrick Colp , xen-devel@lists.xensource.com, Andrew Peace , Keir Fraser List-Id: xen-devel@lists.xenproject.org On Wed, Dec 16, 2009 at 11:14:47PM +0000, Grzegorz Milos wrote: > The series of 46 patches attached to this email contain the initial > implementation of memory paging and sharing for Xen. Patrick Colp > leads the work on the pager, and I am mostly responsible for memory > sharing. We would be grateful for any comments/suggestions you might > have. Individual patches are labeled with comments describing their > purpose and a sign-off footnote. Of course we are happy to discuss > them in more detail, as required. Assuming that there are no major > objections against including them in the mainstream xen-unstable tree, > we would like to move future development to that tree. Congrats! And now to the tiny review I did: 1). mem_event_xen_core.patch "Copyright (c) 2009 Citrix (R)&D) Ltd. (Patrick Colp)" The lawyers might not like it the copyright being assigned to a non-existent entity :-( 2). mem_paging_xen_pfec_page_paged.patch: 15 + if ( p2m_is_paging(p2mt) ) 16 + { 17 +// if ( p2m_is_paged(p2mt) ) I think you can safely remove the commented out code. 3). mem_sharing_xen_mm.patch: 27 + /* 28 + * Initialise our DOMID_IO domain. ^^->COW 29 + * This domain owns sharable pages. 30 + */ 31 + dom_cow = domain_create(DOMID_COW, DOMCRF_dummy, 0); 4), mem_sharing_tools_eagain.patch The code spins with a 1 second timeout forever. Would it make sense to include a retry counter so that, say after an hour (or maybe something much smaller) you give up? In the pv-ops patch series: 1). The "*mfnp |= 0x80000000U;" and "*mfnp |= 0xf0000000U;" should use a #define. Maybe copy over the #defines from the xen tree ?