Re: [Xen-devel] [PATCH 02/20] PVH xen: add XENMEM_add_to_physmap_range

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 16 May 2013, Jan Beulich wrote:
> >>> On 16.05.13 at 01:05, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> > On Wed, 15 May 2013 10:58:43 +0100
> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> > 
> >> >>> On 15.05.13 at 02:52, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >> >>> 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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel