From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olaf Hering Subject: Re: [PATCH 12/17] xenpaging: handle HVMCOPY_gfn_paged_out in copy_from/to_user Date: Tue, 7 Dec 2010 10:45:35 +0100 Message-ID: <20101207094535.GA23113@aepfle.de> References: <20101206205907.848643876@aepfle.de> <20101206205912.343173055@aepfle.de> <4CFE0BF90200007800026536@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Content-Disposition: inline In-Reply-To: <4CFE0BF90200007800026536@vpn.id2.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: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Tue, Dec 07, Jan Beulich wrote: > >>> On 06.12.10 at 21:59, Olaf Hering wrote: > > --- xen-unstable.hg-4.1.22459.orig/xen/arch/x86/mm/guest_walk.c > > +++ xen-unstable.hg-4.1.22459/xen/arch/x86/mm/guest_walk.c > > @@ -93,11 +93,12 @@ static inline void *map_domain_gfn(struc > > uint32_t *rc) > > { > > /* Translate the gfn, unsharing if shared */ > > +retry: > > *mfn = gfn_to_mfn_unshare(p2m, gfn_x(gfn), p2mt, 0); > > if ( p2m_is_paging(*p2mt) ) > > { > > - p2m_mem_paging_populate(p2m, gfn_x(gfn)); > > - > > + if ( p2m_mem_paging_populate(p2m, gfn_x(gfn)) ) > > + goto retry; > > *rc = _PAGE_PAGED; > > return NULL; > > } > > Is this retry loop (and similar ones later in the patch) guaranteed > to be bounded in some way? This needs to be fixed, yes. For the plain __hvm_copy case, with nothing else being modified, the 'return HVMCOPY_gfn_paged_out' could be just a 'continue'. But even then, something needs to break the loop. > > /* gfn is already on its way back and vcpu is not paused */ > > - return; > > + goto populate_out; > > Do you really need a goto here (i.e. are you foreseeing to get stuff > added between the label and the return below)? Thats something for my debug patch, I have a trace_var at the end of each function. > > +/* retval 1 means the page is present on return */ > > +int p2m_mem_paging_populate(struct p2m_domain *p2m, unsigned long gfn); > > Isn't this a case where you absolutely need the return value checked? > If so, you will want to add __must_check here. Yes, that would be a good addition. Maybe the wait_event/wake_up could be done unconditionally, independent if the p2m domain differs from the vcpu domain. Olaf >