[Xen-devel] [PATCH 1 of 5] Make p2m lookups fully synchronized wrt modifications

[Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>]

 xen/arch/x86/mm/hap/hap.c  |   6 +++++
 xen/arch/x86/mm/mm-locks.h |  40 ++++++++++++++++++++++++--------------
 xen/arch/x86/mm/p2m.c      |  21 ++++++++++++++++++-
 xen/include/asm-x86/p2m.h  |  47 ++++++++++++++++++++++++++++-----------------
 4 files changed, 79 insertions(+), 35 deletions(-)


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
do get_gfn, and later, another attempt at grabbing the p2m_lock

The lock is not taken for shadow lookups, as the testing needed to rule out the
possibility of locking inversions and deadlock has not yet been carried out for
shadow-paging. This is 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@xxxxxxxxxxxxxxxx>

diff -r 9a55109e4d7e -r 223512f9fb5b xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -786,7 +786,12 @@ 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 */
+    (void)get_gfn(d, cr3_gfn, &t);
     paging_lock(d);
 
     v->arch.paging.mode = hap_paging_get_mode(v);
@@ -803,6 +808,7 @@ static void hap_update_paging_modes(stru
     hap_update_cr3(v, 0);
 
     paging_unlock(d);
+    put_gfn(d, cr3_gfn);
 }
 
 #if CONFIG_PAGING_LEVELS == 3
diff -r 9a55109e4d7e -r 223512f9fb5b xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -144,6 +144,31 @@ static inline void mm_enforce_order_unlo
  *                                                                      *
  ************************************************************************/
 
+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_sha
  * - 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, 
diff -r 9a55109e4d7e -r 223512f9fb5b xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -144,9 +144,9 @@ void p2m_change_entry_type_global(struct
     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_dom
         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_dom
     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)
 {
diff -r 9a55109e4d7e -r 223512f9fb5b xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -309,9 +309,14 @@ struct p2m_domain *p2m_get_p2m(struct vc
 
 #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 vc
  * 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_unty
     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)

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