From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 02/20] PVH xen: add XENMEM_add_to_physmap_range Date: Thu, 16 May 2013 08:21:16 +0100 Message-ID: <5194A50C02000078000D6A14@nat28.tlf.novell.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130515160500.337cd014@mantra.us.oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Mukesh Rathor Cc: xen-devel , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org >>> 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? Jan