From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 02/20] PVH xen: add XENMEM_add_to_physmap_range Date: Thu, 16 May 2013 12:03:27 +0100 Message-ID: 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 , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Thu, 16 May 2013, 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. > > 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. > > >+ { > >+ return -EFAULT; > >+ } > > Pointless (and inconsistent with code further down in this same > function) braces. > > >+ > >+ xatp.space = xatpr->space; > >+ xatp.idx = idx; > >+ xatp.gpfn = gpfn; > >+ rc = xenmem_add_to_physmap_once(d, &xatp, xatpr->foreign_domid); > > xatp has a domid field - why don't you use that instead of adding a > new function parameter? I'm unclear anyway why two domain IDs > are useful here at all - Ian, Stefano, for one I still can't spot any use > of xen_add_to_physmap_range in tools and qemu (and hence can't > see a clear use case), and then I doubt there's real use for one > domain mapping GFNs from a second domain into a third one. If it's > really dead code that got added here, shouldn't we drop it now > rather than releasing 4.3 with it baked into the interface? We use XENMEM_add_to_physmap_range to map foreign mfns in dom0 during domain creation. > Jan >