Xen devel - Apr 2012 - [PATCH 0 of 6] 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 <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(-)

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

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

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/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));
 }
 
 /**************************************************************************/

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));
+    }
 }

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;

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.

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

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>

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.

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.



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

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

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>

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

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

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.

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

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>