WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] [PATCH 3 of 3] Make p2m critical sections hold a ref on the

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-devel] [PATCH 3 of 3] Make p2m critical sections hold a ref on the underlying mfn
From: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
Date: Tue, 08 Nov 2011 16:42:26 -0500
Cc: olaf@xxxxxxxxx, George.Dunlap@xxxxxxxxxxxxx, andres@xxxxxxxxxxxxxx, tim@xxxxxxx, keir.xen@xxxxxxxxx, adin@xxxxxxxxxxxxxx
Delivery-date: Tue, 08 Nov 2011 13:46:57 -0800
Dkim-signature: v=1; a=rsa-sha1; c=relaxed; d=lagarcavilla.org; h= content-type:mime-version:content-transfer-encoding:subject :message-id:in-reply-to:references:date:from:to:cc; s= lagarcavilla.org; bh=ASKtW1CnwC1VSDOKLpwAlt7PJHM=; b=BqiNYL4NVT2 mQVqjpDAkjrwR4cww6sST1HjXePQ7EdPgiVccNOLHmWQ9XAbYwMs5//6/LW89J9D SjT9swpZgUhfZGjjo1actF8dOZJbytY3Zqeq4vKKHBzb4e7KHXskxCmMoxwe0C05 gdJrWdmbKu5iVcd0uT4g29imox61rcog=
Domainkey-signature: a=rsa-sha1; c=nofws; d=lagarcavilla.org; h=content-type :mime-version:content-transfer-encoding:subject:message-id :in-reply-to:references:date:from:to:cc; q=dns; s= lagarcavilla.org; b=Zx/P7bxN8X1h4oWuOVC4o0K4AfHriCMGBvenzDptVAg0 LipJp6IowQaLJGkDy/g7306710pj7US0+PiT+9kbmf7Jj9XvHdsk3DfSj31L1hFz ob3wHXe1KbptgovgMJALgGUf49AS408hrYQ8EJfzfvOeY96sWrq/upJUv4Ytuhs=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <patchbomb.1320788543@xxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <patchbomb.1320788543@xxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mercurial-patchbomb/1.8.4
 xen/arch/x86/mm.c             |  18 +++++++++----
 xen/arch/x86/mm/mem_sharing.c |  13 +++------
 xen/arch/x86/mm/p2m.c         |  56 +++++++++++++++++++++++++++++++++++++++++-
 xen/common/grant_table.c      |   4 +-
 xen/common/memory.c           |  10 +++---
 xen/include/asm-x86/mm.h      |   3 +-
 xen/include/asm-x86/p2m.h     |  10 ++++++-
 xen/include/xen/paging.h      |   2 +-
 xen/include/xen/tmem_xen.h    |   2 +-
 9 files changed, 89 insertions(+), 29 deletions(-)


For translated domains, critical sections demarcated by a
get_gfn/put_gfn pair will hold an additional ref on the
underlying mfn.

This requires keeping tabs on special manipulations that
change said mfn:
 - physmap remove page
 - sharing
 - steal page

Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4128,7 +4128,7 @@ static int replace_grant_p2m_mapping(
                  type, mfn_x(old_mfn), frame);
         return GNTST_general_error;
     }
-    guest_physmap_remove_page(d, gfn, frame, 0);
+    guest_physmap_remove_page(d, gfn, frame, 0, HOLDING_GFN);
 
     put_gfn(d, gfn);
     return GNTST_okay;
@@ -4248,8 +4248,10 @@ int donate_page(
 }
 
 int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags)
+    struct domain *d, struct page_info *page, unsigned int memflags,
+    int holding_gfn)
 {
+    unsigned count;
     unsigned long x, y;
     bool_t drop_dom_ref = 0;
 
@@ -4261,11 +4263,14 @@ int steal_page(
     /*
      * We require there is just one reference (PGC_allocated). We temporarily
      * drop this reference now so that we can safely swizzle the owner.
+     * If, however, this is invoked by a caller holding the p2m entry, there
+     * will be another reference.
      */
+    count = (holding_gfn) ? 1 : 2;
     y = page->count_info;
     do {
         x = y;
-        if ( (x & (PGC_count_mask|PGC_allocated)) != (1 | PGC_allocated) )
+        if ( (x & (PGC_count_mask|PGC_allocated)) != (count | PGC_allocated) )
             goto fail;
         y = cmpxchg(&page->count_info, x, x & ~PGC_count_mask);
     } while ( y != x );
@@ -4276,7 +4281,7 @@ int steal_page(
     do {
         x = y;
         BUG_ON((x & (PGC_count_mask|PGC_allocated)) != PGC_allocated);
-    } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
+    } while ( (y = cmpxchg(&page->count_info, x, x | count)) != x );
 
     /* Unlink from original owner. */
     if ( !(memflags & MEMF_no_refcount) && !--d->tot_pages )
@@ -4779,7 +4784,8 @@ long arch_memory_op(int op, XEN_GUEST_HA
         {
             if ( is_xen_heap_mfn(prev_mfn) )
                 /* Xen heap frames are simply unhooked from this phys slot. */
-                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0);
+                guest_physmap_remove_page(d, xatp.gpfn, prev_mfn, 0, 
+                                            HOLDING_GFN);
             else
                 /* Normal domain memory is freed, to avoid leaking memory. */
                 guest_remove_page(d, xatp.gpfn);
@@ -4791,7 +4797,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
         gpfn = get_gpfn_from_mfn(mfn);
         ASSERT( gpfn != SHARED_M2P_ENTRY );
         if ( gpfn != INVALID_M2P_ENTRY )
-            guest_physmap_remove_page(d, gpfn, mfn, 0);
+            guest_physmap_remove_page(d, gpfn, mfn, 0, (gpfn == gfn));
 
         /* Map at new location. */
         rc = guest_physmap_add_page(d, xatp.gpfn, mfn, 0);
diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -561,7 +561,7 @@ int mem_sharing_share_pages(shr_handle_t
         list_del(&gfn->list);
         d = get_domain_by_id(gfn->domain);
         BUG_ON(!d);
-        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn) == 0);
+        BUG_ON(set_shared_p2m_entry(d, gfn->gfn, se->mfn, 0) == 0);
         put_domain(d);
         list_add(&gfn->list, &se->gfns);
         put_page_and_type(cpage);
@@ -670,14 +670,9 @@ gfn_found:
     unmap_domain_page(s);
     unmap_domain_page(t);
 
-    /* NOTE: set_shared_p2m_entry will switch the underlying mfn. If
-     * we do get_page withing get_gfn, the correct sequence here
-     * should be
-       get_page(page);
-       put_page(old_page);
-     * so that the ref to the old page is dropped, and a ref to
-     * the new page is obtained to later be dropped in put_gfn */
-    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page)) == 0);
+    /* NOTE: set_shared_p2m_entry will swap the underlying mfn and the refs. 
+     * It is safe to call put_gfn as usual after this. */
+    BUG_ON(set_shared_p2m_entry(d, gfn, page_to_mfn(page), HOLDING_GFN) == 0);
     put_page_and_type(old_page);
 
 private_page_found:    
diff -r 6203a0549d8a -r 4b684fa74636 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -182,6 +182,13 @@ mfn_t gfn_to_mfn_type_p2m(struct p2m_dom
     }
 #endif
 
+    /* Do a get page to get one additional ref on the mfn */
+    if ( mfn_valid(mfn) )
+    {
+        struct page_info *pg = mfn_to_page(mfn);
+        BUG_ON( !page_get_owner_and_reference(pg) );
+    }
+
     return mfn;
 }
 
@@ -195,6 +202,21 @@ void put_gfn(struct domain *d, unsigned 
 
     ASSERT(p2m_locked_by_me(p2m));
 
+    {
+        p2m_access_t a;
+        p2m_type_t t;
+        mfn_t mfn = p2m->get_entry(p2m, gfn, &t, &a, 
+                                    p2m_query, NULL);
+
+        if ( mfn_valid(mfn) )
+        {
+#ifdef __x86_64__
+            if (likely( !(p2m_is_broken(t)) ))
+#endif
+                put_page(mfn_to_page(mfn));
+        }
+    }    
+
     p2m_unlock(p2m);
 }
 
@@ -416,6 +438,28 @@ void p2m_final_teardown(struct domain *d
     p2m_teardown_nestedp2m(d);
 }
 
+/* If the caller has done get_gfn on this entry, then it has a ref on the
+ * old mfn. Swap the refs so put_gfn puts the appropriate ref */
+static inline void __p2m_swap_entry_refs(struct p2m_domain *p2m, 
+                                         unsigned long gfn, mfn_t mfn)
+{
+    p2m_type_t t;
+    p2m_access_t a;
+    mfn_t omfn;
+
+    if ( !paging_mode_translate(p2m->domain) )
+        return;
+
+    ASSERT(gfn_locked_by_me(p2m, gfn));
+
+    omfn = p2m->get_entry(p2m, gfn, &t, &a, p2m_query, NULL);
+
+    if ( mfn_valid(mfn) )
+        BUG_ON( !page_get_owner_and_reference(mfn_to_page(mfn)) );
+
+    if ( mfn_valid(omfn) )
+        put_page(mfn_to_page(omfn));
+}
 
 static void
 p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
@@ -451,11 +495,14 @@ p2m_remove_page(struct p2m_domain *p2m, 
 
 void
 guest_physmap_remove_page(struct domain *d, unsigned long gfn,
-                          unsigned long mfn, unsigned int page_order)
+                          unsigned long mfn, unsigned int page_order,
+                          int holding_gfn)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_lock(p2m);
     audit_p2m(p2m, 1);
+    if (holding_gfn)
+        __p2m_swap_entry_refs(p2m, gfn, _mfn(INVALID_MFN));
     p2m_remove_page(p2m, gfn, mfn, page_order);
     audit_p2m(p2m, 1);
     p2m_unlock(p2m);
@@ -713,7 +760,8 @@ out:
 }
 
 int
-set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
+set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn, 
+                        int holding_gfn)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     int rc = 0;
@@ -730,6 +778,10 @@ set_shared_p2m_entry(struct domain *d, u
      * sharable first */
     ASSERT(p2m_is_shared(ot));
     ASSERT(mfn_valid(omfn));
+
+    if ( holding_gfn )
+        __p2m_swap_entry_refs(p2m, gfn, mfn);
+
     /* XXX: M2P translations have to be handled properly for shared pages */
     set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/common/grant_table.c
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1497,7 +1497,7 @@ gnttab_transfer(
             goto copyback;
         }
 
-        if ( steal_page(d, page, 0) < 0 )
+        if ( steal_page(d, page, 0, HOLDING_GFN) < 0 )
         {
             put_gfn(d, gop.mfn);
             gop.status = GNTST_bad_page;
@@ -1505,7 +1505,7 @@ gnttab_transfer(
         }
 
 #ifndef __ia64__ /* IA64 implicitly replaces the old page in steal_page(). */
-        guest_physmap_remove_page(d, gop.mfn, mfn, 0);
+        guest_physmap_remove_page(d, gop.mfn, mfn, 0, HOLDING_GFN);
 #endif
         flush_tlb_mask(d->domain_dirty_cpumask);
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/common/memory.c
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -165,7 +165,7 @@ int guest_remove_page(struct domain *d, 
     mfn = mfn_x(get_gfn(d, gmfn, &p2mt)); 
     if ( unlikely(p2m_is_paging(p2mt)) )
     {
-        guest_physmap_remove_page(d, gmfn, mfn, 0);
+        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
         p2m_mem_paging_drop_page(d, gmfn);
         put_gfn(d, gmfn);
         return 1;
@@ -188,7 +188,7 @@ int guest_remove_page(struct domain *d, 
     if(p2m_is_shared(p2mt))
     {
         put_page_and_type(page);
-        guest_physmap_remove_page(d, gmfn, mfn, 0);
+        guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
         put_gfn(d, gmfn);
         return 1;
     }
@@ -207,7 +207,7 @@ int guest_remove_page(struct domain *d, 
     if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
         put_page(page);
 
-    guest_physmap_remove_page(d, gmfn, mfn, 0);
+    guest_physmap_remove_page(d, gmfn, mfn, 0, HOLDING_GFN);
     put_gfn(d, gmfn);
 
     put_page(page);
@@ -387,7 +387,7 @@ static long memory_exchange(XEN_GUEST_HA
 
                 page = mfn_to_page(mfn);
 
-                if ( unlikely(steal_page(d, page, MEMF_no_refcount)) )
+                if ( unlikely(steal_page(d, page, MEMF_no_refcount, 
HOLDING_GFN)) )
                 {
                     put_gfn(d, gmfn + k);
                     rc = -EINVAL;
@@ -427,7 +427,7 @@ static long memory_exchange(XEN_GUEST_HA
             gfn = mfn_to_gmfn(d, mfn);
             /* Pages were unshared above */
             BUG_ON(SHARED_M2P(gfn));
-            guest_physmap_remove_page(d, gfn, mfn, 0);
+            guest_physmap_remove_page(d, gfn, mfn, 0, 0);
             put_page(page);
         }
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/mm.h
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -578,7 +578,8 @@ int compat_arch_memory_op(int op, XEN_GU
 int compat_subarch_memory_op(int op, XEN_GUEST_HANDLE(void));
 
 int steal_page(
-    struct domain *d, struct page_info *page, unsigned int memflags);
+    struct domain *d, struct page_info *page, unsigned int memflags,
+    int holding_gfn);
 int donate_page(
     struct domain *d, struct page_info *page, unsigned int memflags);
 int page_make_sharable(struct domain *d, 
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -353,6 +353,10 @@ static inline unsigned long get_gfn_unty
 /* Will release the p2m_lock and put the page behind this mapping. */
 void put_gfn(struct domain *d, unsigned long gfn);
 
+/* Operations that change the underlying mfn in a p2m entry need to be 
+ * told whether the caller is holding the current gfn */
+#define HOLDING_GFN 1
+
 /* The intent is to have the caller not worry about put_gfn. They apply to 
  * very specific situations: debug printk's, dumps during a domain crash,
  * or to peek at a p2m entry/type. Caller is not holding the p2m entry 
@@ -410,7 +414,8 @@ static inline int guest_physmap_add_page
 /* Remove a page from a domain's p2m table */
 void guest_physmap_remove_page(struct domain *d,
                                unsigned long gfn,
-                               unsigned long mfn, unsigned int page_order);
+                               unsigned long mfn, unsigned int page_order,
+                               int holding_gfn);
 
 /* Set a p2m range as populate-on-demand */
 int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
@@ -471,7 +476,8 @@ p2m_pod_offline_or_broken_replace(struct
 
 #ifdef __x86_64__
 /* Modify p2m table for shared gfn */
-int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
+int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
+                            int holding_gfn);
 
 /* Check if a nominated gfn is valid to be paged out */
 int p2m_mem_paging_nominate(struct domain *d, unsigned long gfn);
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/paging.h
--- a/xen/include/xen/paging.h
+++ b/xen/include/xen/paging.h
@@ -21,7 +21,7 @@
 #define paging_mode_translate(d)              (0)
 #define paging_mode_external(d)               (0)
 #define guest_physmap_add_page(d, p, m, o)    (0)
-#define guest_physmap_remove_page(d, p, m, o) ((void)0)
+#define guest_physmap_remove_page(d, p, m, o, h)    ((void)0)
 
 #endif
 
diff -r 6203a0549d8a -r 4b684fa74636 xen/include/xen/tmem_xen.h
--- a/xen/include/xen/tmem_xen.h
+++ b/xen/include/xen/tmem_xen.h
@@ -198,7 +198,7 @@ static inline void _tmh_free_page_thispo
     struct domain *d = page_get_owner(pi);
 
     ASSERT(IS_VALID_PAGE(pi));
-    if ( (d == NULL) || steal_page(d,pi,0) == 0 )
+    if ( (d == NULL) || steal_page(d,pi,0,0) == 0 )
         tmh_page_list_put(pi);
     else
     {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel