From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH 02/20] PVH xen: add XENMEM_add_to_physmap_range Date: Thu, 16 May 2013 16:56:39 -0700 Message-ID: <20130516165639.4697b751@mantra.us.oracle.com> References: <1368579168-30829-1-git-send-email-mukesh.rathor@oracle.com> <1368579168-30829-3-git-send-email-mukesh.rathor@oracle.com> <5193787302000078000D659C@nat28.tlf.novell.com> <20130515160500.337cd014@mantra.us.oracle.com> <5194A50C02000078000D6A14@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5194A50C02000078000D6A14@nat28.tlf.novell.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: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On Thu, 16 May 2013 08:21:16 +0100 "Jan Beulich" wrote: > >>> On 16.05.13 at 01:05, Mukesh Rathor > >>> wrote: > > On Wed, 15 May 2013 10:58:43 +0100 > > "Jan Beulich" wrote: > > > >> >>> On 15.05.13 at 02:52, Mukesh Rathor > >> >>> wrote: > >> > --- a/xen/arch/x86/mm.c > >> > +++ b/xen/arch/x86/mm.c > >> > @@ -4519,7 +4519,8 @@ static int handle_iomem_range(unsigned long > >> > s, unsigned long e, void *p) > >> > static int xenmem_add_to_physmap_once( > >> > struct domain *d, > >> > - const struct xen_add_to_physmap *xatp) > >> > + const struct xen_add_to_physmap *xatp, > >> > + domid_t foreign_domid) > >> > { > >> > struct page_info *page = NULL; > >> > unsigned long gfn = 0; /* gcc ... */ > >> > @@ -4646,7 +4647,7 @@ static int xenmem_add_to_physmap(struct > >> > domain *d, > >> > >> I know I said this before: This patch can't be complete, or else > >> the new function parameter would actually get used. With the way > >> things are, if this patch gets applied, a user of the new XENMEM_ > >> sub-op would not get the expected behavior. > >> > > > > No, the new foreign_domid parameter is meaningful for only the > > XENMAPSPACE_gmfn_foreign OP which is defined in patch 0018. So we > > should be OK here. > > Mukesh, please. Go look at your own patch again: It adds handling > of XENMEM_add_to_physmap_range to arch_memory_op(), calling > xenmem_add_to_physmap_range(), which in turn calls > xenmem_add_to_physmap_once() passing xatpr->foreign_domid > as the last argument. I don't see anywhere in the patch prevention > of that execution flow for an arbitrary guest. If I'm overlooking > something, please point me to it. Hmm..., looking at the code again, xenmem_add_to_physmap_once() will start with setting mfn=0, then in the case statement, it won't find XENMAPSPACE_gmfn_foreign, so will get out of switch to : if ( !paging_mode_translate(d) || (mfn == 0) ) Since mfn is 0, it will return -EINVAL. The error would be copied to the xatpr->errs and guest will see that. What am I missing? I can add full support in just one patch, adding patch 18 here, but then in the past I was asked to break big patches. > Furthermore, now that you forced me to look at that code yet > another time, > > >+ xen_ulong_t idx; > >+ xen_pfn_t gpfn; > > Pointless variables, ... > > >+ struct xen_add_to_physmap xatp; > >+ > >+ if ( copy_from_guest_offset(&idx, xatpr->idxs, > >xatpr->size-1, 1) || > >+ copy_from_guest_offset(&gpfn, xatpr->gpfns, > >xatpr->size-1, 1) ) > > ... you can read directly into the respective xatp fields here. I could, but it makes the lines long/wrap in the if statement making the code harder to read IMO. The compiler should do exact same thing in both cases. If it really bothers you, I can change it. thanks Mukesh