[Xen-devel] [PATCH 3 of 9] x86/mm: Refactor possibly deadlocking get_gfn calls

[Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>]

 xen/arch/x86/hvm/emulate.c    |  35 +++++++--------
 xen/arch/x86/mm/mem_sharing.c |  28 +++++++------
 xen/include/asm-x86/p2m.h     |  91 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+), 32 deletions(-)


When calling get_gfn multiple times on different gfn's in the same function, we
can easily deadlock if p2m lookups are locked. Thus, refactor these calls to
enforce simple deadlock-avoidance rules:
 - Lowest-numbered domain first
 - Lowest-numbered gfn first

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

diff -r 27031a8a4eff -r 3de7e43b130a xen/arch/x86/hvm/emulate.c
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -660,12 +660,13 @@ static int hvmemul_rep_movs(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
-    unsigned long saddr, daddr, bytes, sgfn, dgfn;
+    unsigned long saddr, daddr, bytes;
     paddr_t sgpa, dgpa;
     uint32_t pfec = PFEC_page_present;
-    p2m_type_t p2mt;
+    p2m_type_t sp2mt, dp2mt;
     int rc, df = !!(ctxt->regs->eflags & X86_EFLAGS_DF);
     char *buf;
+    struct two_gfns *tg;
 
     rc = hvmemul_virtual_to_linear(
         src_seg, src_offset, bytes_per_rep, reps, hvm_access_read,
@@ -693,26 +694,25 @@ static int hvmemul_rep_movs(
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    /* XXX In a fine-grained p2m locking scenario, we need to sort this
-     * get_gfn's, or else we might deadlock */
-    sgfn = sgpa >> PAGE_SHIFT;
-    (void)get_gfn(current->domain, sgfn, &p2mt);
-    if ( !p2m_is_ram(p2mt) && !p2m_is_grant(p2mt) )
+    tg = get_two_gfns(current->domain, sgpa >> PAGE_SHIFT, &sp2mt, NULL, NULL,
+                      current->domain, dgpa >> PAGE_SHIFT, &dp2mt, NULL, NULL,
+                      p2m_guest);
+    if ( !tg )
+        return X86EMUL_UNHANDLEABLE;
+
+    if ( !p2m_is_ram(sp2mt) && !p2m_is_grant(sp2mt) )
     {
         rc = hvmemul_do_mmio(
             sgpa, reps, bytes_per_rep, dgpa, IOREQ_READ, df, NULL);
-        put_gfn(current->domain, sgfn);
+        put_two_gfns(&tg);
         return rc;
     }
 
-    dgfn = dgpa >> PAGE_SHIFT;
-    (void)get_gfn(current->domain, dgfn, &p2mt);
-    if ( !p2m_is_ram(p2mt) && !p2m_is_grant(p2mt) )
+    if ( !p2m_is_ram(dp2mt) && !p2m_is_grant(dp2mt) )
     {
         rc = hvmemul_do_mmio(
             dgpa, reps, bytes_per_rep, sgpa, IOREQ_WRITE, df, NULL);
-        put_gfn(current->domain, sgfn);
-        put_gfn(current->domain, dgfn);
+        put_two_gfns(&tg);
         return rc;
     }
 
@@ -730,8 +730,7 @@ static int hvmemul_rep_movs(
      */
     if ( ((dgpa + bytes_per_rep) > sgpa) && (dgpa < (sgpa + bytes)) )
     {
-        put_gfn(current->domain, sgfn);
-        put_gfn(current->domain, dgfn);
+        put_two_gfns(&tg);
         return X86EMUL_UNHANDLEABLE;
     }
 
@@ -743,8 +742,7 @@ static int hvmemul_rep_movs(
     buf = xmalloc_bytes(bytes);
     if ( buf == NULL )
     {
-        put_gfn(current->domain, sgfn);
-        put_gfn(current->domain, dgfn);
+        put_two_gfns(&tg);
         return X86EMUL_UNHANDLEABLE;
     }
 
@@ -757,8 +755,7 @@ static int hvmemul_rep_movs(
         rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
 
     xfree(buf);
-    put_gfn(current->domain, sgfn);
-    put_gfn(current->domain, dgfn);
+    put_two_gfns(&tg);
 
     if ( rc == HVMCOPY_gfn_paged_out )
         return X86EMUL_RETRY;
diff -r 27031a8a4eff -r 3de7e43b130a xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -718,11 +718,13 @@ int mem_sharing_share_pages(struct domai
     int ret = -EINVAL;
     mfn_t smfn, cmfn;
     p2m_type_t smfn_type, cmfn_type;
+    struct two_gfns *tg;
 
-    /* XXX if sd == cd handle potential deadlock by ordering
-     * the get_ and put_gfn's */
-    smfn = get_gfn(sd, sgfn, &smfn_type);
-    cmfn = get_gfn(cd, cgfn, &cmfn_type);
+    tg = get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
+                      cd, cgfn, &cmfn_type, NULL, &cmfn,
+                      p2m_query);
+    if ( !tg )
+        return -ENOMEM;
 
     /* This tricky business is to avoid two callers deadlocking if 
      * grabbing pages in opposite client/source order */
@@ -819,8 +821,7 @@ int mem_sharing_share_pages(struct domai
     ret = 0;
     
 err_out:
-    put_gfn(cd, cgfn);
-    put_gfn(sd, sgfn);
+    put_two_gfns(&tg);
     return ret;
 }
 
@@ -834,11 +835,13 @@ int mem_sharing_add_to_physmap(struct do
     struct gfn_info *gfn_info;
     struct p2m_domain *p2m = p2m_get_hostp2m(cd);
     p2m_access_t a;
-    
-    /* XXX if sd == cd handle potential deadlock by ordering
-     * the get_ and put_gfn's */
-    smfn = get_gfn_query(sd, sgfn, &smfn_type);
-    cmfn = get_gfn_type_access(p2m, cgfn, &cmfn_type, &a, p2m_query, NULL);
+    struct two_gfns *tg;
+
+    tg = get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
+                      cd, cgfn, &cmfn_type, &a, &cmfn,
+                      p2m_query);
+    if ( !tg )
+        return -ENOMEM;
 
     /* Get the source shared page, check and lock */
     ret = XEN_DOMCTL_MEM_SHARING_S_HANDLE_INVALID;
@@ -886,8 +889,7 @@ int mem_sharing_add_to_physmap(struct do
 err_unlock:
     mem_sharing_page_unlock(spage);
 err_out:
-    put_gfn(cd, cgfn);
-    put_gfn(sd, sgfn);
+    put_two_gfns(&tg);
     return ret;
 }
 
diff -r 27031a8a4eff -r 3de7e43b130a xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -378,6 +378,97 @@ static inline unsigned long mfn_to_gfn(s
         return mfn_x(mfn);
 }
 
+/* Deadlock-avoidance scheme when calling get_gfn on different gfn's */
+struct two_gfns {
+    struct domain  *first_domain;
+    unsigned long   first_gfn;
+    struct domain  *second_domain;
+    unsigned long   second_gfn;
+};
+
+#define assign_pointers(dest, source)                                       \
+do {                                                                        \
+    dest ## _mfn = (source ## mfn) ? (source ## mfn) : &__ ## dest ## _mfn;  \
+    dest ## _a   = (source ## a)   ? (source ## a)   : &__ ## dest ## _a;    \
+    dest ## _t   = (source ## t)   ? (source ## t)   : &__ ## dest ## _t;    \
+} while(0)
+
+/* Returns mfn, type and access for potential caller consumption, but any
+ * of those can be NULL */
+static inline struct two_gfns *get_two_gfns(struct domain *rd, unsigned long 
rgfn,
+        p2m_type_t *rt, p2m_access_t *ra, mfn_t *rmfn, struct domain *ld, 
+        unsigned long lgfn, p2m_type_t *lt, p2m_access_t *la, mfn_t *lmfn,
+        p2m_query_t q)
+{
+    mfn_t           *first_mfn, *second_mfn, __first_mfn, __second_mfn;
+    p2m_access_t    *first_a, *second_a, __first_a, __second_a;
+    p2m_type_t      *first_t, *second_t, __first_t, __second_t;
+    
+    struct two_gfns *rval = xmalloc(struct two_gfns);
+    if ( !rval )
+        return NULL;
+
+    if ( rd == ld )
+    {
+        rval->first_domain = rval->second_domain = rd;
+
+        /* Sort by gfn */
+        if (rgfn <= lgfn)
+        {
+            rval->first_gfn     = rgfn;
+            rval->second_gfn    = lgfn;
+            assign_pointers(first, r);
+            assign_pointers(second, l);
+        } else {
+            rval->first_gfn     = lgfn;
+            rval->second_gfn    = rgfn;
+            assign_pointers(first, l);
+            assign_pointers(second, r);
+        }        
+    } else {
+        /* Sort by domain */
+        if ( rd->domain_id <= ld->domain_id )
+        {
+            rval->first_domain  = rd;
+            rval->first_gfn     = rgfn;
+            rval->second_domain = ld;
+            rval->second_gfn    = lgfn;
+            assign_pointers(first, r);
+            assign_pointers(second, l);
+        } else {
+            rval->first_domain  = ld;
+            rval->first_gfn     = lgfn;
+            rval->second_domain = rd;
+            rval->second_gfn    = rgfn;
+            assign_pointers(first, l);
+            assign_pointers(second, r);
+        }
+    }
+
+    /* Now do the gets */
+    *first_mfn  = get_gfn_type_access(p2m_get_hostp2m(rval->first_domain), 
+                                      rval->first_gfn, first_t, first_a, q, 
NULL);
+    *second_mfn = get_gfn_type_access(p2m_get_hostp2m(rval->second_domain), 
+                                      rval->second_gfn, second_t, second_a, q, 
NULL);
+
+    return rval;
+}
+
+static inline void put_two_gfns(struct two_gfns **arg_ptr)
+{
+    struct two_gfns *arg;
+
+    if ( !arg_ptr || !(*arg_ptr) )
+        return;
+
+    arg = *arg_ptr;
+    put_gfn(arg->second_domain, arg->second_gfn);
+    put_gfn(arg->first_domain, arg->first_gfn);
+
+    xfree(arg);
+    *arg_ptr = NULL;
+}
+
 /* Init the datastructures for later use by the p2m code */
 int p2m_init(struct domain *d);
 

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