[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] Re: [PATCH 4 of 4] Nested p2m: rework locking around nested-p2m flushes and updates



On 06/22/11 18:10, Tim Deegan wrote:
# HG changeset patch
# User Tim Deegan<Tim.Deegan@xxxxxxxxxx>
# Date 1308758840 -3600
# Node ID a168af9b6d2f604c841465e5ed911b7a12b5ba15
# Parent  b265371addbbc8a58c95a269fe3cd0fdc866aaa3
Nested p2m: rework locking around nested-p2m flushes and updates.

The nestedp2m_lock now only covers the mapping from nested-cr3 to
nested-p2m; the tables themselves may be updated or flushed using only
the relevant p2m lock.

This means that the nested-p2m lock is only taken on one path, and
always before any p2m locks.

Signed-off-by: Tim Deegan<Tim.Deegan@xxxxxxxxxx>

diff -r b265371addbb -r a168af9b6d2f xen/arch/x86/mm/hap/nested_hap.c
--- a/xen/arch/x86/mm/hap/nested_hap.c  Wed Jun 22 17:04:08 2011 +0100
+++ b/xen/arch/x86/mm/hap/nested_hap.c  Wed Jun 22 17:07:20 2011 +0100
@@ -96,17 +96,23 @@ nestedp2m_write_p2m_entry(struct p2m_dom
  /*          NESTED VIRT FUNCTIONS           */
  /********************************************/
  static void
-nestedhap_fix_p2m(struct p2m_domain *p2m, paddr_t L2_gpa, paddr_t L0_gpa,
-    p2m_type_t p2mt, p2m_access_t p2ma)
+nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
+                  paddr_t L2_gpa, paddr_t L0_gpa,
+                  p2m_type_t p2mt, p2m_access_t p2ma)
  {
-    int rv;
+    int rv = 0;
      ASSERT(p2m);
      ASSERT(p2m->set_entry);

      p2m_lock(p2m);
-    rv = set_p2m_entry(p2m, L2_gpa>>  PAGE_SHIFT,
-                         page_to_mfn(maddr_to_page(L0_gpa)),
-                         0 /*4K*/, p2mt, p2ma);
+
+    /* If this p2m table has been flushed or recycled under our feet,
+     * leave it alone.  We'll pick up the right one as we try to
+     * vmenter the guest. */
+    if ( p2m->cr3 == nhvm_vcpu_hostcr3(v) )
+         rv = set_p2m_entry(p2m, L2_gpa>>  PAGE_SHIFT,
+                            page_to_mfn(maddr_to_page(L0_gpa)),
+                            0 /*4K*/, p2mt, p2ma);

The introduction of this check leads to this crash when the L2 guest
initializes its third vcpu:


(XEN) nested_hap.c:121:d1 failed to set entry for 0xebcff0 -> 0x2654bcff0
(XEN) Xen BUG at nested_hap.c:122
(XEN) ----[ Xen-4.2-unstable  x86_64  debug=y  Tainted:    C ]----
(XEN) CPU:    1
(XEN) RIP: e008:[<ffff82c48020aa70>] nestedhvm_hap_nested_page_fault+0x27f/0x29d
(XEN) RFLAGS: 0000000000010292   CONTEXT: hypervisor
(XEN) rax: 0000000000000000   rbx: 0000000000000000   rcx: 0000000000000001
(XEN) rdx: ffff82c48025b2e8   rsi: 000000000000000a   rdi: ffff82c48025b2e8
(XEN) rbp: ffff830425217cf8   rsp: ffff830425217ca8   r8:  0000000000000001
(XEN) r9:  0000000000000004   r10: 0000000000000004   r11: 0000000000000004
(XEN) r12: ffff8300cf2a1000   r13: ffff830421db1d90   r14: ffff830421db1d90
(XEN) r15: ffff830421db1d90   cr0: 000000008005003b   cr4: 00000000000406f0
(XEN) cr3: 0000000403b02000   cr2: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0010   gs: 0010   ss: 0000   cs: e008
(XEN) Xen stack trace from rsp=ffff830425217ca8:
(XEN)    ffff8300cf2a1000 0000000000ebcff0 00000002654bcff0 00000002654bcff0
(XEN)    00000000d98bcff0 0000000000000000 ffff8300cf2a1000 0000000000ebcff0
(XEN)    ffff8300cf2a1000 0000000000ebcff0 ffff830425217d58 ffff82c4801aa78d
(XEN)    00000000cf2a1000 ffffffffffffffff 00ff830425217d28 ffff82c4801a66ad
(XEN)    ffff830425217d58 0000000000000000 0000000000000ebc 0000000000ebcff0
(XEN)    ffff8300cf2a1000 ffff83023ec07000 ffff830425217f08 ffff82c4801c0faa
(XEN)    ffff830400000000 ffff82c48017638e ffff830425217d90 ffff830421db1d90
(XEN) ffff830425217f18<G><0>nested_hap.c:121:d1 failed to set entry for 0x7ff8 ->
0x2644d1ff8
(XEN)  0100000000000293Xen BUG at nested_hap.c:122

Christoph



      p2m_unlock(p2m);

      if (rv == 0) {
@@ -211,12 +217,10 @@ nestedhvm_hap_nested_page_fault(struct v
          break;
      }

-    nestedp2m_lock(d);
      /* fix p2m_get_pagetable(nested_p2m) */
-    nestedhap_fix_p2m(nested_p2m, L2_gpa, L0_gpa,
+    nestedhap_fix_p2m(v, nested_p2m, L2_gpa, L0_gpa,
          p2m_ram_rw,
          p2m_access_rwx /* FIXME: Should use same permission as l1 guest */);
-    nestedp2m_unlock(d);

      return NESTEDHVM_PAGEFAULT_DONE;
  }
diff -r b265371addbb -r a168af9b6d2f xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h        Wed Jun 22 17:04:08 2011 +0100
+++ b/xen/arch/x86/mm/mm-locks.h        Wed Jun 22 17:07:20 2011 +0100
@@ -96,8 +96,11 @@ declare_mm_lock(shr)

  /* Nested P2M lock (per-domain)
   *
- * A per-domain lock that protects some of the nested p2m datastructures.
- * TODO: find out exactly what needs to be covered by this lock */
+ * A per-domain lock that protects the mapping from nested-CR3 to
+ * nested-p2m.  In particular it covers:
+ * - the array of nested-p2m tables, and all LRU activity therein; and
+ * - setting the "cr3" field of any p2m table to a non-CR3_EADDR value.
+ *   (i.e. assigning a p2m table to be the shadow of that cr3 */

  declare_mm_lock(nestedp2m)
  #define nestedp2m_lock(d)   mm_lock(nestedp2m,&(d)->arch.nested_p2m_lock)
diff -r b265371addbb -r a168af9b6d2f xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c     Wed Jun 22 17:04:08 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c     Wed Jun 22 17:07:20 2011 +0100
@@ -1052,7 +1052,7 @@ p2m_getlru_nestedp2m(struct domain *d, s

  /* Reset this p2m table to be empty */
  static void
-p2m_flush_locked(struct p2m_domain *p2m)
+p2m_flush_table(struct p2m_domain *p2m)
  {
      struct page_info *top, *pg;
      struct domain *d = p2m->domain;
@@ -1094,21 +1094,16 @@ p2m_flush(struct vcpu *v, struct p2m_dom

      ASSERT(v->domain == d);
      vcpu_nestedhvm(v).nv_p2m = NULL;
-    nestedp2m_lock(d);
-    p2m_flush_locked(p2m);
+    p2m_flush_table(p2m);
      hvm_asid_flush_vcpu(v);
-    nestedp2m_unlock(d);
  }

  void
  p2m_flush_nestedp2m(struct domain *d)
  {
      int i;
-
-    nestedp2m_lock(d);
      for ( i = 0; i<  MAX_NESTEDP2M; i++ )
-        p2m_flush_locked(d->arch.nested_p2m[i]);
-    nestedp2m_unlock(d);
+        p2m_flush_table(d->arch.nested_p2m[i]);
  }

  struct p2m_domain *
@@ -1132,17 +1127,23 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
      d = v->domain;
      nestedp2m_lock(d);
      p2m = nv->nv_p2m;
-    if ( p2m&&  (p2m->cr3 == cr3 || p2m->cr3 == CR3_EADDR) )
+    if ( p2m )
      {
-        nv->nv_flushp2m = 0;
-        p2m_getlru_nestedp2m(d, p2m);
-        nv->nv_p2m = p2m;
-        if (p2m->cr3 == CR3_EADDR)
-            hvm_asid_flush_vcpu(v);
-        p2m->cr3 = cr3;
-        cpu_set(v->processor, p2m->p2m_dirty_cpumask);
-        nestedp2m_unlock(d);
-        return p2m;
+        p2m_lock(p2m);
+        if ( p2m->cr3 == cr3 || p2m->cr3 == CR3_EADDR )
+        {
+            nv->nv_flushp2m = 0;
+            p2m_getlru_nestedp2m(d, p2m);
+            nv->nv_p2m = p2m;
+            if (p2m->cr3 == CR3_EADDR)
+                hvm_asid_flush_vcpu(v);
+            p2m->cr3 = cr3;
+            cpu_set(v->processor, p2m->p2m_dirty_cpumask);
+            p2m_unlock(p2m);
+            nestedp2m_unlock(d);
+            return p2m;
+        }
+        p2m_unlock(p2m)
      }

      /* All p2m's are or were in use. Take the least recent used one,
@@ -1150,14 +1151,16 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
       */
      for (i = 0; i<  MAX_NESTEDP2M; i++) {
          p2m = p2m_getlru_nestedp2m(d, NULL);
-        p2m_flush_locked(p2m);
+        p2m_flush_table(p2m);
      }
+    p2m_lock(p2m);
      nv->nv_p2m = p2m;
      p2m->cr3 = cr3;
      nv->nv_flushp2m = 0;
      hvm_asid_flush_vcpu(v);
      nestedhvm_vmcx_flushtlb(nv->nv_p2m);
      cpu_set(v->processor, p2m->p2m_dirty_cpumask);
+    p2m_unlock(p2m);
      nestedp2m_unlock(d);

      return p2m;
diff -r b265371addbb -r a168af9b6d2f xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h Wed Jun 22 17:04:08 2011 +0100
+++ b/xen/include/asm-x86/p2m.h Wed Jun 22 17:07:20 2011 +0100
@@ -201,8 +201,13 @@ struct p2m_domain {
      cpumask_t          p2m_dirty_cpumask;

      struct domain     *domain;   /* back pointer to domain */
+
+    /* Nested p2ms only: nested-CR3 value that this p2m shadows.
+     * This can be cleared to CR3_EADDR under the per-p2m lock but
+     * needs both the per-p2m lock and the per-domain nestedp2m lock
+     * to set it to any other value. */
  #define CR3_EADDR     (~0ULL)
-    uint64_t           cr3;      /* to identify this p2m for re-use */
+    uint64_t           cr3;

      /* Pages used to construct the p2m */
      struct page_list_head pages;



--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.