From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Deegan Subject: Re: [PATCH 4/6] mm: New XENMEM space, XENMAPSPACE_gmfn_range Date: Thu, 10 Nov 2011 10:15:18 +0000 Message-ID: <20111110101518.GC62117@ocelot.phlegethon.org> References: <1320914644-4357-1-git-send-email-jean.guyader@eu.citrix.com> <1320914644-4357-2-git-send-email-jean.guyader@eu.citrix.com> <1320914644-4357-3-git-send-email-jean.guyader@eu.citrix.com> <1320914644-4357-4-git-send-email-jean.guyader@eu.citrix.com> <1320914644-4357-5-git-send-email-jean.guyader@eu.citrix.com> <4EBBAD770200007800060155@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Return-path: Content-Disposition: inline In-Reply-To: <4EBBAD770200007800060155@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: keir@xen.org, xen-devel@lists.xensource.com, allen.m.kay@intel.com, Jean Guyader List-Id: xen-devel@lists.xenproject.org At 09:54 +0000 on 10 Nov (1320918887), Jan Beulich wrote: > >>> On 10.11.11 at 09:44, Jean Guyader wrote: > > In the native implementation I neither see the XENMAPSPACE_gmfn_range > case getting actually handled in the main switch (did you mean to change > xatp.space to XENMAPSPACE_gmfn in that case?), nor do I see how you > communicate back how many of the pages were successfully processed in > the event of an error in the middle of the processing or when a > continuation is required. > > But with the patch being pretty hard to read, maybe I'm simply > overlooking something? The patch changes the (compat-translated) hypercall arguments in place to reflect what's been done. Agreed that it's particularly hard to read, though. Tim. > Further (I realize I should have commented on this earlier) I think that > in order to allow forward progress you should not check for preemption > on the very first iteration of each (re-)invocation. That would also > guarantee no behavioral change to the original single-page variants. > > >--- a/xen/arch/x86/x86_64/compat/mm.c > >+++ b/xen/arch/x86/x86_64/compat/mm.c > >@@ -63,6 +63,16 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) > > > > XLAT_add_to_physmap(nat, &cmp); > > rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); > >+ if ( rc < 0 ) > >+ return rc; > >+ > >+ if ( rc == __HYPERVISOR_memory_op ) > >+ hypercall_xlat_continuation(NULL, 0x2, nat, arg); > >+ > >+ XLAT_add_to_physmap(&cmp, nat); > >+ > >+ if ( copy_to_guest(arg, &cmp, 1) ) > >+ return -EFAULT; > > Other than in the XENMEM_[gs]et_pod_target you (so far, subject to the > above comment resulting in a behavioral change) don't have any real > outputs here, and hence there's no need to always to the outbound > translation - i.e. all of this could be moved into the if ()'s body. > > Jan > > > > > break; > > } >