[PATCH 14/18 V2]: PVH xen: add xenmem_add_foreign_to_pmap()

[PATCH 14/18 V2]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

 In this patch, a new function, xenmem_add_foreign_to_pmap(), is added
 to map pages from foreign guest into current dom0 for domU creation.
 Also, allow XENMEM_remove_from_physmap to remove p2m_map_foreign
 pages. Note, in this path, we must release the refcount that was taken
 during the map phase.

Changes in V2:
  - Move the XENMEM_remove_from_physmap changes here instead of prev patch
  - Move grant changes from this to one of the next patches.
  - In xenmem_add_foreign_to_pmap(), do locked get_gfn
  - Fail the mappings for qemu mapping pages for memory not there.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/mm.c   |   74 ++++++++++++++++++++++++++++++++++++++++++++++++--
 xen/common/memory.c |   44 +++++++++++++++++++++++++++---
 2 files changed, 110 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6603752..dbac811 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2656,7 +2656,7 @@ static struct domain *get_pg_owner(domid_t domid)
         goto out;
     }
 
-    if ( unlikely(paging_mode_translate(curr)) )
+    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
     {
         MEM_LOG("Cannot mix foreign mappings with translated domains");
         goto out;
@@ -4192,7 +4192,7 @@ long do_update_descriptor(u64 pa, u64 desc)
     page = get_page_from_gfn(dom, gmfn, NULL, P2M_ALLOC);
     if ( (((unsigned int)pa % sizeof(struct desc_struct)) != 0) ||
          !page ||
-         !check_descriptor(dom, &d) )
+         (!is_pvh_domain(dom) && !check_descriptor(dom, &d)) )
     {
         if ( page )
             put_page(page);
@@ -4266,6 +4266,66 @@ static int handle_iomem_range(unsigned long s, unsigned long e, void *p)
     return 0;
 }
 
+/* 
+ * Add frames from foreign domain to current domain's physmap. Similar to 
+ * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
+ * and is not removed from foreign domain. 
+ * Usage: libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap.
+ * Side Effect: the mfn for fgfn will be refcounted so it is not lost
+ *              while mapped here. The refcnt is released in do_memory_op() 
+ *              via XENMEM_remove_from_physmap.
+ * Returns: 0 ==> success
+ */
+static int xenmem_add_foreign_to_pmap(domid_t foreign_domid, 
+                                      unsigned long fgfn, unsigned long gpfn)
+{
+    p2m_type_t p2mt, p2mt_prev;
+    int rc = -EINVAL;
+    unsigned long prev_mfn, mfn = 0;
+    struct domain *fdom, *currd = current->domain;
+
+    if ( (fdom = get_pg_owner(foreign_domid)) == NULL )
+        return -EPERM;
+
+    mfn = mfn_x(get_gfn_query(fdom, fgfn, &p2mt));
+    if ( !mfn_valid(mfn) || !p2m_is_valid(p2mt) )
+        goto out_rc;
+
+    if ( !get_page_from_pagenr(mfn, fdom) )
+        goto out_rc;
+
+    /* Remove previously mapped page if it is present. */
+    prev_mfn = mfn_x(get_gfn(currd, gpfn, &p2mt_prev));
+    if ( mfn_valid(prev_mfn) )
+    {
+        if ( is_xen_heap_mfn(prev_mfn) )
+            /* Xen heap frames are simply unhooked from this phys slot */
+            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
+        else
+            /* Normal domain memory is freed, to avoid leaking memory. */
+            guest_remove_page(currd, gpfn);
+    }
+    put_gfn(currd, gpfn);
+
+    /* Create the new mapping. Can't use guest_physmap_add_page() because it 
+     * will update the m2p table which will result in  mfn -> gpfn of dom0 
+     * and not fgfn of domU.
+     */
+    if ( set_foreign_p2m_entry(currd, gpfn, _mfn(mfn)) == 0 ) {
+
+        printk("guest_physmap_add_page failed. gpfn:%lx mfn:%lx fgfn:%lx\n", 
+               gpfn, mfn, fgfn);
+        put_page(mfn_to_page(mfn));
+        goto out_rc;
+    }
+    rc = 0;
+
+out_rc:
+    put_gfn(fdom, fgfn);
+    put_pg_owner(fdom);
+    return rc;
+}
+
 static int xenmem_add_to_physmap_once(
     struct domain *d,
     const struct xen_add_to_physmap *xatp,
@@ -4328,6 +4388,14 @@ static int xenmem_add_to_physmap_once(
             page = mfn_to_page(mfn);
             break;
         }
+
+        case XENMAPSPACE_gmfn_foreign:
+        {
+            rc = xenmem_add_foreign_to_pmap(foreign_domid, xatp->idx, 
+                                            xatp->gpfn);
+            return rc;
+        }
+
         default:
             break;
     }
@@ -4425,7 +4493,7 @@ static int xenmem_add_to_physmap(struct domain *d,
     return xenmem_add_to_physmap_once(d, xatp, -1);
 }
 
-static noinline int xenmem_add_to_physmap_range(struct domain *d,
+static int xenmem_add_to_physmap_range(struct domain *d,
                                        struct xen_add_to_physmap_range *xatpr)
 {
     int rc;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 68501d1..91a56b6 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -675,9 +675,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case XENMEM_remove_from_physmap:
     {
+        unsigned long argmfn, foreign_mfn = INVALID_MFN;
         struct xen_remove_from_physmap xrfp;
         struct page_info *page;
-        struct domain *d;
+        struct domain *d, *foreign_dom = NULL;
+        p2m_type_t p2mt, tp;
+        int valid_pvh_pg, is_curr_pvh = is_pvh_vcpu(current);
 
         if ( copy_from_guest(&xrfp, arg, 1) )
             return -EFAULT;
@@ -695,14 +698,45 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         domain_lock(d);
 
-        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
-        if ( page )
+        /* PVH note: if PVH, the gfn could be mapped to a mfn from foreign
+         * domain by the user space tool during domain creation. We need to
+         * check for that, free it up from the p2m, and release refcnt on it.
+         * In such a case, page would be NULL. */
+
+        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
+        valid_pvh_pg = is_curr_pvh && 
+                       (p2m_is_mmio(p2mt) || p2m_is_foreign(p2mt));
+
+        if ( page || valid_pvh_pg)
         {
-            guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
-            put_page(page);
+            argmfn = page ? page_to_mfn(page) : INVALID_MFN;
+
+            if ( is_curr_pvh && p2m_is_foreign(p2mt) )
+            {
+                foreign_mfn = mfn_x(get_gfn_query_unlocked(d, xrfp.gpfn, &tp));
+                foreign_dom = page_get_owner(mfn_to_page(foreign_mfn));
+                ASSERT(p2m_is_mmio(tp) || p2m_is_foreign(tp));
+            }
+
+            guest_physmap_remove_page(d, xrfp.gpfn, argmfn, 0);
+            if (page)
+                put_page(page);
+
+            /* if pages were mapped from foreign domain via 
+             * xenmem_add_foreign_to_pmap(), we must drop a refcnt here */
+            if ( is_curr_pvh && p2m_is_foreign(p2mt) )
+            {
+                ASSERT( d != foreign_dom );
+                put_page(mfn_to_page(foreign_mfn));
+            }
         }
         else
+        {
+            if ( is_curr_pvh )
+                gdprintk(XENLOG_WARNING, "%s: Domain:%u gmfn:%lx invalid\n",
+                         __func__, current->domain->domain_id, xrfp.gpfn);
             rc = -ENOENT;
+        }
 
         domain_unlock(d);
 
-- 
1.7.2.3

Re: [PATCH 14/18 V2]: PVH xen: add xenmem_add_foreign_to_pmap()

[Tim Deegan]

At 17:58 -0700 on 15 Mar (1363370311), Mukesh Rathor wrote:
>  In this patch, a new function, xenmem_add_foreign_to_pmap(), is added
>  to map pages from foreign guest into current dom0 for domU creation.
>  Also, allow XENMEM_remove_from_physmap to remove p2m_map_foreign
>  pages. Note, in this path, we must release the refcount that was taken
>  during the map phase.
> 
> Changes in V2:
>   - Move the XENMEM_remove_from_physmap changes here instead of prev patch
>   - Move grant changes from this to one of the next patches.
>   - In xenmem_add_foreign_to_pmap(), do locked get_gfn
>   - Fail the mappings for qemu mapping pages for memory not there.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/mm.c   |   74 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  xen/common/memory.c |   44 +++++++++++++++++++++++++++---
>  2 files changed, 110 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 6603752..dbac811 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2656,7 +2656,7 @@ static struct domain *get_pg_owner(domid_t domid)
>          goto out;
>      }
>  
> -    if ( unlikely(paging_mode_translate(curr)) )
> +    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
>      {
>          MEM_LOG("Cannot mix foreign mappings with translated domains");
>          goto out;
> @@ -4192,7 +4192,7 @@ long do_update_descriptor(u64 pa, u64 desc)
>      page = get_page_from_gfn(dom, gmfn, NULL, P2M_ALLOC);
>      if ( (((unsigned int)pa % sizeof(struct desc_struct)) != 0) ||
>           !page ||
> -         !check_descriptor(dom, &d) )
> +         (!is_pvh_domain(dom) && !check_descriptor(dom, &d)) )

Is this change related to xenmem_add_foreign_to_pmap() ?

>      {
>          if ( page )
>              put_page(page);
> @@ -4266,6 +4266,66 @@ static int handle_iomem_range(unsigned long s, unsigned long e, void *p)
>      return 0;
>  }
>  
> +/* 
> + * Add frames from foreign domain to current domain's physmap. Similar to 
> + * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
> + * and is not removed from foreign domain. 
> + * Usage: libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap.
> + * Side Effect: the mfn for fgfn will be refcounted so it is not lost
> + *              while mapped here. The refcnt is released in do_memory_op() 
> + *              via XENMEM_remove_from_physmap.
> + * Returns: 0 ==> success
> + */
> +static int xenmem_add_foreign_to_pmap(domid_t foreign_domid, 
> +                                      unsigned long fgfn, unsigned long gpfn)
> +{
> +    p2m_type_t p2mt, p2mt_prev;
> +    int rc = -EINVAL;
> +    unsigned long prev_mfn, mfn = 0;
> +    struct domain *fdom, *currd = current->domain;
> +
> +    if ( (fdom = get_pg_owner(foreign_domid)) == NULL )
> +        return -EPERM;
> +
> +    mfn = mfn_x(get_gfn_query(fdom, fgfn, &p2mt));
> +    if ( !mfn_valid(mfn) || !p2m_is_valid(p2mt) )
> +        goto out_rc;
> +
> +    if ( !get_page_from_pagenr(mfn, fdom) )
> +        goto out_rc;

I think you can use get_page_from_gfn() here instead of doing the
translation and the get_page() by hand.  That way you don't need to
worry about the put_gfn().  Which is just as well, as otherwise the
second get_gfn() might deadlock if fdom == currd.

> +    /* Remove previously mapped page if it is present. */
> +    prev_mfn = mfn_x(get_gfn(currd, gpfn, &p2mt_prev));
> +    if ( mfn_valid(prev_mfn) )
> +    {
> +        if ( is_xen_heap_mfn(prev_mfn) )
> +            /* Xen heap frames are simply unhooked from this phys slot */
> +            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
> +        else
> +            /* Normal domain memory is freed, to avoid leaking memory. */
> +            guest_remove_page(currd, gpfn);
> +    }
> +    put_gfn(currd, gpfn);

Again, without the p2m lock held, another CPU could populate gpfn
between here and set_foreign_p2m_entry().

> +    /* Create the new mapping. Can't use guest_physmap_add_page() because it 
> +     * will update the m2p table which will result in  mfn -> gpfn of dom0 
> +     * and not fgfn of domU.
> +     */
> +    if ( set_foreign_p2m_entry(currd, gpfn, _mfn(mfn)) == 0 ) {
> +
> +        printk("guest_physmap_add_page failed. gpfn:%lx mfn:%lx fgfn:%lx\n", 
> +               gpfn, mfn, fgfn);
> +        put_page(mfn_to_page(mfn));
> +        goto out_rc;
> +    }
> +    rc = 0;
> +
> +out_rc:
> +    put_gfn(fdom, fgfn);
> +    put_pg_owner(fdom);
> +    return rc;
> +}
> +
>  static int xenmem_add_to_physmap_once(
>      struct domain *d,
>      const struct xen_add_to_physmap *xatp,
> @@ -4328,6 +4388,14 @@ static int xenmem_add_to_physmap_once(
>              page = mfn_to_page(mfn);
>              break;
>          }
> +
> +        case XENMAPSPACE_gmfn_foreign:
> +        {
> +            rc = xenmem_add_foreign_to_pmap(foreign_domid, xatp->idx, 
> +                                            xatp->gpfn);
> +            return rc;

I don't see any access control on this hypercall.  I'd expect to see an
IS_PRIV() somewhere, or equivalent xsm runes. 

Also, is it intended to be restricted to PVH domains or available to HVM
domains too?  If it's for everyone, the unwind code below shouldn't gate
on is_pvh_vcpu(current).

> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 68501d1..91a56b6 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -675,9 +675,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      case XENMEM_remove_from_physmap:
>      {
> +        unsigned long argmfn, foreign_mfn = INVALID_MFN;
>          struct xen_remove_from_physmap xrfp;
>          struct page_info *page;
> -        struct domain *d;
> +        struct domain *d, *foreign_dom = NULL;
> +        p2m_type_t p2mt, tp;
> +        int valid_pvh_pg, is_curr_pvh = is_pvh_vcpu(current);
>  
>          if ( copy_from_guest(&xrfp, arg, 1) )
>              return -EFAULT;
> @@ -695,14 +698,45 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          domain_lock(d);
>  
> -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> -        if ( page )
> +        /* PVH note: if PVH, the gfn could be mapped to a mfn from foreign
> +         * domain by the user space tool during domain creation. We need to
> +         * check for that, free it up from the p2m, and release refcnt on it.
> +         * In such a case, page would be NULL. */
> +
> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> +        valid_pvh_pg = is_curr_pvh && 
> +                       (p2m_is_mmio(p2mt) || p2m_is_foreign(p2mt));

If you're testing for PVH, it should be 'd', not 'current', but I think
that test is unnecessary anyway.

And as I think I said last time, you should be testing fpr
p2m_mmio_direct() here.

> +
> +        if ( page || valid_pvh_pg)
>          {
> -            guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
> -            put_page(page);
> +            argmfn = page ? page_to_mfn(page) : INVALID_MFN;
> +
> +            if ( is_curr_pvh && p2m_is_foreign(p2mt) )

Again, testing 'current' is wrong here.

> +            {
> +                foreign_mfn = mfn_x(get_gfn_query_unlocked(d, xrfp.gpfn, &tp));
> +                foreign_dom = page_get_owner(mfn_to_page(foreign_mfn));

That's not safe for MMIO mappings! mfn_to_page() might not give you a
valid pointer unless the MFN is a RAM page.  I think it would be best to
handle MMIO and foreign separately here, just for clarity.

> +                ASSERT(p2m_is_mmio(tp) || p2m_is_foreign(tp));

If you want to guarantee that the mapping is still the same as the one
you saw at the top, you should use get_gfn() at the top, and not
put_gfn() until you've removed the mapping.

> +            }
> +
> +            guest_physmap_remove_page(d, xrfp.gpfn, argmfn, 0);
> +            if (page)
> +                put_page(page);
> +
> +            /* if pages were mapped from foreign domain via 
> +             * xenmem_add_foreign_to_pmap(), we must drop a refcnt here */
> +            if ( is_curr_pvh && p2m_is_foreign(p2mt) )
> +            {
> +                ASSERT( d != foreign_dom );

You'll need to make sure that the foreign-map op doesn't accept
DOMID_SELF, then.

Cheers,

Tim.

> +                put_page(mfn_to_page(foreign_mfn));
> +            }
>          }
>          else
> +        {
> +            if ( is_curr_pvh )
> +                gdprintk(XENLOG_WARNING, "%s: Domain:%u gmfn:%lx invalid\n",
> +                         __func__, current->domain->domain_id, xrfp.gpfn);
>              rc = -ENOENT;
> +        }
>  
>          domain_unlock(d);
>  
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 14/18 V2]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

On Thu, 21 Mar 2013 17:41:46 +0000
Tim Deegan <tim@xen.org> wrote:

> At 17:58 -0700 on 15 Mar (1363370311), Mukesh Rathor wrote:
> >  In this patch, a new function, xenmem_add_foreign_to_pmap(), is
> > added to map pages from foreign guest into current dom0 for domU
> > creation. Also, allow XENMEM_remove_from_physmap to remove
> > p2m_map_foreign pages. Note, in this path, we must release the
> > refcount that was taken during the map phase.
> > 
> > Changes in V2:
> >   - Move the XENMEM_remove_from_physmap changes here instead of
> > prev patch
> >   - Move grant changes from this to one of the next patches.
> >   - In xenmem_add_foreign_to_pmap(), do locked get_gfn
> >   - Fail the mappings for qemu mapping pages for memory not there.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  xen/arch/x86/mm.c   |   74
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > xen/common/memory.c |   44 +++++++++++++++++++++++++++--- 2 files
> > changed, 110 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 6603752..dbac811 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4266,6 +4266,66 @@ static int handle_iomem_range(unsigned long
> > s, unsigned long e, void *p) return 0;
> >  }
> >  
> > +/* 
> > + * Add frames from foreign domain to current domain's physmap.
> > Similar to 
> > + * XENMAPSPACE_gmfn but the frame is foreign being mapped into
> > current,
> > + * and is not removed from foreign domain. 
> > + * Usage: libxl on pvh dom0 creating a guest and doing
> > privcmd_ioctl_mmap.
> > + * Side Effect: the mfn for fgfn will be refcounted so it is not
> > lost
> > + *              while mapped here. The refcnt is released in
> > do_memory_op() 
> > + *              via XENMEM_remove_from_physmap.
> > + * Returns: 0 ==> success
> > + */
> > +static int xenmem_add_foreign_to_pmap(domid_t foreign_domid, 
> > +                                      unsigned long fgfn, unsigned
> > long gpfn) +{
> > +    p2m_type_t p2mt, p2mt_prev;
> > +    int rc = -EINVAL;
> > +    unsigned long prev_mfn, mfn = 0;
> > +    struct domain *fdom, *currd = current->domain;
> > +
> > +    if ( (fdom = get_pg_owner(foreign_domid)) == NULL )
> > +        return -EPERM;
> > +
> > +    mfn = mfn_x(get_gfn_query(fdom, fgfn, &p2mt));
> > +    if ( !mfn_valid(mfn) || !p2m_is_valid(p2mt) )
> > +        goto out_rc;
> > +
> > +    if ( !get_page_from_pagenr(mfn, fdom) )
> > +        goto out_rc;
> 
> I think you can use get_page_from_gfn() here instead of doing the
> translation and the get_page() by hand.  That way you don't need to
> worry about the put_gfn().  Which is just as well, as otherwise the
> second get_gfn() might deadlock if fdom == currd.

please see below.

> > +    /* Remove previously mapped page if it is present. */
> > +    prev_mfn = mfn_x(get_gfn(currd, gpfn, &p2mt_prev));
> > +    if ( mfn_valid(prev_mfn) )
> > +    {
> > +        if ( is_xen_heap_mfn(prev_mfn) )
> > +            /* Xen heap frames are simply unhooked from this phys
> > slot */
> > +            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
> > +        else
> > +            /* Normal domain memory is freed, to avoid leaking
> > memory. */
> > +            guest_remove_page(currd, gpfn);
> > +    }
> > +    put_gfn(currd, gpfn);
> 
> Again, without the p2m lock held, another CPU could populate gpfn
> between here and set_foreign_p2m_entry().

Ok, I suppose I should move the put_gfn. Here's what I got now:


/* 
 * Add frames from foreign domain to current domain's physmap. Similar to 
 * XENMAPSPACE_gmfn but the frame is foreign being mapped into current,
 * and is not removed from foreign domain. 
 * Usage: libxl on pvh dom0 creating a guest and doing privcmd_ioctl_mmap.
 * Side Effect: the mfn for fgfn will be refcounted so it is not lost
 *              while mapped here. The refcnt is released in do_memory_op() 
 *              via XENMEM_remove_from_physmap.
 * Returns: 0 ==> success
 */
static int xenmem_add_foreign_to_pmap(domid_t foreign_domid, 
                                      unsigned long fgfn, unsigned long gpfn)
{
    p2m_type_t p2mt, p2mt_prev;
    int rc = -EINVAL;
    unsigned long prev_mfn, mfn = 0;
    struct domain *fdom, *currd = current->domain;
    struct page_info *page = NULL;

    if ( currd->domain_id == foreign_domid || foreign_domid == DOMID_SELF )
        return -EINVAL;

    if ( !IS_PRIV(currd) || (fdom = get_pg_owner(foreign_domid)) == NULL )
        return -EPERM;

    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
    if ( !page || !p2m_is_valid(p2mt) )
    {
        if ( page )
            put_page(page);
        put_pg_owner(fdom);
        return rc;
    }

    /* Remove previously mapped page if it is present. */
    prev_mfn = mfn_x(get_gfn(currd, gpfn, &p2mt_prev));
    if ( mfn_valid(prev_mfn) )
    {
        if ( is_xen_heap_mfn(prev_mfn) )
            /* Xen heap frames are simply unhooked from this phys slot */
            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
        else
            /* Normal domain memory is freed, to avoid leaking memory. */
            guest_remove_page(currd, gpfn);
    }

    /*
     * Create the new mapping. Can't use guest_physmap_add_page() because it 
     * will update the m2p table which will result in  mfn -> gpfn of dom0 
     * and not fgfn of domU.
     */
    if ( set_foreign_p2m_entry(currd, gpfn, _mfn(mfn)) == 0 ) 
    {
        dprintk(XENLOG_WARNING,
                "guest_physmap_add_page failed. gpfn:%lx mfn:%lx fgfn:%lx\n", 
                gpfn, mfn, fgfn);
        put_page(page);
    } 
    else
        rc = 0;

    /*
     * We must do this put_gfn after set_foreign_p2m_entry so another cpu
     * doesn't populate the gpfn before us.
     */
    put_gfn(currd, gpfn);

    put_pg_owner(fdom);
    return rc;
}

Re: [PATCH 14/18 V2]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

On Thu, 21 Mar 2013 17:41:46 +0000
Tim Deegan <tim@xen.org> wrote:

> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index 68501d1..91a56b6 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > +        /* PVH note: if PVH, the gfn could be mapped to a mfn from
> > foreign
> > +         * domain by the user space tool during domain creation.
> > We need to
> > +         * check for that, free it up from the p2m, and release
> > refcnt on it.
> > +         * In such a case, page would be NULL. */
> > +
> > +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> > +        valid_pvh_pg = is_curr_pvh && 
> > +                       (p2m_is_mmio(p2mt) || p2m_is_foreign(p2mt));
> 

Actually, looking at this again, I can't find reason why I can't just change
get_page_from_gfn_p2m() to return page for foreign also:

    if ( likely(!p2m_locked_by_me(p2m)) )
    {   
        /* Fast path: look up and get out */
        p2m_read_lock(p2m);
        mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
        if ( (p2m_is_ram(*t) || p2m_is_grant(*t) || p2m_is_foreign(*t)) <====
             && mfn_valid(mfn)
             && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
        {
            page = mfn_to_page(mfn);
            if ( !get_page(page, d)
                 /* Page could be shared */
                 && !get_page(page, dom_cow) )
                page = NULL;
        }
......

    /* Slow path: take the write lock and do fixups */
    mfn = get_gfn_type_access(p2m, gfn, t, a, q, NULL);
    if ( p2m_is_ram(*t) && mfn_valid(mfn) ) <=======  Add p2m_is_foreign HERE
    {

Please lmk if you forsee any issues with above.
BTW, in the slow path, why doesn't it check for grant?

Also, I removed the mmio from map, so I can remove from here also.

thanks,
Mukesh

Re: [PATCH 14/18 V2]: PVH xen: add xenmem_add_foreign_to_pmap()

[Mukesh Rathor]

On Mon, 8 Apr 2013 17:17:58 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Thu, 21 Mar 2013 17:41:46 +0000
> Tim Deegan <tim@xen.org> wrote:
> 
> > > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > > index 68501d1..91a56b6 100644
> > 
> 
> Actually, looking at this again, I can't find reason why I can't just
> change get_page_from_gfn_p2m() to return page for foreign also:
> 
>     if ( likely(!p2m_locked_by_me(p2m)) )
>     {   
>         /* Fast path: look up and get out */
>         p2m_read_lock(p2m);
>         mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
>         if ( (p2m_is_ram(*t) || p2m_is_grant(*t) ||
> p2m_is_foreign(*t)) <==== && mfn_valid(mfn)
>              && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
>         {
>             page = mfn_to_page(mfn);
>             if ( !get_page(page, d)
>                  /* Page could be shared */
>                  && !get_page(page, dom_cow) )
>                 page = NULL;
>         }
> ......

Nah, get_page() barfs on me because the mfn owner is foreign domain.
Sigh... any suggestion? May be for now, I can just leave
get_page_from_gfn_p2m() as is, and put my change back in
XENMEM_remove_from_physmap where it checks for p2m_is_foreign. 

thanks,
Mukesh