|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH 1 of 7] Make p2m lookups fully synchronized wrt modifications
xen/arch/x86/mm/hap/hap.c | 8 +++++++
xen/arch/x86/mm/mm-locks.h | 40 ++++++++++++++++++++++++--------------
xen/arch/x86/mm/p2m.c | 21 ++++++++++++++++++-
xen/include/asm-x86/mm.h | 6 ++--
xen/include/asm-x86/p2m.h | 47 ++++++++++++++++++++++++++++-----------------
5 files changed, 84 insertions(+), 38 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
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@xxxxxxxxxxxxxxxx>
diff -r 1756949233a9 -r e29c2634afb1 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,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(stru
hap_update_cr3(v, 0);
paging_unlock(d);
+ put_gfn(d, cr3_gfn);
}
#if CONFIG_PAGING_LEVELS == 3
diff -r 1756949233a9 -r e29c2634afb1 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 1756949233a9 -r e29c2634afb1 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 1756949233a9 -r e29c2634afb1 xen/include/asm-x86/mm.h
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -350,9 +350,9 @@ void clear_superpage_mark(struct page_in
* 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.
diff -r 1756949233a9 -r e29c2634afb1 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |