[PATCH 0 of 6] x86/mm/shadow: fixes and synchronized p2m lookups

[PATCH 0 of 6] x86/mm/shadow: fixes and synchronized p2m lookups

[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.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

 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(-)

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

[Andres Lagar-Cavilla]

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


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 7b606c043208 -r aa4ef559da60 xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/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)         \

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

[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 <andres@lagarcavilla.org>

diff -r aa4ef559da60 -r f8fd4a4239a8 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/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

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

[Andres Lagar-Cavilla]

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


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r f8fd4a4239a8 -r 964c6cbad926 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/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

[PATCH 4 of 6] x86/mm/shadow: fix p2m/paging deadlock when updating shadow mode

[Andres Lagar-Cavilla]

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


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 964c6cbad926 -r a7ca6ae73992 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/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));
 }
 
 /**************************************************************************/

[PATCH 5 of 6] x86/mm/shadow: fix p2m/paging deadlock when updating shadow cr3

[Andres Lagar-Cavilla]

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


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r a7ca6ae73992 -r 1d8566a05642 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/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));
+    }
 }

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

[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 <andres@lagarcavilla.org>

diff -r 1d8566a05642 -r 8ec52231c03d xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/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;

Re: [PATCH 0 of 6] x86/mm/shadow: fixes and synchronized p2m lookups

[Tim Deegan]

At 12:22 -0400 on 13 Apr (1334319738), Andres Lagar-Cavilla wrote:
> 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.

Re: [PATCH 0 of 6] x86/mm/shadow: fixes and synchronized p2m lookups

[Andres Lagar-Cavilla]

> At 12:22 -0400 on 13 Apr (1334319738), Andres Lagar-Cavilla wrote:
>> 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
>
> Cheers,
>
> Tim.
>

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

[Gianluca Guida]

On Fri, Apr 13, 2012 at 9:22 AM, Andres Lagar-Cavilla
<andres@lagarcavilla.org> wrote:
>  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 <gianluca.guida@citrix.com>

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

[Tim Deegan]

At 07:08 -0700 on 14 Apr (1334387280), Gianluca Guida wrote:
> On Fri, Apr 13, 2012 at 9:22 AM, Andres Lagar-Cavilla
> <andres@lagarcavilla.org> wrote:
> >  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 <gianluca.guida@citrix.com>

Applied, thanks.

Tim.

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

[Tim Deegan]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

At 12:22 -0400 on 13 Apr (1334319739), Andres Lagar-Cavilla wrote:
>  xen/arch/x86/mm/mm-locks.h |  3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

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.

[-- Attachment #2: backtrace --]
[-- Type: text/plain, Size: 1284 bytes --]

# HG changeset patch
# User Tim Deegan <tim@xen.org>
# 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 <andres@lagarcavilla.org>
Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 7c777cb8f705 xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h	Wed Apr 18 16:49:55 2012 +0100
+++ b/xen/arch/x86/mm/mm-locks.h	Wed Apr 18 16:50:45 2012 +0100
@@ -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)         \

[-- Attachment #3: parens --]
[-- Type: text/plain, Size: 850 bytes --]

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

Signed-off-by: Tim Deegan <tim@xen.org>

diff -r 87eb032bda77 xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h	Wed Apr 18 16:43:13 2012 +0100
+++ b/xen/arch/x86/mm/mm-locks.h	Wed Apr 18 16:50:49 2012 +0100
@@ -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));                \

[-- Attachment #4: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andres Lagar-Cavilla]

> At 12:22 -0400 on 13 Apr (1334319739), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/mm-locks.h |  3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> 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

>
> Tim.
>

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

[Tim Deegan]

At 12:22 -0400 on 13 Apr (1334319744), Andres Lagar-Cavilla wrote:
>  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 <andres@lagarcavilla.org>

Once we're happy with the rest of the series, 
Acked-by: Tim Deegan <tim@xen.org>

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

[Tim Deegan]

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.

At 12:22 -0400 on 13 Apr (1334319741), Andres Lagar-Cavilla wrote:
>  xen/arch/x86/mm/shadow/multi.c |  25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r f8fd4a4239a8 -r 964c6cbad926 xen/arch/x86/mm/shadow/multi.c
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/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
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Andres Lagar-Cavilla]

> 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

>
> If I don't get this done today I'll look into it tomorrow.
>
> Cheers,
>
> Tim.
>
> At 12:22 -0400 on 13 Apr (1334319741), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/shadow/multi.c |  25 +++++++++++++++++++++++++
>>  1 files changed, 25 insertions(+), 0 deletions(-)
>>
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r f8fd4a4239a8 -r 964c6cbad926 xen/arch/x86/mm/shadow/multi.c
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/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
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>

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

[Tim Deegan]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

At 09:25 -0700 on 18 Apr (1334741158), Andres Lagar-Cavilla wrote:
> > 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)

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.

[-- Attachment #2: shadow-vs-p2m --]
[-- Type: text/plain, Size: 6880 bytes --]

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 <tim@xen.org>

diff -r 6f89f15fd3c8 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c	Thu Apr 19 15:31:30 2012 +0100
+++ b/xen/arch/x86/mm/shadow/common.c	Thu Apr 19 15:48:30 2012 +0100
@@ -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);
diff -r 6f89f15fd3c8 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c	Thu Apr 19 15:31:30 2012 +0100
+++ b/xen/arch/x86/mm/shadow/multi.c	Thu Apr 19 15:48:30 2012 +0100
@@ -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) )
diff -r 6f89f15fd3c8 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h	Thu Apr 19 15:31:30 2012 +0100
+++ b/xen/include/asm-x86/p2m.h	Thu Apr 19 15:48:30 2012 +0100
@@ -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.

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andres Lagar-Cavilla]

On Apr 19, 2012, at 10:51 AM, Tim Deegan wrote:

> At 09:25 -0700 on 18 Apr (1334741158), Andres Lagar-Cavilla wrote:
>>> 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)
> 
> 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 <andres@lagarcavilla.org>

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