]> xenbits.xensource.com Git - people/vhanquez/xen.git/commitdiff
x86/mm: Make p2m lookups fully synchronized wrt modifications
authorAndres Lagar-Cavilla <andres@lagarcavilla.org>
Fri, 10 Feb 2012 16:07:07 +0000 (16:07 +0000)
committerAndres Lagar-Cavilla <andres@lagarcavilla.org>
Fri, 10 Feb 2012 16:07:07 +0000 (16:07 +0000)
We achieve this by locking/unlocking the global p2m_lock in get/put_gfn.

The lock is always taken recursively, as there are many paths that
call get_gfn, and later, make another attempt at grabbing the p2m_lock.

The lock is not taken for shadow lookups. We believe there are no problems
remaining for synchronized p2m+shadow paging, but we are not enabling this
combination due to lack of testing. Unlocked shadow p2m access are tolerable as
long as shadows do not gain support for paging or sharing.

HAP (EPT) lookups and all modifications do take the lock.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Acked-by: Tim Deegan <tim@xen.org>
Committed-by: Tim Deegan <tim@xen.org>
xen/arch/x86/mm/hap/hap.c
xen/arch/x86/mm/mm-locks.h
xen/arch/x86/mm/p2m.c
xen/include/asm-x86/mm.h
xen/include/asm-x86/p2m.h

index 5ab7c5a595abc2b73875ff855ab0075b065b9535..debaba4f9e8a6241e31c6d365a009fc533398075 100644 (file)
@@ -786,7 +786,14 @@ hap_paging_get_mode(struct vcpu *v)
 static void hap_update_paging_modes(struct vcpu *v)
 {
     struct domain *d = v->domain;
-
+    unsigned long cr3_gfn = v->arch.hvm_vcpu.guest_cr[3];
+    p2m_type_t t;
+
+    /* We hold onto the cr3 as it may be modified later, and
+     * we need to respect lock ordering. No need for 
+     * checks here as they are performed by vmx_load_pdptrs
+     * (the potential user of the cr3) */
+    (void)get_gfn(d, cr3_gfn, &t);
     paging_lock(d);
 
     v->arch.paging.mode = hap_paging_get_mode(v);
@@ -803,6 +810,7 @@ static void hap_update_paging_modes(struct vcpu *v)
     hap_update_cr3(v, 0);
 
     paging_unlock(d);
+    put_gfn(d, cr3_gfn);
 }
 
 #if CONFIG_PAGING_LEVELS == 3
index 9f8413d2b0b04a29c6ed62da35606bc61e34572e..998a4d825cffe4855767bb5b5bf9db8f7fd7d750 100644 (file)
@@ -144,6 +144,31 @@ static inline void mm_enforce_order_unlock(int unlock_level,
  *                                                                      *
  ************************************************************************/
 
+declare_mm_lock(nestedp2m)
+#define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
+#define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
+
+/* P2M lock (per-p2m-table)
+ * 
+ * This protects all queries and updates to the p2m table. 
+ *
+ * A note about ordering:
+ *   The order established here is enforced on all mutations of a p2m.
+ *   For lookups, the order established here is enforced only for hap
+ *   domains (1. shadow domains present a few nasty inversions; 
+ *            2. shadow domains do not support paging and sharing, 
+ *               the main sources of dynamic p2m mutations)
+ * 
+ * The lock is recursive as it is common for a code path to look up a gfn
+ * and later mutate it.
+ */
+
+declare_mm_lock(p2m)
+#define p2m_lock(p)           mm_lock_recursive(p2m, &(p)->lock)
+#define p2m_lock_recursive(p) mm_lock_recursive(p2m, &(p)->lock)
+#define p2m_unlock(p)         mm_unlock(&(p)->lock)
+#define p2m_locked_by_me(p)   mm_locked_by_me(&(p)->lock)
+
 /* Sharing per page lock
  *
  * This is an external lock, not represented by an mm_lock_t. The memory
@@ -167,21 +192,6 @@ declare_mm_order_constraint(per_page_sharing)
  * - setting the "cr3" field of any p2m table to a non-CR3_EADDR value. 
  *   (i.e. assigning a p2m table to be the shadow of that cr3 */
 
-declare_mm_lock(nestedp2m)
-#define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
-#define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
-
-/* P2M lock (per-p2m-table)
- * 
- * This protects all updates to the p2m table.  Updates are expected to
- * be safe against concurrent reads, which do *not* require the lock. */
-
-declare_mm_lock(p2m)
-#define p2m_lock(p)           mm_lock(p2m, &(p)->lock)
-#define p2m_lock_recursive(p) mm_lock_recursive(p2m, &(p)->lock)
-#define p2m_unlock(p)         mm_unlock(&(p)->lock)
-#define p2m_locked_by_me(p)   mm_locked_by_me(&(p)->lock)
-
 /* Page alloc lock (per-domain)
  *
  * This is an external lock, not represented by an mm_lock_t. However, 
index ab0fcbf89aafcfdceaf5e43acb1b819dfeafbec9..a7e738963805e44e9e789d23bb4750fa51dc49db 100644 (file)
@@ -144,9 +144,9 @@ void p2m_change_entry_type_global(struct domain *d,
     p2m_unlock(p2m);
 }
 
-mfn_t get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
+mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
                     p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
-                    unsigned int *page_order)
+                    unsigned int *page_order, bool_t locked)
 {
     mfn_t mfn;
 
@@ -158,6 +158,11 @@ mfn_t get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
         return _mfn(gfn);
     }
 
+    /* For now only perform locking on hap domains */
+    if ( locked && (hap_enabled(p2m->domain)) )
+        /* Grab the lock here, don't release until put_gfn */
+        p2m_lock(p2m);
+
     mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order);
 
 #ifdef __x86_64__
@@ -182,6 +187,18 @@ mfn_t get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
     return mfn;
 }
 
+void __put_gfn(struct p2m_domain *p2m, unsigned long gfn)
+{
+    if ( !p2m || !paging_mode_translate(p2m->domain) 
+              || !hap_enabled(p2m->domain) )
+        /* Nothing to do in this case */
+        return;
+
+    ASSERT(p2m_locked_by_me(p2m));
+
+    p2m_unlock(p2m);
+}
+
 int set_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 
                   unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma)
 {
index 5f004da5547a839a35c6f3863807af6360b1865b..ea52c7c92ba328bdcf10fd5ff5891a597870ff3b 100644 (file)
@@ -350,9 +350,9 @@ void clear_superpage_mark(struct page_info *page);
  * of (gfn,domain) tupples to a list of gfn's that the shared page is currently 
  * backing. Nesting may happen when sharing (and locking) two pages -- deadlock 
  * is avoided by locking pages in increasing order.
- * Memory sharing may take the p2m_lock within a page_lock/unlock
- * critical section. We enforce ordering between page_lock and p2m_lock using an
- * mm-locks.h construct. 
+ * All memory sharing code paths take the p2m lock of the affected gfn before
+ * taking the lock for the underlying page. We enforce ordering between page_lock 
+ * and p2m_lock using an mm-locks.h construct. 
  *
  * These two users (pte serialization and memory sharing) do not collide, since
  * sharing is only supported for hvm guests, which do not perform pv pte updates.
index 168f5d4cb121995b53c3000b463ca2f7fd8e91e3..4c150b4903e047e1064e56d799217307308585ed 100644 (file)
@@ -309,9 +309,14 @@ struct p2m_domain *p2m_get_p2m(struct vcpu *v);
 
 #define p2m_get_pagetable(p2m)  ((p2m)->phys_table)
 
-/**** p2m query accessors. After calling any of the variants below, you
- * need to call put_gfn(domain, gfn). If you don't, you'll lock the
- * hypervisor. ****/
+/**** p2m query accessors. They lock p2m_lock, and thus serialize
+ * lookups wrt modifications. They _do not_ release the lock on exit.
+ * After calling any of the variants below, caller needs to use
+ * put_gfn. ****/
+
+mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
+                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
+                    unsigned int *page_order, bool_t locked);
 
 /* Read a particular P2M table, mapping pages as we go.  Most callers
  * should _not_ call this directly; use the other get_gfn* functions
@@ -320,9 +325,8 @@ struct p2m_domain *p2m_get_p2m(struct vcpu *v);
  * If the lookup succeeds, the return value is != INVALID_MFN and 
  * *page_order is filled in with the order of the superpage (if any) that
  * the entry was found in.  */
-mfn_t get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
-                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
-                    unsigned int *page_order);
+#define get_gfn_type_access(p, g, t, a, q, o)   \
+        __get_gfn_type_access((p), (g), (t), (a), (q), (o), 1)
 
 /* General conversion function from gfn to mfn */
 static inline mfn_t get_gfn_type(struct domain *d,
@@ -353,21 +357,28 @@ static inline unsigned long get_gfn_untyped(struct domain *d, unsigned long gpfn
     return INVALID_MFN;
 }
 
-/* This is a noop for now. */
-static inline void __put_gfn(struct p2m_domain *p2m, unsigned long gfn)
-{
-}
+/* Will release the p2m_lock for this gfn entry. */
+void __put_gfn(struct p2m_domain *p2m, unsigned long gfn);
 
 #define put_gfn(d, gfn) __put_gfn(p2m_get_hostp2m((d)), (gfn))
 
-/* These are identical for now. The intent is to have the caller not worry 
- * about put_gfn. To only be used in printk's, crash situations, or to 
- * peek at a type. You're not holding the p2m entry exclsively after calling
- * this. */
-#define get_gfn_unlocked(d, g, t)         get_gfn_type((d), (g), (t), p2m_alloc)
-#define get_gfn_query_unlocked(d, g, t)   get_gfn_type((d), (g), (t), p2m_query)
-#define get_gfn_guest_unlocked(d, g, t)   get_gfn_type((d), (g), (t), p2m_guest)
-#define get_gfn_unshare_unlocked(d, g, t) get_gfn_type((d), (g), (t), p2m_unshare)
+/* The intent of the "unlocked" accessor 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 exclusively during or after calling this. 
+ *
+ * Note that an unlocked accessor only makes sense for a "query" lookup.
+ * Any other type of query can cause a change in the p2m and may need to
+ * perform locking.
+ */
+static inline mfn_t get_gfn_query_unlocked(struct domain *d, 
+                                           unsigned long gfn, 
+                                           p2m_type_t *t)
+{
+    p2m_access_t a;
+    return __get_gfn_type_access(p2m_get_hostp2m(d), gfn, t, &a, 
+                                    p2m_query, NULL, 0);
+}
 
 /* General conversion function from mfn to gfn */
 static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)