x86/mm/shadow: fixes and synchronized p2m lookups

PATCH In review Andres Lagar-Cavilla 4 participants 18 messages Apr 13, 2012 – Apr 19, 2012

Share & tools

Andres Lagar-Cavilla

author

2012-04-13 16:22 UTC #0 lore raw

PATCH 1/1 x86/mm/shadow: fixes and synchronized p2m lookups

Allow shadow page tables to take advantage of synchronized p2m lookups. This required a number of deadlock fixes building up to enabling of this feature.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

 xen/arch/x86/mm/mm-locks.h      |   3 +++
 xen/arch/x86/mm/shadow/multi.c  |   3 ++-
 xen/arch/x86/mm/shadow/multi.c  |  25 +++++++++++++++++++++++++
 xen/arch/x86/mm/shadow/common.c |   4 ++++
 xen/arch/x86/mm/shadow/multi.c  |  13 +++++++++++--
 xen/arch/x86/mm/p2m.c           |   5 ++---
 6 files changed, 47 insertions(+), 6 deletions(-)

Andres Lagar-Cavilla

author

2012-04-13 16:22 UTC #1 lore raw

PATCH 1/1 x86/mm: Print stack trace on a an mm-locks deadlock panic

 xen/arch/x86/mm/mm-locks.h |  3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Signed-off-by Andres Lagar-Cavilla <[email protected]>

xen/arch/x86/mm/mm-locks.h +3 -0

diff -r 7b606c043208 -r aa4ef559da60 xen/arch/x86/mm/mm-locks.h
@@ -50,8 +50,11 @@ static inline int mm_locked_by_me(mm_loc
 #define __check_lock_level(l)                           \
 do {                                                    \
     if ( unlikely(__get_lock_level()) > (l) )           \
+    {                                                   \
+        WARN();                                         \
         panic("mm locking order violation: %i > %i\n",  \
               __get_lock_level(), (l));                 \
+    }                                                   \
 } while(0)
 
 #define __set_lock_level(l)         \

Tim Deegan

2012-04-18 15:55 UTC #2 lore raw

Re: [PATCH 1 of 6] x86/mm: Print stack trace on a an mm-locks deadlock panic

Quoted 5 lines · Andres Lagar-Cavilla

 xen/arch/x86/mm/mm-locks.h |  3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Signed-off-by Andres Lagar-Cavilla <[email protected]>

Good idea, but 'WARN(); panic();' is a bit ugly. How about the attached two patches instead? The second one fixes what I think must be a typo, unless I'm forgetting something subtle.

Tim.

# HG changeset patch
# User Tim Deegan <[email protected]>
# Date 1334763793 -3600
# Node ID 33978b6482d058887c00454aff078476cef4155d # Parent 7c777cb8f705411b77c551f34ba88bdc09e38ab8 x86/mm: BUG() rather than panic() on mm lock order violations

That gives us a backtrace showing where the bad lock happens.

Reported-by Andres Lagar-Cavilla <[email protected]>

Signed-off-by Tim Deegan <[email protected]>

xen/arch/x86/mm/mm-locks.h +5 -2

diff -r 7c777cb8f705 xen/arch/x86/mm/mm-locks.h
@@ -50,8 +50,11 @@ static inline int mm_locked_by_me(mm_loc
 #define __check_lock_level(l)                           \
 do {                                                    \
     if ( unlikely(__get_lock_level()) > (l) )           \
-        panic("mm locking order violation: %i > %i\n",  \
-              __get_lock_level(), (l));                 \
+    {                                                   \
+        printk("mm locking order violation: %i > %i\n", \
+               __get_lock_level(), (l));                \
+        BUG();                                          \
+    }                                                   \
 } while(0)
 
 #define __set_lock_level(l)         \

# HG changeset patch
# Parent 87eb032bda77ade8cd75b530a97ea50170becb0b
x86/mm: fix parens in mm lock level check

Signed-off-by Tim Deegan <[email protected]>

xen/arch/x86/mm/mm-locks.h +1 -1

diff -r 87eb032bda77 xen/arch/x86/mm/mm-locks.h
@@ -49,7 +49,7 @@ static inline int mm_locked_by_me(mm_loc
  * where the offending locks are declared. */
 #define __check_lock_level(l)                           \
 do {                                                    \
-    if ( unlikely(__get_lock_level()) > (l) )           \
+    if ( unlikely(__get_lock_level() > (l)) )           \
     {                                                   \
         printk("mm locking order violation: %i > %i\n", \
                __get_lock_level(), (l));                \

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andres Lagar-Cavilla

author

2012-04-18 15:58 UTC #3 lore raw

Re: [PATCH 1 of 6] x86/mm: Print stack trace on a an mm-locks deadlock panic

Quoted 10 lines

Quoted 5 lines · Andres Lagar-Cavilla
 xen/arch/x86/mm/mm-locks.h |  3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Signed-off-by Andres Lagar-Cavilla <[email protected]>
Good idea, but 'WARN(); panic();' is a bit ugly. How about the attached two patches instead? The second one fixes what I think must be a typo, unless I'm forgetting something subtle.

Better. Thanks. Ack'ed both
Andres

Quoted 3 lines

Tim.

Andres Lagar-Cavilla

author

2012-04-13 16:22 UTC #4 lore raw

PATCH 1/1 x86/mm/shadow: enclose an OOS function in the proper conditional ifdef

In reply to Andres Lagar-Cavilla

 xen/arch/x86/mm/shadow/multi.c |  3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Otherwise compilation fails if the feature is disabled.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

xen/arch/x86/mm/shadow/multi.c +2 -1

diff -r aa4ef559da60 -r f8fd4a4239a8 xen/arch/x86/mm/shadow/multi.c
@@ -248,6 +248,7 @@ shadow_check_gwalk(struct vcpu *v, unsig
     return !mismatch;
 }
 
+#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
 static int
 shadow_check_gl1e(struct vcpu *v, walk_t *gw)
 {

@@ -263,7 +264,7 @@ shadow_check_gl1e(struct vcpu *v, walk_t
 
     return gw->l1e.l1 != nl1e.l1;
 }
-
+#endif
 
 /* Remove write access permissions from a gwalk_t in a batch, and
  * return OR-ed result for TLB flush hint and need to rewalk the guest

Gianluca Guida

Acked-by on v1 1/1

2012-04-14 14:08 UTC #5 lore raw

Re: [PATCH 2 of 6] x86/mm/shadow: enclose an OOS function in the proper conditional ifdef

Quoted 5 lines · Andres Lagar-Cavilla

xen/arch/x86/mm/shadow/multi.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Otherwise compilation fails if the feature is disabled.

Oh yes, my fault. This has been around for quite a few years in my disks with the promise to "Send It Next Time" (TM). Thanks for fixing this.

Acked-by Gianluca Guida <[email protected]>

Tim Deegan

Applied

2012-04-18 15:52 UTC #6 lore raw

Re: [PATCH 2 of 6] x86/mm/shadow: enclose an OOS function in the proper conditional ifdef

Quoted 6 lines · Gianluca Guida

Quoted 4 lines · Andres Lagar-Cavilla
xen/arch/x86/mm/shadow/multi.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Quoted 7 lines

Quoted 1 line
Otherwise compilation fails if the feature is disabled.
Oh yes, my fault. This has been around for quite a few years in my disks with the promise to "Send It Next Time" (TM). Thanks for fixing this.
Acked-by Gianluca Guida <[email protected]>

Applied, thanks.

Tim.

Andres Lagar-Cavilla

author

2012-04-13 16:22 UTC #7 lore raw

PATCH 1/1 x86/mm/shadow: fix potential p2m/paging deadlock when emulating page table writes

In reply to Andres Lagar-Cavilla

 xen/arch/x86/mm/shadow/multi.c |  25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

Signed-off-by Andres Lagar-Cavilla <[email protected]>

xen/arch/x86/mm/shadow/multi.c +25 -0

diff -r f8fd4a4239a8 -r 964c6cbad926 xen/arch/x86/mm/shadow/multi.c
@@ -5033,9 +5033,21 @@ sh_x86_emulate_write(struct vcpu *v, uns
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
+    /* To prevent a shadow mode deadlock, we need to hold the p2m from here
+     * onwards. emulate_unmap_dest may need to verify the pte that is being
+     * written to here, and thus get_gfn on the gfn contained in the payload
+     * that is being written here. p2m_lock is recursive, so all is well on
+     * that regard. Further, holding the p2m lock ensures the page table gfn
+     * being written to won't go away (although that could be achieved with
+     * a page reference, as done elsewhere).
+     */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     paging_lock(v->domain);
     memcpy(addr, src, bytes);

@@ -5059,6 +5071,7 @@ sh_x86_emulate_write(struct vcpu *v, uns
     emulate_unmap_dest(v, addr, bytes, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return X86EMUL_OKAY;
 }

@@ -5075,9 +5088,14 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
+    /* see comment in sh_x86_emulate_write. */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     paging_lock(v->domain);
     switch ( bytes )

@@ -5101,6 +5119,7 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     emulate_unmap_dest(v, addr, bytes, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return rv;
 }

@@ -5119,9 +5138,14 @@ sh_x86_emulate_cmpxchg8b(struct vcpu *v,
     if ( (vaddr & 7) && !is_hvm_vcpu(v) )
         return X86EMUL_UNHANDLEABLE;
 
+    /* see comment in sh_x86_emulate_write. */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, 8, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     old = (((u64) old_hi) << 32) | (u64) old_lo;
     new = (((u64) new_hi) << 32) | (u64) new_lo;

@@ -5135,6 +5159,7 @@ sh_x86_emulate_cmpxchg8b(struct vcpu *v,
     emulate_unmap_dest(v, addr, 8, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return rv;
 }
 #endif

Tim Deegan

NAK

2012-04-18 16:13 UTC #8 lore raw

Re: [PATCH 3 of 6] x86/mm/shadow: fix potential p2m/paging deadlock when emulating page table writes

Nack, at least for now; we spent a fair amount of effort taking all this emulation code out from under the shadow lock; serializing it under the p2m lock would be unfortunate. The other patches are less worrying, since they wrap a shadow_lock() in a p2m_lock() but I hope they can all be avoided.

The existing interlock between the shadow code and the p2m code prevents any p2m modifications from happening when the shadow lock is held, so I hope I can solve this by switching to unlocked lookups instead. I'm building a test kernel now to tell me exactly which lookps are to blame.

If I don't get this done today I'll look into it tomorrow.

Cheers,

Tim.

Quoted 90 lines · Andres Lagar-Cavilla

 xen/arch/x86/mm/shadow/multi.c |  25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

Signed-off-by Andres Lagar-Cavilla <[email protected]>

diff -r f8fd4a4239a8 -r 964c6cbad926 xen/arch/x86/mm/shadow/multi.c

xen/arch/x86/mm/shadow/multi.c +25 -0

@@ -5033,9 +5033,21 @@ sh_x86_emulate_write(struct vcpu *v, uns
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
+    /* To prevent a shadow mode deadlock, we need to hold the p2m from here
+     * onwards. emulate_unmap_dest may need to verify the pte that is being
+     * written to here, and thus get_gfn on the gfn contained in the payload
+     * that is being written here. p2m_lock is recursive, so all is well on
+     * that regard. Further, holding the p2m lock ensures the page table gfn
+     * being written to won't go away (although that could be achieved with
+     * a page reference, as done elsewhere).
+     */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     paging_lock(v->domain);
     memcpy(addr, src, bytes);

@@ -5059,6 +5071,7 @@ sh_x86_emulate_write(struct vcpu *v, uns
     emulate_unmap_dest(v, addr, bytes, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return X86EMUL_OKAY;
 }

@@ -5075,9 +5088,14 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
+    /* see comment in sh_x86_emulate_write. */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     paging_lock(v->domain);
     switch ( bytes )

@@ -5101,6 +5119,7 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     emulate_unmap_dest(v, addr, bytes, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return rv;
 }

@@ -5119,9 +5138,14 @@ sh_x86_emulate_cmpxchg8b(struct vcpu *v,
     if ( (vaddr & 7) && !is_hvm_vcpu(v) )
         return X86EMUL_UNHANDLEABLE;
 
+    /* see comment in sh_x86_emulate_write. */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, 8, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     old = (((u64) old_hi) << 32) | (u64) old_lo;
     new = (((u64) new_hi) << 32) | (u64) new_lo;

@@ -5135,6 +5159,7 @@ sh_x86_emulate_cmpxchg8b(struct vcpu *v,
     emulate_unmap_dest(v, addr, 8, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return rv;
 }
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andres Lagar-Cavilla

author

2012-04-18 16:25 UTC #9 lore raw

Re: [PATCH 3 of 6] x86/mm/shadow: fix potential p2m/paging deadlock when emulating page table writes

Quoted 11 lines

Nack, at least for now; we spent a fair amount of effort taking all this emulation code out from under the shadow lock; serializing it under the p2m lock would be unfortunate. The other patches are less worrying, since they wrap a shadow_lock() in a p2m_lock() but I hope they can all be avoided.
The existing interlock between the shadow code and the p2m code prevents any p2m modifications from happening when the shadow lock is held, so I hope I can solve this by switching to unlocked lookups instead. I'm building a test kernel now to tell me exactly which lookps are to blame.

FWIW, get_gfn_query for the target gfn of the PTE entry that is being checked in validate_gl?e entry, is the call that causes the panic this patch tries to fix.

As for the other two patches, sh_resync_l1 is the trigger (again, get_gfn_query on the gfn that is contained by the PTE being verified)

Andres

Quoted 105 lines

If I don't get this done today I'll look into it tomorrow.

Cheers,

Tim.

Quoted 96 lines · Andres Lagar-Cavilla

 xen/arch/x86/mm/shadow/multi.c |  25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

Signed-off-by Andres Lagar-Cavilla <[email protected]>

diff -r f8fd4a4239a8 -r 964c6cbad926 xen/arch/x86/mm/shadow/multi.c

xen/arch/x86/mm/shadow/multi.c +1 -0

@@ -5033,9 +5033,21 @@ sh_x86_emulate_write(struct vcpu *v, uns
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
+    /* To prevent a shadow mode deadlock, we need to hold the p2m from

here
+ * onwards. emulate_unmap_dest may need to verify the pte that is
being
+ * written to here, and thus get_gfn on the gfn contained in the
payload
+ * that is being written here. p2m_lock is recursive, so all is
well on
+ * that regard. Further, holding the p2m lock ensures the page
table gfn
+ * being written to won't go away (although that could be achieved
with

xen/arch/x86/mm/shadow/multi.c +19 -0

+     * a page reference, as done elsewhere).
+     */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     paging_lock(v->domain);
     memcpy(addr, src, bytes);

@@ -5059,6 +5071,7 @@ sh_x86_emulate_write(struct vcpu *v, uns
     emulate_unmap_dest(v, addr, bytes, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return X86EMUL_OKAY;
 }

@@ -5075,9 +5088,14 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     if ( (vaddr & (bytes - 1)) && !is_hvm_vcpu(v)  )
         return X86EMUL_UNHANDLEABLE;
 
+    /* see comment in sh_x86_emulate_write. */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, bytes, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     paging_lock(v->domain);
     switch ( bytes )

@@ -5101,6 +5119,7 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
     emulate_unmap_dest(v, addr, bytes, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return rv;
 }

@@ -5119,9 +5138,14 @@ sh_x86_emulate_cmpxchg8b(struct vcpu *v,
     if ( (vaddr & 7) && !is_hvm_vcpu(v) )
         return X86EMUL_UNHANDLEABLE;
 
+    /* see comment in sh_x86_emulate_write. */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     addr = emulate_map_dest(v, vaddr, 8, sh_ctxt);
     if ( emulate_map_dest_failed(addr) )
+    {
+        p2m_unlock(p2m_get_hostp2m(v->domain));
         return (long)addr;
+    }
 
     old = (((u64) old_hi) << 32) | (u64) old_lo;
     new = (((u64) new_hi) << 32) | (u64) new_lo;

@@ -5135,6 +5159,7 @@ sh_x86_emulate_cmpxchg8b(struct vcpu *v,
     emulate_unmap_dest(v, addr, 8, sh_ctxt);
     shadow_audit_tables(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
     return rv;
 }
 #endif

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Tim Deegan

2012-04-19 14:51 UTC #10 lore raw

Re: [PATCH 3 of 6] x86/mm/shadow: fix potential p2m/paging deadlock when emulating page table writes

Quoted 12 lines · Andres Lagar-Cavilla

Quoted 11 lines
Nack, at least for now; we spent a fair amount of effort taking all this emulation code out from under the shadow lock; serializing it under the p2m lock would be unfortunate. The other patches are less worrying, since they wrap a shadow_lock() in a p2m_lock() but I hope they can all be avoided.
The existing interlock between the shadow code and the p2m code prevents any p2m modifications from happening when the shadow lock is held, so I hope I can solve this by switching to unlocked lookups instead. I'm building a test kernel now to tell me exactly which lookps are to blame.

Quoted 6 lines

FWIW, get_gfn_query for the target gfn of the PTE entry that is being checked in validate_gl?e entry, is the call that causes the panic this patch tries to fix.
As for the other two patches, sh_resync_l1 is the trigger (again, get_gfn_query on the gfn that is contained by the PTE being verified)

Thanks. I've taken a sweep through mm/shadow, making the lookups into unlocked ones wherever the paging_lock is already held. With the attached patch and your final patch to enable the locking, I can start HVM guests without crashing Xen. I haven't given it any more serious testing yet.

Cheers,

Tim.

x86/mm/shadow: don't use locking p2m lookups with the paging lock held.

The existing interlock between shadow and p2m (where p2m table updates are done under the paging lock) keeps us safe from the p2m changing under our feet, and using the locking lookups is a violation of the locking discipline (which says always to take the p2m lock first).

Signed-off-by Tim Deegan <[email protected]>

xen/arch/x86/mm/shadow/common.c +1 -3

diff -r 6f89f15fd3c8 xen/arch/x86/mm/shadow/common.c
@@ -3676,7 +3676,7 @@ int shadow_track_dirty_vram(struct domai
 
         /* Iterate over VRAM to track dirty bits. */
         for ( i = 0; i < nr; i++ ) {
-            mfn_t mfn = get_gfn_query(d, begin_pfn + i, &t);
+            mfn_t mfn = get_gfn_query_unlocked(d, begin_pfn + i, &t);
             struct page_info *page;
             int dirty = 0;
             paddr_t sl1ma = dirty_vram->sl1ma[i];

@@ -3741,8 +3741,6 @@ int shadow_track_dirty_vram(struct domai
                 }
             }
 
-            put_gfn(d, begin_pfn + i);
-
             if ( dirty )
             {
                 dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);

xen/arch/x86/mm/shadow/multi.c +8 ~~-16~~

diff -r 6f89f15fd3c8 xen/arch/x86/mm/shadow/multi.c
@@ -2266,7 +2266,7 @@ static int validate_gl4e(struct vcpu *v,
     if ( guest_l4e_get_flags(new_gl4e) & _PAGE_PRESENT )
     {
         gfn_t gl3gfn = guest_l4e_get_gfn(new_gl4e);
-        mfn_t gl3mfn = get_gfn_query(d, gl3gfn, &p2mt);
+        mfn_t gl3mfn = get_gfn_query_unlocked(d, gfn_x(gl3gfn), &p2mt);
         if ( p2m_is_ram(p2mt) )
             sl3mfn = get_shadow_status(v, gl3mfn, SH_type_l3_shadow);
         else if ( p2mt != p2m_populate_on_demand )

@@ -2276,7 +2276,6 @@ static int validate_gl4e(struct vcpu *v,
         if ( mfn_valid(sl3mfn) )
             shadow_resync_all(v);
 #endif
-        put_gfn(d, gfn_x(gl3gfn));
     }
     l4e_propagate_from_guest(v, new_gl4e, sl3mfn, &new_sl4e, ft_prefetch);

@@ -2324,7 +2323,7 @@ static int validate_gl3e(struct vcpu *v,
     if ( guest_l3e_get_flags(new_gl3e) & _PAGE_PRESENT )
     {
         gfn_t gl2gfn = guest_l3e_get_gfn(new_gl3e);
-        mfn_t gl2mfn = get_gfn_query(v->domain, gl2gfn, &p2mt);
+        mfn_t gl2mfn = get_gfn_query_unlocked(v->domain, gfn_x(gl2gfn), &p2mt);
         if ( p2m_is_ram(p2mt) )
             sl2mfn = get_shadow_status(v, gl2mfn, SH_type_l2_shadow);
         else if ( p2mt != p2m_populate_on_demand )

@@ -2334,7 +2333,6 @@ static int validate_gl3e(struct vcpu *v,
         if ( mfn_valid(sl2mfn) )
             shadow_resync_all(v);
 #endif
-        put_gfn(v->domain, gfn_x(gl2gfn));
     }
     l3e_propagate_from_guest(v, new_gl3e, sl2mfn, &new_sl3e, ft_prefetch);
     result |= shadow_set_l3e(v, sl3p, new_sl3e, sl3mfn);

@@ -2374,12 +2372,12 @@ static int validate_gl2e(struct vcpu *v,
         }
         else
         {
-            mfn_t gl1mfn = get_gfn_query(v->domain, gl1gfn, &p2mt);
+            mfn_t gl1mfn = get_gfn_query_unlocked(v->domain, gfn_x(gl1gfn),
+                                                  &p2mt);
             if ( p2m_is_ram(p2mt) )
                 sl1mfn = get_shadow_status(v, gl1mfn, SH_type_l1_shadow); 
             else if ( p2mt != p2m_populate_on_demand )
                 result |= SHADOW_SET_ERROR;
-            put_gfn(v->domain, gfn_x(gl1gfn));
         }
     }
     l2e_propagate_from_guest(v, new_gl2e, sl1mfn, &new_sl2e, ft_prefetch);

@@ -2505,11 +2503,9 @@ void sh_resync_l1(struct vcpu *v, mfn_t 
             shadow_l1e_t nsl1e;
 
             gfn = guest_l1e_get_gfn(gl1e);
-            gmfn = get_gfn_query(v->domain, gfn, &p2mt);
+            gmfn = get_gfn_query_unlocked(v->domain, gfn_x(gfn), &p2mt);
             l1e_propagate_from_guest(v, gl1e, gmfn, &nsl1e, ft_prefetch, p2mt);
             rc |= shadow_set_l1e(v, sl1p, nsl1e, p2mt, sl1mfn);
-
-            put_gfn(v->domain, gfn_x(gfn));
             *snpl1p = gl1e;
         }
     });

@@ -2830,7 +2826,7 @@ static void sh_prefetch(struct vcpu *v, 
 
         /* Look at the gfn that the l1e is pointing at */
         gfn = guest_l1e_get_gfn(gl1e);
-        gmfn = get_gfn_query(v->domain, gfn, &p2mt);
+        gmfn = get_gfn_query_unlocked(v->domain, gfn_x(gfn), &p2mt);
 
         /* Propagate the entry.  */
         l1e_propagate_from_guest(v, gl1e, gmfn, &sl1e, ft_prefetch, p2mt);

@@ -2840,8 +2836,6 @@ static void sh_prefetch(struct vcpu *v, 
         if ( snpl1p != NULL )
             snpl1p[i] = gl1e;
 #endif /* OOS */
-
-        put_gfn(v->domain, gfn_x(gfn));
     }
     if ( gl1p != NULL )
         sh_unmap_domain_page(gl1p);

@@ -4323,14 +4317,13 @@ sh_update_cr3(struct vcpu *v, int do_loc
             if ( guest_l3e_get_flags(gl3e[i]) & _PAGE_PRESENT )
             {
                 gl2gfn = guest_l3e_get_gfn(gl3e[i]);
-                gl2mfn = get_gfn_query(d, gl2gfn, &p2mt);
+                gl2mfn = get_gfn_query_unlocked(d, gfn_x(gl2gfn), &p2mt);
                 if ( p2m_is_ram(p2mt) )
                     sh_set_toplevel_shadow(v, i, gl2mfn, (i == 3) 
                                            ? SH_type_l2h_shadow 
                                            : SH_type_l2_shadow);
                 else
                     sh_set_toplevel_shadow(v, i, _mfn(INVALID_MFN), 0); 
-                put_gfn(d, gfn_x(gl2gfn));
             }
             else
                 sh_set_toplevel_shadow(v, i, _mfn(INVALID_MFN), 0);

@@ -4748,9 +4741,8 @@ static void sh_pagetable_dying(struct vc
             /* retrieving the l2s */
             gl2a = guest_l3e_get_paddr(gl3e[i]);
             gfn = gl2a >> PAGE_SHIFT;
-            gmfn = get_gfn_query(v->domain, _gfn(gfn), &p2mt);
+            gmfn = get_gfn_query_unlocked(v->domain, gfn, &p2mt);
             smfn = shadow_hash_lookup(v, mfn_x(gmfn), SH_type_l2_pae_shadow);
-            put_gfn(v->domain, gfn);
         }
 
         if ( mfn_valid(smfn) )

xen/include/asm-x86/p2m.h +5 -0

diff -r 6f89f15fd3c8 xen/include/asm-x86/p2m.h
@@ -360,6 +360,11 @@ void __put_gfn(struct p2m_domain *p2m, u
  * 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. 
  *
+ * This is also used in the shadow code whenever the paging lock is
+ * held -- in those cases, the caller is protected against concurrent
+ * p2m updates by the fact that shadow_write_p2m_entry() also takes
+ * the paging lock.
+ *
  * 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.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andres Lagar-Cavilla

2012-04-19 15:07 UTC #11 lore raw

Re: [PATCH 3 of 6] x86/mm/shadow: fix potential p2m/paging deadlock when emulating page table writes

Quoted 17 lines · Tim Deegan

Quoted 16 lines · Andres Lagar-Cavilla
Quoted 11 lines
Nack, at least for now; we spent a fair amount of effort taking all this emulation code out from under the shadow lock; serializing it under the p2m lock would be unfortunate. The other patches are less worrying, since they wrap a shadow_lock() in a p2m_lock() but I hope they can all be avoided.
The existing interlock between the shadow code and the p2m code prevents any p2m modifications from happening when the shadow lock is held, so I hope I can solve this by switching to unlocked lookups instead. I'm building a test kernel now to tell me exactly which lookps are to blame.
FWIW, get_gfn_query for the target gfn of the PTE entry that is being checked in validate_gl?e entry, is the call that causes the panic this patch tries to fix.

Quoted 8 lines

Quoted 2 lines
As for the other two patches, sh_resync_l1 is the trigger (again, get_gfn_query on the gfn that is contained by the PTE being verified)
Thanks. I've taken a sweep through mm/shadow, making the lookups into unlocked ones wherever the paging_lock is already held. With the attached patch and your final patch to enable the locking, I can start HVM guests without crashing Xen. I haven't given it any more serious testing yet.

It looks good to me. Thanks.
Acked-by Andres Lagar-Cavilla <[email protected]>

Andres

Quoted 4 lines

Cheers,
Tim.<shadow-vs-p2m.txt>

Andres Lagar-Cavilla

author

2012-04-13 16:22 UTC #12 lore raw

PATCH 1/1 x86/mm/shadow: fix p2m/paging deadlock when updating shadow mode

In reply to Andres Lagar-Cavilla

 xen/arch/x86/mm/shadow/common.c |  4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Signed-off-by Andres Lagar-Cavilla <[email protected]>

xen/arch/x86/mm/shadow/common.c +4 -0

diff -r 964c6cbad926 -r a7ca6ae73992 xen/arch/x86/mm/shadow/common.c
@@ -2997,9 +2997,13 @@ static void sh_update_paging_modes(struc
 
 void shadow_update_paging_modes(struct vcpu *v)
 {
+    /* Avoid deadlock in shadow mode. When updating, we might need to resync an
+     * l1 and thus get_gfn on all the gfn's pointed to by the guest l1e pte's. */
+    p2m_lock(p2m_get_hostp2m(v->domain));
     paging_lock(v->domain);
     sh_update_paging_modes(v);
     paging_unlock(v->domain);
+    p2m_unlock(p2m_get_hostp2m(v->domain));
 }
 
 /**************************************************************************/

Andres Lagar-Cavilla

author

2012-04-13 16:22 UTC #13 lore raw

PATCH 1/1 x86/mm/shadow: fix p2m/paging deadlock when updating shadow cr3

In reply to Andres Lagar-Cavilla

 xen/arch/x86/mm/shadow/multi.c |  13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

Signed-off-by Andres Lagar-Cavilla <[email protected]>

xen/arch/x86/mm/shadow/multi.c +11 -2

diff -r a7ca6ae73992 -r 1d8566a05642 xen/arch/x86/mm/shadow/multi.c
@@ -4190,7 +4190,12 @@ sh_update_cr3(struct vcpu *v, int do_loc
         return;
     }
 
-    if ( do_locking ) paging_lock(v->domain);
+    if ( do_locking ) 
+    {
+        /* See comment in shadow_update_paging_modes. */
+        p2m_lock(p2m_get_hostp2m(v->domain));
+        paging_lock(v->domain);
+    }
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
     /* Need to resync all the shadow entries on a TLB flush.  Resync

@@ -4434,7 +4439,11 @@ sh_update_cr3(struct vcpu *v, int do_loc
 #endif
 
     /* Release the lock, if we took it (otherwise it's the caller's problem) */
-    if ( do_locking ) paging_unlock(v->domain);
+    if ( do_locking )
+    {
+        paging_unlock(v->domain);
+        p2m_unlock(p2m_get_hostp2m(v->domain));
+    }
 }

Andres Lagar-Cavilla

author

2012-04-13 16:22 UTC #14 lore raw

PATCH 1/1 x86/mm: Locking p2m lookups in shadow mode

In reply to Andres Lagar-Cavilla

 xen/arch/x86/mm/p2m.c |  5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

Fully synchronized p2m lookups have been added to hap already. Enable that for shadow mode as well.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

xen/arch/x86/mm/p2m.c +2 -3

diff -r 1d8566a05642 -r 8ec52231c03d xen/arch/x86/mm/p2m.c
@@ -164,7 +164,7 @@ mfn_t __get_gfn_type_access(struct p2m_d
     }
 
     /* For now only perform locking on hap domains */
-    if ( locked && (hap_enabled(p2m->domain)) )
+    if ( locked )
         /* Grab the lock here, don't release until put_gfn */
         gfn_lock(p2m, gfn, 0);

@@ -197,8 +197,7 @@ mfn_t __get_gfn_type_access(struct p2m_d
 
 void __put_gfn(struct p2m_domain *p2m, unsigned long gfn)
 {
-    if ( !p2m || !paging_mode_translate(p2m->domain) 
-              || !hap_enabled(p2m->domain) )
+    if ( !p2m || !paging_mode_translate(p2m->domain) )
         /* Nothing to do in this case */
         return;

Tim Deegan

Acked-by on v1 1/1

2012-04-18 16:03 UTC #15 lore raw

Re: [PATCH 6 of 6] x86/mm: Locking p2m lookups in shadow mode

Quoted 8 lines · Andres Lagar-Cavilla

 xen/arch/x86/mm/p2m.c |  5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
Fully synchronized p2m lookups have been added to hap already. Enable that for shadow mode as well.
Signed-off-by Andres Lagar-Cavilla <[email protected]>

Once we're happy with the rest of the series,

Acked-by Tim Deegan <[email protected]>

Tim Deegan

2012-04-13 17:35 UTC #16 lore raw

In reply to Andres Lagar-Cavilla

Quoted 2 lines · Andres Lagar-Cavilla

Allow shadow page tables to take advantage of synchronized p2m lookups. This required a number of deadlock fixes building up to enabling of this feature.

Thanks for this. I'm on holiday this week but I'll look at this (and your other MM patches) next week. I think the other patches are bugfixes and should be OK for 4.2; not sure yet whether this series will make it past the feature freeze.

Cheers,

Tim.

Andres Lagar-Cavilla

author

2012-04-13 17:37 UTC #17 lore raw

Quoted 10 lines

Quoted 4 lines · Andres Lagar-Cavilla
Allow shadow page tables to take advantage of synchronized p2m lookups. This
required a number of deadlock fixes building up to enabling of this feature.
Thanks for this. I'm on holiday this week but I'll look at this (and your other MM patches) next week. I think the other patches are bugfixes and should be OK for 4.2; not sure yet whether this series will make it past the feature freeze.

In terms of priority I rank much higher the sharing scalability patches.

Thanks and enjoy the vacation time
Andres

Quoted 5 lines

Cheers,
Tim.