Re: [V6 PATCH 6.2/7] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Mon, 16 Dec 2013 08:40:41 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 14.12.13 at 03:48, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> >> Also, Jan may have an opinion about whether a teardown operation
> >> that has to walk each p2m entry would have to be made
> >> preemptible.  I'm not sure where we draw the line on such things.
> > 
> > Since at present teardown cleanup of foreign is not really that
> > important as its only applicable to dom0, let me submit another
> > patch for it on Mon with few ideas. That would also keep this patch
> > size reasonable, and keep you from having to look at the same code
> > over and over. 
> > 
> > So, please take a look at the version below with above two fixes. If
> > you approve it, i can resubmit the entire series rebased to latest 
> > with your ack on Monday, and the series can go in while we resolve
> > the p2m teardown.
> 
> Going through the patch again, I'm not seeing any loop being
> added. Am I missing something here?

Yes. Since the destruction of p2m leaking foreign pages only applies
to control domain being destroyed, i don't think it is that critical
that part get into 4.4. So, I'm submitting a separate patch for it,
like said above.

> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -36,8 +36,6 @@
> >  
> >  #define
> > atomic_read_ept_entry(__pepte)                              \
> > ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } )
> > -#define atomic_write_ept_entry(__pepte,
> > __epte)                     \
> > -    write_atomic(&(__pepte)->epte, (__epte).epte)
> >  
> >  #define is_epte_present(ept_entry)      ((ept_entry)->epte & 0x7)
> >  #define is_epte_superpage(ept_entry)    ((ept_entry)->sp)
> > @@ -46,6 +44,25 @@ static inline bool_t is_epte_valid(ept_entry_t
> > *e) return (e->epte != 0 && e->sa_p2mt != p2m_invalid);
> >  }
> >  
> > +static inline void write_ept_entry(ept_entry_t *entryptr,
> > ept_entry_t *new)
> 
> So why do you drop the "atomic_" prefix here?

To distinguish it from the older atomic_* macro which did nothing but
atomically write the entry. But if it helps get your approval, I added
atomic prefix. 

> Also the second parameter could be "const"...

Ok.

Final version below:

thanks
Mukesh
---------------------

In this patch, a new function, p2m_add_foreign(), is added
to map pages from foreign guest into current dom0 for domU creation.
Such pages are typed p2m_map_foreign. Another function
p2m_remove_foreign() is added to remove such pages. Note, it is the
nature of such pages that a refcnt is held during their stay in the p2m.
The refcnt is added and released in the low level ept code for convenience.
The cleanup of foreign pages from p2m upon it's destruction, is submitted
subsequently under a separate patch.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/mm.c         |   23 +++++++---
 xen/arch/x86/mm/p2m-ept.c |   30 +++++++++++---
 xen/arch/x86/mm/p2m-pt.c  |    9 ++++-
 xen/arch/x86/mm/p2m.c     |   96 +++++++++++++++++++++++++++++++++++++++++++++
 xen/common/memory.c       |   12 +++++-
 xen/include/asm-arm/p2m.h |    8 +++-
 xen/include/asm-x86/p2m.h |    7 +++
 7 files changed, 169 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e3da479..6c2edc4 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2810,7 +2810,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;
@@ -4522,7 +4522,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,
+    struct domain *fdom)
 {
     struct page_info *page = NULL;
     unsigned long gfn = 0; /* gcc ... */
@@ -4581,6 +4582,13 @@ static int xenmem_add_to_physmap_once(
             page = mfn_to_page(mfn);
             break;
         }
+
+        case XENMAPSPACE_gmfn_foreign:
+        {
+            rc = p2m_add_foreign(d, xatp->idx, xatp->gpfn, fdom);
+            return rc;
+        }
+
         default:
             break;
     }
@@ -4646,7 +4654,7 @@ static int xenmem_add_to_physmap(struct domain *d,
         start_xatp = *xatp;
         while ( xatp->size > 0 )
         {
-            rc = xenmem_add_to_physmap_once(d, xatp);
+            rc = xenmem_add_to_physmap_once(d, xatp, NULL);
             if ( rc < 0 )
                 return rc;
 
@@ -4672,11 +4680,12 @@ static int xenmem_add_to_physmap(struct domain *d,
         return rc;
     }
 
-    return xenmem_add_to_physmap_once(d, xatp);
+    return xenmem_add_to_physmap_once(d, xatp, NULL);
 }
 
 static int xenmem_add_to_physmap_range(struct domain *d,
-                                       struct xen_add_to_physmap_range *xatpr)
+                                       struct xen_add_to_physmap_range *xatpr,
+                                       struct domain *fdom)
 {
     /* Process entries in reverse order to allow continuations */
     while ( xatpr->size > 0 )
@@ -4693,7 +4702,7 @@ static int xenmem_add_to_physmap_range(struct domain *d,
         xatp.space = xatpr->space;
         xatp.idx = idx;
         xatp.gpfn = gpfn;
-        rc = xenmem_add_to_physmap_once(d, &xatp);
+        rc = xenmem_add_to_physmap_once(d, &xatp, fdom);
 
         if ( copy_to_guest_offset(xatpr->errs, xatpr->size-1, &rc, 1) )
             return -EFAULT;
@@ -4780,7 +4789,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
         }
         rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d, fd);
         if ( rc == 0 )
-            rc = xenmem_add_to_physmap_range(d, &xatpr);
+            rc = xenmem_add_to_physmap_range(d, &xatpr, fd);
 
         rcu_unlock_domain(d);
         if ( fd )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 08d1d72..0ba2365 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -36,8 +36,6 @@
 
 #define atomic_read_ept_entry(__pepte)                              \
     ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } )
-#define atomic_write_ept_entry(__pepte, __epte)                     \
-    write_atomic(&(__pepte)->epte, (__epte).epte)
 
 #define is_epte_present(ept_entry)      ((ept_entry)->epte & 0x7)
 #define is_epte_superpage(ept_entry)    ((ept_entry)->sp)
@@ -46,6 +44,26 @@ static inline bool_t is_epte_valid(ept_entry_t *e)
     return (e->epte != 0 && e->sa_p2mt != p2m_invalid);
 }
 
+static inline void atomic_write_ept_entry(ept_entry_t *entryptr,
+                                          const ept_entry_t *new)
+{
+    if ( p2m_is_foreign(new->sa_p2mt) )
+    {
+        struct page_info *page;
+        struct domain *fdom;
+
+        ASSERT(mfn_valid(new->mfn));
+        page = mfn_to_page(new->mfn);
+        fdom = page_get_owner(page);
+        get_page(page, fdom);
+    }
+    if ( p2m_is_foreign(entryptr->sa_p2mt) )
+        put_page(mfn_to_page(entryptr->mfn));
+
+    write_atomic(&entryptr->epte, new->epte);
+}
+
+
 static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_access_t access)
 {
     /* First apply type permissions */
@@ -378,7 +396,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
             ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
         }
 
-        atomic_write_ept_entry(ept_entry, new_entry);
+        atomic_write_ept_entry(ept_entry, &new_entry);
     }
     else
     {
@@ -398,7 +416,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
 
         /* now install the newly split ept sub-tree */
         /* NB: please make sure domian is paused and no in-fly VT-d DMA. */
-        atomic_write_ept_entry(ept_entry, split_ept_entry);
+        atomic_write_ept_entry(ept_entry, &split_ept_entry);
 
         /* then move to the level we want to make real changes */
         for ( ; i > target; i-- )
@@ -428,7 +446,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
 
         ept_p2m_type_to_flags(&new_entry, p2mt, p2ma);
 
-        atomic_write_ept_entry(ept_entry, new_entry);
+        atomic_write_ept_entry(ept_entry, &new_entry);
     }
 
     /* Track the highest gfn for which we have ever had a valid mapping */
@@ -665,7 +683,7 @@ static void ept_change_entry_type_page(mfn_t ept_page_mfn, int ept_page_level,
 
             e.sa_p2mt = nt;
             ept_p2m_type_to_flags(&e, nt, e.access);
-            atomic_write_ept_entry(&epte[i], e);
+            atomic_write_ept_entry(&epte[i], &e);
         }
     }
 
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 09b60ce..766fd67 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -276,7 +276,7 @@ p2m_next_level(struct p2m_domain *p2m, mfn_t *table_mfn, void **table,
     return 1;
 }
 
-// Returns 0 on error (out of memory)
+// Returns 0 on error (out of memory, or unimplemented p2m type)
 static int
 p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 
               unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma)
@@ -312,6 +312,13 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         __trace_var(TRC_MEM_SET_P2M_ENTRY, 0, sizeof(t), &t);
     }
 
+    if ( p2m_is_foreign(p2mt) )
+    {
+        /* pvh fixme: foreign types are only supported on ept at present */
+        gdprintk(XENLOG_WARNING, "Unimplemented foreign p2m type.\n");
+        return 0;
+    }
+
     if ( !p2m_next_level(p2m, &table_mfn, &table, &gfn_remainder, gfn,
                          L4_PAGETABLE_SHIFT - PAGE_SHIFT,
                          L4_PAGETABLE_ENTRIES, PGT_l3_page_table) )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 0659ef1..441d151 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1741,6 +1741,102 @@ out_p2m_audit:
 #endif /* P2M_AUDIT */
 
 /*
+ * Add frames from foreign domain to target 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 in lower level routines
+ *              so it is not lost while mapped here. The refcnt is released
+ *              via the p2m_remove_foreign path.
+ * Returns: 0 ==> success
+ */
+int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
+                    unsigned long gpfn, struct domain *fdom)
+{
+    p2m_type_t p2mt, p2mt_prev;
+    mfn_t prev_mfn, mfn;
+    struct page_info *page;
+    int rc = 0;
+
+    if ( tdom == fdom || !tdom || !fdom || !is_pvh_domain(tdom) )
+        return -EINVAL;
+
+    /* following will take a refcnt on the mfn */
+    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
+    if ( !page || !p2m_is_valid(p2mt) )
+    {
+        if ( page )
+            put_page(page);
+        return -EINVAL;
+    }
+    mfn = page_to_mfn(page);
+
+    /* Remove previously mapped page if it is present. */
+    prev_mfn = get_gfn(tdom, gpfn, &p2mt_prev);
+    if ( mfn_valid(prev_mfn) )
+    {
+        if ( is_xen_heap_mfn(mfn_x(prev_mfn)) )
+            /* Xen heap frames are simply unhooked from this phys slot */
+            guest_physmap_remove_page(tdom, gpfn, mfn_x(prev_mfn), 0);
+        else
+            /* Normal domain memory is freed, to avoid leaking memory. */
+            guest_remove_page(tdom, 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(tdom, gpfn, mfn) == 0 )
+    {
+        gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
+                 "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
+                 gpfn, mfn_x(mfn), fgfn, tdom->domain_id, fdom->domain_id);
+        rc = -EINVAL;
+    }
+    put_page(page);
+
+    /*
+     * We must do this put_gfn after set_foreign_p2m_entry so another cpu
+     * doesn't populate the gpfn before us.
+     */
+    put_gfn(tdom, gpfn);
+
+    return rc;
+}
+
+int p2m_remove_foreign(struct domain *d, unsigned long gpfn)
+{
+    mfn_t mfn;
+    p2m_type_t p2mt;
+    struct domain *foreign_dom;
+
+    ASSERT(is_pvh_domain(d));
+
+    mfn = get_gfn_query(d, gpfn, &p2mt);
+    if ( unlikely(!p2m_is_foreign(p2mt)) )
+    {
+        put_gfn(d, gpfn);
+        gdprintk(XENLOG_WARNING, "Invalid type for gpfn:%lx domid:%d t:%d\n",
+                 gpfn, d->domain_id, p2mt);
+        return -EINVAL;
+    }
+    if ( unlikely(!mfn_valid(mfn)) )
+    {
+        put_gfn(d, gpfn);
+        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
+                 gpfn, d->domain_id);
+        return -EINVAL;
+    }
+
+    foreign_dom = page_get_owner(mfn_to_page(mfn));
+    ASSERT(d != foreign_dom);
+
+    guest_physmap_remove_page(d, gpfn, mfn_x(mfn), 0);
+    put_gfn(d, gpfn);
+    return 0;
+}
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/common/memory.c b/xen/common/memory.c
index eb7b72b..53f67c1 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -678,6 +678,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct xen_remove_from_physmap xrfp;
         struct page_info *page;
         struct domain *d;
+        p2m_type_t p2mt = INT_MAX;
 
         if ( copy_from_guest(&xrfp, arg, 1) )
             return -EFAULT;
@@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return rc;
         }
 
-        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
+        /*
+         * If autotranslate guest, (eg 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 and the following
+         * call would not have refcnt'd the page.
+         */
+        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
         if ( page )
         {
             guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
             put_page(page);
         }
+        else if ( p2m_is_foreign(p2mt) )
+            rc = p2m_remove_foreign(d, xrfp.gpfn);
         else
             rc = -ENOENT;
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index c660820..a664950 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -83,7 +83,7 @@ static inline struct page_info *get_page_from_gfn(
     struct page_info *page;
     unsigned long mfn = gmfn_to_mfn(d, gfn);
 
-    ASSERT(t == NULL);
+    ASSERT(!t || *t == INT_MAX);
 
     if (!mfn_valid(mfn))
         return NULL;
@@ -110,6 +110,12 @@ static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
+#define p2m_is_foreign(_t) (0 && (_t))
+static inline int p2m_remove_foreign(struct domain *d, unsigned long gpfn)
+{
+    return -ENOSYS;
+}
+
 #endif /* _XEN_P2M_H */
 
 /*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 6fc71a1..6371705 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -515,6 +515,13 @@ int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
 /* Set foreign mfn in the current guest's p2m table. */
 int set_foreign_p2m_entry(struct domain *domp, unsigned long gfn, mfn_t mfn);
 
+/* Add foreign mapping to the guest's p2m table. */
+int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
+                    unsigned long gpfn, struct domain *fdom);
+
+/* Remove foreign mapping from the guest's p2m table. */
+int p2m_remove_foreign(struct domain *d, unsigned long gpfn);
+
 /* 
  * Populate-on-demand
  */
-- 
1.7.2.3