RFC: Nested-p2m cleanups and locking changes

# HG changeset patch
# User Tim Deegan<[email protected]>
# Date 1308758648 -3600
# Node ID c323e69a0a08ce9f1e54d2e2fa2edd9845bc8efe
# Parent b7e5a25663329254cba539e21f4fbd5b32c67556 Nested p2m: implement "flush" as a first-class action rather than using the teardown and init functions.
This makes the locking clearer and avoids an expensive scan of all pfns that's only needed for non-nested p2ms. It also moves the tlb flush into the proper place in the flush logic, avoiding a possible race against other CPUs.

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

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/hvm/nestedhvm.c

xen/arch/x86/hvm/nestedhvm.c +0 -6

@@ -119,12 +119,6 @@ nestedhvm_vmcx_flushtlb(struct p2m_domai
      cpus_clear(p2m->p2m_dirty_cpumask);
  }
 
-void
-nestedhvm_vmcx_flushtlbdomain(struct domain *d)
-{
-    on_selected_cpus(d->domain_dirty_cpumask, nestedhvm_flushtlb_ipi, d, 1);
-}
-
  bool_t
  nestedhvm_is_n2(struct vcpu *v)
  {

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +37 -23

@@ -1050,20 +1050,41 @@ p2m_getlru_nestedp2m(struct domain *d, s
      return lrup2m;
  }
 
-static int
+/* Reset this p2m table to be empty */
+static void
  p2m_flush_locked(struct p2m_domain *p2m)
  {
-    ASSERT(p2m);
-    if (p2m->cr3 == CR3_EADDR)
-        /* Microoptimisation: p2m is already empty.
-         * =>  about 0.3% speedup of overall system performance.
-         */
-        return 0;
+    struct page_info *top, *pg;
+    struct domain *d = p2m->domain;
+    void *p;
 
-    p2m_teardown(p2m);
-    p2m_initialise(p2m->domain, p2m);
-    p2m->write_p2m_entry = nestedp2m_write_p2m_entry;
-    return p2m_alloc_table(p2m);
+    p2m_lock(p2m);
+
+    /* "Host" p2m tables can have shared entries&c that need a bit more
+     * care when discarding them */
+    ASSERT(p2m_is_nestedp2m(p2m));
+    ASSERT(page_list_empty(&p2m->pod.super));
+    ASSERT(page_list_empty(&p2m->pod.single));
+
+    /* This is no longer a valid nested p2m for any address space */
+    p2m->cr3 = CR3_EADDR;
+
+    /* Zap the top level of the trie */
+    top = mfn_to_page(pagetable_get_mfn(p2m_get_pagetable(p2m)));
+    p = map_domain_page(top);
+    clear_page(p);
+    unmap_domain_page(p);
+
+    /* Make sure nobody else is using this p2m table */
+    nestedhvm_vmcx_flushtlb(p2m);
+
+    /* Free the rest of the trie pages back to the paging pool */
+    while ( (pg = page_list_remove_head(&p2m->pages)) )
+        if ( pg != top )
+            d->arch.paging.free_page(d, pg);
+    page_list_add(top,&p2m->pages);
+
+    p2m_unlock(p2m);
  }
 
  void

@@ -1074,9 +1095,8 @@ p2m_flush(struct vcpu *v, struct p2m_dom
      ASSERT(v->domain == d);
      vcpu_nestedhvm(v).nv_p2m = NULL;
      nestedp2m_lock(d);
-    BUG_ON(p2m_flush_locked(p2m) != 0);
+    p2m_flush_locked(p2m);
      hvm_asid_flush_vcpu(v);
-    nestedhvm_vmcx_flushtlb(p2m);
      nestedp2m_unlock(d);
  }
 

@@ -1086,12 +1106,8 @@ p2m_flush_nestedp2m(struct domain *d)
      int i;
 
      nestedp2m_lock(d);
-    for (i = 0; i<  MAX_NESTEDP2M; i++) {
-        struct p2m_domain *p2m = d->arch.nested_p2m[i];
-        BUG_ON(p2m_flush_locked(p2m) != 0);
-        cpus_clear(p2m->p2m_dirty_cpumask);
-    }
-    nestedhvm_vmcx_flushtlbdomain(d);
+    for ( i = 0; i<  MAX_NESTEDP2M; i++ )
+        p2m_flush_locked(d->arch.nested_p2m[i]);
      nestedp2m_unlock(d);
  }
 

@@ -1104,7 +1120,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
      volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
      struct domain *d;
      struct p2m_domain *p2m;
-    int i, rv;
+    int i;
 
      if (cr3 == 0 || cr3 == CR3_EADDR)
          cr3 = v->arch.hvm_vcpu.guest_cr[3];

@@ -1136,9 +1152,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
       */
      for (i = 0; i<  MAX_NESTEDP2M; i++) {
          p2m = p2m_getlru_nestedp2m(d, NULL);
-        rv = p2m_flush_locked(p2m);
-        if (rv == 0)
-            break;
+        p2m_flush_locked(p2m);
      }
      nv->nv_p2m = p2m;
      p2m->cr3 = cr3;

diff -r b7e5a2566332 -r c323e69a0a08 xen/include/asm-x86/hvm/nestedhvm.h

xen/include/asm-x86/hvm/nestedhvm.h +0 -1

@@ -61,7 +61,6 @@ unsigned long *nestedhvm_vcpu_iomap_get(
      (!!vcpu_nestedhvm((v)).nv_vmswitch_in_progress)
 
  void nestedhvm_vmcx_flushtlb(struct p2m_domain *p2m);
-void nestedhvm_vmcx_flushtlbdomain(struct domain *d);
 
  bool_t nestedhvm_is_n2(struct vcpu *v);
 

This patch crashes the host (and it doesn't disappear with the other patches applied):

(XEN) Assertion 'pfn_to_pdx(ma>> PAGE_SHIFT)< (DIRECTMAP_SIZE>> PAGE_SHIFT)'
failed at xen/include/asm/x86_64/page.h:99
(XEN) ----[ Xen-4.2-unstable x86_64 debug=y Tainted: C ]----
(XEN) CPU: 1
(XEN) RIP: e008:[<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN) RFLAGS: 0000000000010212 CONTEXT: hypervisor

(XEN) rax: 0000000000000000   rbx: ffff830421db1d90   rcx: 0000000000000000
(XEN) rdx: f82f608076a60000   rsi: 000f82f608076a60   rdi: 0000000000000000
(XEN) rbp: ffff830425217a08   rsp: ffff8304252179c8   r8:  0000000000000000
(XEN) r9:  0000000000000004   r10: 00000000fffffffb   r11: 0000000000000003
(XEN) r12: ffff830421db1d90   r13: ffff82f608076a60   r14: ffff830403bbebe8
(XEN) r15: ffff830403bbe000   cr0: 000000008005003b   cr4: 00000000000406f0
(XEN) cr3: 0000000420f2b000   cr2: 000000000045f000
(XEN) ds: 0017   es: 0017   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen stack trace from rsp=ffff8304252179c8:
(XEN)    ffff830403bbebe8 ffff830421db1d90 ffff830421db1d90 0000000000000000
(XEN)    ffff830403bbe000 ffff830403bbebe8 ffff830403bbebe8 ffff830403bbeab0
(XEN)    ffff830425217a38 ffff82c4801d2599 0000000000000001 0000000000000067
(XEN)    ffff830403bbeab0 ffff830403af4000 ffff830425217a88 ffff82c480208a8c
(XEN)    0000000125941810 ffff830403bbe000 0000000000000000 ffff830425217b18
(XEN)    ffff830421db1cc0 00000000000ff000 ffffffffffffffff 0000000000000000
(XEN)    ffff830425217a98 ffff82c4801ce585 ffff830425217b68 ffff82c4801d4b4e
(XEN)    0000000000000200 1000000000000000 ffff830425217b28 ffff830425217b20
(XEN)    0000000000000001 ffff830425217ae8 ffff830425217ef8 ffff830403af4000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffff830403af4000
(XEN)    0000000000403af4 ffff82c4801d5ec1 ffff830425217bfc ffffffffffffffff
(XEN)    0000000000000000 0000000000000001 00000000000ff000 ffff830421db1cc0
(XEN)    ffff830425217bb8 ffff82c4801d02d1 0000000100000007 ffff830403bbe000
(XEN)    0000000125217bc8 ffff8280019fafc0 ffff82c400cfd7e0 000000000033f5f8
(XEN)    ffff830421db1cc0 0000000000000001 ffff830425217c28 ffff82c4801d053d
(XEN)    ffff830425217bfc ffff830425217bf8 0000000025217bf0 00000000000ff000
(XEN)    0000000000000001 ffff830425217c10 0000000000000007 ffff830421db1cc0
(XEN)    ffff830421db1cc0 ffff830425217f18 ffff82c4802e1260 0000000000000000
(XEN)    ffff830425217c78 ffff82c4801d11fa 000000000033f5f7 00000000000ff000
(XEN) Xen call trace:
(XEN)    [<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN)    [<ffff82c4801d2599>] p2m_flush_nestedp2m+0xea/0x140
(XEN)    [<ffff82c480208a8c>] hap_write_p2m_entry+0x223/0x246
(XEN)    [<ffff82c4801ce585>] paging_write_p2m_entry+0x6e/0x75
(XEN)    [<ffff82c4801d4b4e>] p2m_set_entry+0x42b/0x6a8
(XEN)    [<ffff82c4801d02d1>] set_p2m_entry+0xf2/0x140
(XEN)    [<ffff82c4801d053d>] p2m_remove_page+0x1dd/0x1ec
(XEN)    [<ffff82c4801d11fa>] guest_physmap_remove_page+0x109/0x148
(XEN)    [<ffff82c480168000>] arch_memory_op+0x6af/0x1039
(XEN)    [<ffff82c4801134fb>] do_memory_op+0x1bd7/0x1c2c
(XEN)    [<ffff82c480217708>] syscall_enter+0xc8/0x122

(XEN)
(XEN)
(XEN) ****************************************

Christoph

Quoted 144 lines · Tim Deegan

# HG changeset patch
# User Tim Deegan<[email protected]>
# Date 1308758648 -3600
# Node ID c323e69a0a08ce9f1e54d2e2fa2edd9845bc8efe
# Parent b7e5a25663329254cba539e21f4fbd5b32c67556 Nested p2m: implement "flush" as a first-class action rather than using the teardown and init functions.
This makes the locking clearer and avoids an expensive scan of all pfns that's only needed for non-nested p2ms. It also moves the tlb flush into the proper place in the flush logic, avoiding a possible race against other CPUs.

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

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/hvm/nestedhvm.c

xen/arch/x86/hvm/nestedhvm.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/hvm/nestedhvm.c Tue Jun 21 18:28:53 2011 +0100 +0 -6

@@ -119,12 +119,6 @@ nestedhvm_vmcx_flushtlb(struct p2m_domai
       cpus_clear(p2m->p2m_dirty_cpumask);
   }
 
-void
-nestedhvm_vmcx_flushtlbdomain(struct domain *d)
-{
-    on_selected_cpus(d->domain_dirty_cpumask, nestedhvm_flushtlb_ipi, d, 1);
-}
-
   bool_t
   nestedhvm_is_n2(struct vcpu *v)
   {

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +37 -23

@@ -1050,20 +1050,41 @@ p2m_getlru_nestedp2m(struct domain *d, s
       return lrup2m;
   }
 
-static int
+/* Reset this p2m table to be empty */
+static void
   p2m_flush_locked(struct p2m_domain *p2m)
   {
-    ASSERT(p2m);
-    if (p2m->cr3 == CR3_EADDR)
-        /* Microoptimisation: p2m is already empty.
-         * =>   about 0.3% speedup of overall system performance.
-         */
-        return 0;
+    struct page_info *top, *pg;
+    struct domain *d = p2m->domain;
+    void *p;
 
-    p2m_teardown(p2m);
-    p2m_initialise(p2m->domain, p2m);
-    p2m->write_p2m_entry = nestedp2m_write_p2m_entry;
-    return p2m_alloc_table(p2m);
+    p2m_lock(p2m);
+
+    /* "Host" p2m tables can have shared entries&c that need a bit more
+     * care when discarding them */
+    ASSERT(p2m_is_nestedp2m(p2m));
+    ASSERT(page_list_empty(&p2m->pod.super));
+    ASSERT(page_list_empty(&p2m->pod.single));
+
+    /* This is no longer a valid nested p2m for any address space */
+    p2m->cr3 = CR3_EADDR;
+
+    /* Zap the top level of the trie */
+    top = mfn_to_page(pagetable_get_mfn(p2m_get_pagetable(p2m)));
+    p = map_domain_page(top);
+    clear_page(p);
+    unmap_domain_page(p);
+
+    /* Make sure nobody else is using this p2m table */
+    nestedhvm_vmcx_flushtlb(p2m);
+
+    /* Free the rest of the trie pages back to the paging pool */
+    while ( (pg = page_list_remove_head(&p2m->pages)) )
+        if ( pg != top )
+            d->arch.paging.free_page(d, pg);
+    page_list_add(top,&p2m->pages);
+
+    p2m_unlock(p2m);
   }
 
   void

@@ -1074,9 +1095,8 @@ p2m_flush(struct vcpu *v, struct p2m_dom
       ASSERT(v->domain == d);
       vcpu_nestedhvm(v).nv_p2m = NULL;
       nestedp2m_lock(d);
-    BUG_ON(p2m_flush_locked(p2m) != 0);
+    p2m_flush_locked(p2m);
       hvm_asid_flush_vcpu(v);
-    nestedhvm_vmcx_flushtlb(p2m);
       nestedp2m_unlock(d);
   }
 

@@ -1086,12 +1106,8 @@ p2m_flush_nestedp2m(struct domain *d)
       int i;
 
       nestedp2m_lock(d);
-    for (i = 0; i<   MAX_NESTEDP2M; i++) {
-        struct p2m_domain *p2m = d->arch.nested_p2m[i];
-        BUG_ON(p2m_flush_locked(p2m) != 0);
-        cpus_clear(p2m->p2m_dirty_cpumask);
-    }
-    nestedhvm_vmcx_flushtlbdomain(d);
+    for ( i = 0; i<   MAX_NESTEDP2M; i++ )
+        p2m_flush_locked(d->arch.nested_p2m[i]);
       nestedp2m_unlock(d);
   }
 

@@ -1104,7 +1120,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
       volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
       struct domain *d;
       struct p2m_domain *p2m;
-    int i, rv;
+    int i;
 
       if (cr3 == 0 || cr3 == CR3_EADDR)
           cr3 = v->arch.hvm_vcpu.guest_cr[3];

@@ -1136,9 +1152,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
        */
       for (i = 0; i<   MAX_NESTEDP2M; i++) {
           p2m = p2m_getlru_nestedp2m(d, NULL);
-        rv = p2m_flush_locked(p2m);
-        if (rv == 0)
-            break;
+        p2m_flush_locked(p2m);
       }
       nv->nv_p2m = p2m;
       p2m->cr3 = cr3;

diff -r b7e5a2566332 -r c323e69a0a08 xen/include/asm-x86/hvm/nestedhvm.h

xen/include/asm-x86/hvm/nestedhvm.h Wed Jun 22 17:04:08 2011 +0100 renamed from xen/include/asm-x86/hvm/nestedhvm.h Tue Jun 21 18:28:53 2011 +0100 +0 -1

@@ -61,7 +61,6 @@ unsigned long *nestedhvm_vcpu_iomap_get(
       (!!vcpu_nestedhvm((v)).nv_vmswitch_in_progress)
 
   void nestedhvm_vmcx_flushtlb(struct p2m_domain *p2m);
-void nestedhvm_vmcx_flushtlbdomain(struct domain *d);
 
   bool_t nestedhvm_is_n2(struct vcpu *v);
 

Quoted 3 lines · Christoph Egger

This patch crashes the host (and it doesn't disappear with the other patches applied):

err.. it crashes the host when the l1 guest boots the hvmloader invokes the VGA BIOS.

Quoted 94 lines

(XEN) Assertion 'pfn_to_pdx(ma>> PAGE_SHIFT)< (DIRECTMAP_SIZE>>
PAGE_SHIFT)'
failed at xen/include/asm/x86_64/page.h:99
(XEN) ----[ Xen-4.2-unstable x86_64 debug=y Tainted: C ]----
(XEN) CPU: 1
(XEN) RIP: e008:[<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN) RFLAGS: 0000000000010212 CONTEXT: hypervisor

(XEN) rax: 0000000000000000   rbx: ffff830421db1d90   rcx: 0000000000000000
(XEN) rdx: f82f608076a60000   rsi: 000f82f608076a60   rdi: 0000000000000000
(XEN) rbp: ffff830425217a08   rsp: ffff8304252179c8   r8:  0000000000000000
(XEN) r9:  0000000000000004   r10: 00000000fffffffb   r11: 0000000000000003
(XEN) r12: ffff830421db1d90   r13: ffff82f608076a60   r14: ffff830403bbebe8
(XEN) r15: ffff830403bbe000   cr0: 000000008005003b   cr4: 00000000000406f0
(XEN) cr3: 0000000420f2b000   cr2: 000000000045f000
(XEN) ds: 0017   es: 0017   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen stack trace from rsp=ffff8304252179c8:
(XEN)    ffff830403bbebe8 ffff830421db1d90 ffff830421db1d90 0000000000000000
(XEN)    ffff830403bbe000 ffff830403bbebe8 ffff830403bbebe8 ffff830403bbeab0
(XEN)    ffff830425217a38 ffff82c4801d2599 0000000000000001 0000000000000067
(XEN)    ffff830403bbeab0 ffff830403af4000 ffff830425217a88 ffff82c480208a8c
(XEN)    0000000125941810 ffff830403bbe000 0000000000000000 ffff830425217b18
(XEN)    ffff830421db1cc0 00000000000ff000 ffffffffffffffff 0000000000000000
(XEN)    ffff830425217a98 ffff82c4801ce585 ffff830425217b68 ffff82c4801d4b4e
(XEN)    0000000000000200 1000000000000000 ffff830425217b28 ffff830425217b20
(XEN)    0000000000000001 ffff830425217ae8 ffff830425217ef8 ffff830403af4000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffff830403af4000
(XEN)    0000000000403af4 ffff82c4801d5ec1 ffff830425217bfc ffffffffffffffff
(XEN)    0000000000000000 0000000000000001 00000000000ff000 ffff830421db1cc0
(XEN)    ffff830425217bb8 ffff82c4801d02d1 0000000100000007 ffff830403bbe000
(XEN)    0000000125217bc8 ffff8280019fafc0 ffff82c400cfd7e0 000000000033f5f8
(XEN)    ffff830421db1cc0 0000000000000001 ffff830425217c28 ffff82c4801d053d
(XEN)    ffff830425217bfc ffff830425217bf8 0000000025217bf0 00000000000ff000
(XEN)    0000000000000001 ffff830425217c10 0000000000000007 ffff830421db1cc0
(XEN)    ffff830421db1cc0 ffff830425217f18 ffff82c4802e1260 0000000000000000
(XEN)    ffff830425217c78 ffff82c4801d11fa 000000000033f5f7 00000000000ff000
(XEN) Xen call trace:
(XEN)    [<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN)    [<ffff82c4801d2599>] p2m_flush_nestedp2m+0xea/0x140
(XEN)    [<ffff82c480208a8c>] hap_write_p2m_entry+0x223/0x246
(XEN)    [<ffff82c4801ce585>] paging_write_p2m_entry+0x6e/0x75
(XEN)    [<ffff82c4801d4b4e>] p2m_set_entry+0x42b/0x6a8
(XEN)    [<ffff82c4801d02d1>] set_p2m_entry+0xf2/0x140
(XEN)    [<ffff82c4801d053d>] p2m_remove_page+0x1dd/0x1ec
(XEN)    [<ffff82c4801d11fa>] guest_physmap_remove_page+0x109/0x148
(XEN)    [<ffff82c480168000>] arch_memory_op+0x6af/0x1039
(XEN)    [<ffff82c4801134fb>] do_memory_op+0x1bd7/0x1c2c
(XEN)    [<ffff82c480217708>] syscall_enter+0xc8/0x122

(XEN)
(XEN)
(XEN) ****************************************

Christoph

Quoted 37 lines · Tim Deegan

# HG changeset patch
# User Tim Deegan<[email protected]>
# Date 1308758648 -3600
# Node ID c323e69a0a08ce9f1e54d2e2fa2edd9845bc8efe
# Parent b7e5a25663329254cba539e21f4fbd5b32c67556 Nested p2m: implement "flush" as a first-class action rather than using the teardown and init functions.
This makes the locking clearer and avoids an expensive scan of all pfns that's only needed for non-nested p2ms. It also moves the tlb flush into the proper place in the flush logic, avoiding a possible race against other CPUs.

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

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/hvm/nestedhvm.c

xen/arch/x86/hvm/nestedhvm.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/hvm/nestedhvm.c Tue Jun 21 18:28:53 2011 +0100 +0 -6

@@ -119,12 +119,6 @@ nestedhvm_vmcx_flushtlb(struct p2m_domai
        cpus_clear(p2m->p2m_dirty_cpumask);
    }
 
-void
-nestedhvm_vmcx_flushtlbdomain(struct domain *d)
-{
-    on_selected_cpus(d->domain_dirty_cpumask, nestedhvm_flushtlb_ipi, d, 1);
-}
-
    bool_t
    nestedhvm_is_n2(struct vcpu *v)
    {

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +0 -0

@@ -1050,20 +1050,41 @@ p2m_getlru_nestedp2m(struct domain *d, s
        return lrup2m;
    }
 

Quoted 6 lines

Quoted 6 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +2 -2

-static int
+/* Reset this p2m table to be empty */
+static void
    p2m_flush_locked(struct p2m_domain *p2m)
    {
-    ASSERT(p2m);

Quoted 8 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 p2m_getlru_nestedp2m(struct domain *d, s · line 1059

Quoted 8 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +3 -5

-    if (p2m->cr3 == CR3_EADDR)
-        /* Microoptimisation: p2m is already empty.
-         * =>    about 0.3% speedup of overall system performance.
-         */
-        return 0;
+    struct page_info *top, *pg;
+    struct domain *d = p2m->domain;
+    void *p;

Quoted 17 lines

Quoted 7 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +2 -4

-    p2m_teardown(p2m);
-    p2m_initialise(p2m->domain, p2m);
-    p2m->write_p2m_entry = nestedp2m_write_p2m_entry;
-    return p2m_alloc_table(p2m);
+    p2m_lock(p2m);
+

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 p2m_getlru_nestedp2m(struct domain *d, s · line 1071

Quoted 10 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +10 -0

+    /* "Host" p2m tables can have shared entries&c that need a bit more
+     * care when discarding them */
+    ASSERT(p2m_is_nestedp2m(p2m));
+    ASSERT(page_list_empty(&p2m->pod.super));
+    ASSERT(page_list_empty(&p2m->pod.single));
+
+    /* This is no longer a valid nested p2m for any address space */
+    p2m->cr3 = CR3_EADDR;
+
+    /* Zap the top level of the trie */

Quoted 74 lines

Quoted 74 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +18 -12

+    clear_page(p);
+    unmap_domain_page(p);
+
+    /* Make sure nobody else is using this p2m table */
+    nestedhvm_vmcx_flushtlb(p2m);
+
+    /* Free the rest of the trie pages back to the paging pool */
+    while ( (pg = page_list_remove_head(&p2m->pages)) )
+        if ( pg != top )
+            d->arch.paging.free_page(d, pg);
+    page_list_add(top,&p2m->pages);
+
+    p2m_unlock(p2m);
    }
 
    void

@@ -1074,9 +1095,8 @@ p2m_flush(struct vcpu *v, struct p2m_dom
        ASSERT(v->domain == d);
        vcpu_nestedhvm(v).nv_p2m = NULL;
        nestedp2m_lock(d);
-    BUG_ON(p2m_flush_locked(p2m) != 0);
+    p2m_flush_locked(p2m);
        hvm_asid_flush_vcpu(v);
-    nestedhvm_vmcx_flushtlb(p2m);
        nestedp2m_unlock(d);
    }
 

@@ -1086,12 +1106,8 @@ p2m_flush_nestedp2m(struct domain *d)
        int i;
 
        nestedp2m_lock(d);
-    for (i = 0; i<    MAX_NESTEDP2M; i++) {
-        struct p2m_domain *p2m = d->arch.nested_p2m[i];
-        BUG_ON(p2m_flush_locked(p2m) != 0);
-        cpus_clear(p2m->p2m_dirty_cpumask);
-    }
-    nestedhvm_vmcx_flushtlbdomain(d);
+    for ( i = 0; i<    MAX_NESTEDP2M; i++ )
+        p2m_flush_locked(d->arch.nested_p2m[i]);
        nestedp2m_unlock(d);
    }
 

@@ -1104,7 +1120,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
        volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
        struct domain *d;
        struct p2m_domain *p2m;
-    int i, rv;
+    int i;
 
        if (cr3 == 0 || cr3 == CR3_EADDR)
            cr3 = v->arch.hvm_vcpu.guest_cr[3];

@@ -1136,9 +1152,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
         */
        for (i = 0; i<    MAX_NESTEDP2M; i++) {
            p2m = p2m_getlru_nestedp2m(d, NULL);
-        rv = p2m_flush_locked(p2m);
-        if (rv == 0)
-            break;
+        p2m_flush_locked(p2m);
        }
        nv->nv_p2m = p2m;
        p2m->cr3 = cr3;

diff -r b7e5a2566332 -r c323e69a0a08 xen/include/asm-x86/hvm/nestedhvm.h

xen/include/asm-x86/hvm/nestedhvm.h Wed Jun 22 17:04:08 2011 +0100 renamed from xen/include/asm-x86/hvm/nestedhvm.h Tue Jun 21 18:28:53 2011 +0100 +0 -1

@@ -61,7 +61,6 @@ unsigned long *nestedhvm_vcpu_iomap_get(
        (!!vcpu_nestedhvm((v)).nv_vmswitch_in_progress)
 
    void nestedhvm_vmcx_flushtlb(struct p2m_domain *p2m);
-void nestedhvm_vmcx_flushtlbdomain(struct domain *d);
 
    bool_t nestedhvm_is_n2(struct vcpu *v);
 

I have a fix for this crash. See inline.

Christoph

Quoted 102 lines · Christoph Egger

Quoted 3 lines · Christoph Egger

This patch crashes the host (and it doesn't disappear with the other patches applied):

err.. it crashes the host when the l1 guest boots the hvmloader invokes the VGA BIOS.

Quoted 94 lines

(XEN) Assertion 'pfn_to_pdx(ma>> PAGE_SHIFT)< (DIRECTMAP_SIZE>>
PAGE_SHIFT)'
failed at xen/include/asm/x86_64/page.h:99
(XEN) ----[ Xen-4.2-unstable x86_64 debug=y Tainted: C ]----
(XEN) CPU: 1
(XEN) RIP: e008:[<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN) RFLAGS: 0000000000010212 CONTEXT: hypervisor

(XEN) rax: 0000000000000000   rbx: ffff830421db1d90   rcx: 0000000000000000
(XEN) rdx: f82f608076a60000   rsi: 000f82f608076a60   rdi: 0000000000000000
(XEN) rbp: ffff830425217a08   rsp: ffff8304252179c8   r8:  0000000000000000
(XEN) r9:  0000000000000004   r10: 00000000fffffffb   r11: 0000000000000003
(XEN) r12: ffff830421db1d90   r13: ffff82f608076a60   r14: ffff830403bbebe8
(XEN) r15: ffff830403bbe000   cr0: 000000008005003b   cr4: 00000000000406f0
(XEN) cr3: 0000000420f2b000   cr2: 000000000045f000
(XEN) ds: 0017   es: 0017   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen stack trace from rsp=ffff8304252179c8:
(XEN)    ffff830403bbebe8 ffff830421db1d90 ffff830421db1d90 0000000000000000
(XEN)    ffff830403bbe000 ffff830403bbebe8 ffff830403bbebe8 ffff830403bbeab0
(XEN)    ffff830425217a38 ffff82c4801d2599 0000000000000001 0000000000000067
(XEN)    ffff830403bbeab0 ffff830403af4000 ffff830425217a88 ffff82c480208a8c
(XEN)    0000000125941810 ffff830403bbe000 0000000000000000 ffff830425217b18
(XEN)    ffff830421db1cc0 00000000000ff000 ffffffffffffffff 0000000000000000
(XEN)    ffff830425217a98 ffff82c4801ce585 ffff830425217b68 ffff82c4801d4b4e
(XEN)    0000000000000200 1000000000000000 ffff830425217b28 ffff830425217b20
(XEN)    0000000000000001 ffff830425217ae8 ffff830425217ef8 ffff830403af4000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffff830403af4000
(XEN)    0000000000403af4 ffff82c4801d5ec1 ffff830425217bfc ffffffffffffffff
(XEN)    0000000000000000 0000000000000001 00000000000ff000 ffff830421db1cc0
(XEN)    ffff830425217bb8 ffff82c4801d02d1 0000000100000007 ffff830403bbe000
(XEN)    0000000125217bc8 ffff8280019fafc0 ffff82c400cfd7e0 000000000033f5f8
(XEN)    ffff830421db1cc0 0000000000000001 ffff830425217c28 ffff82c4801d053d
(XEN)    ffff830425217bfc ffff830425217bf8 0000000025217bf0 00000000000ff000
(XEN)    0000000000000001 ffff830425217c10 0000000000000007 ffff830421db1cc0
(XEN)    ffff830421db1cc0 ffff830425217f18 ffff82c4802e1260 0000000000000000
(XEN)    ffff830425217c78 ffff82c4801d11fa 000000000033f5f7 00000000000ff000
(XEN) Xen call trace:
(XEN)    [<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN)    [<ffff82c4801d2599>] p2m_flush_nestedp2m+0xea/0x140
(XEN)    [<ffff82c480208a8c>] hap_write_p2m_entry+0x223/0x246
(XEN)    [<ffff82c4801ce585>] paging_write_p2m_entry+0x6e/0x75
(XEN)    [<ffff82c4801d4b4e>] p2m_set_entry+0x42b/0x6a8
(XEN)    [<ffff82c4801d02d1>] set_p2m_entry+0xf2/0x140
(XEN)    [<ffff82c4801d053d>] p2m_remove_page+0x1dd/0x1ec
(XEN)    [<ffff82c4801d11fa>] guest_physmap_remove_page+0x109/0x148
(XEN)    [<ffff82c480168000>] arch_memory_op+0x6af/0x1039
(XEN)    [<ffff82c4801134fb>] do_memory_op+0x1bd7/0x1c2c
(XEN)    [<ffff82c480217708>] syscall_enter+0xc8/0x122

(XEN)
(XEN)
(XEN) ****************************************

Christoph

Quoted 37 lines · Tim Deegan

# HG changeset patch
# User Tim Deegan<[email protected]>
# Date 1308758648 -3600
# Node ID c323e69a0a08ce9f1e54d2e2fa2edd9845bc8efe
# Parent b7e5a25663329254cba539e21f4fbd5b32c67556 Nested p2m: implement "flush" as a first-class action rather than using the teardown and init functions.
This makes the locking clearer and avoids an expensive scan of all pfns that's only needed for non-nested p2ms. It also moves the tlb flush into the proper place in the flush logic, avoiding a possible race against other CPUs.

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

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/hvm/nestedhvm.c

xen/arch/x86/hvm/nestedhvm.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/hvm/nestedhvm.c Tue Jun 21 18:28:53 2011 +0100 +0 -6

@@ -119,12 +119,6 @@ nestedhvm_vmcx_flushtlb(struct p2m_domai
         cpus_clear(p2m->p2m_dirty_cpumask);
     }
 
-void
-nestedhvm_vmcx_flushtlbdomain(struct domain *d)
-{
-    on_selected_cpus(d->domain_dirty_cpumask, nestedhvm_flushtlb_ipi, d, 1);
-}
-
     bool_t
     nestedhvm_is_n2(struct vcpu *v)
     {

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +0 -0

@@ -1050,20 +1050,41 @@ p2m_getlru_nestedp2m(struct domain *d, s
         return lrup2m;
     }
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +2 -2

-static int
+/* Reset this p2m table to be empty */
+static void
     p2m_flush_locked(struct p2m_domain *p2m)
     {
-    ASSERT(p2m);

Quoted 8 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 p2m_getlru_nestedp2m(struct domain *d, s · line 1059

Quoted 8 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +3 -5

-    if (p2m->cr3 == CR3_EADDR)
-        /* Microoptimisation: p2m is already empty.
-         * =>     about 0.3% speedup of overall system performance.
-         */
-        return 0;
+    struct page_info *top, *pg;
+    struct domain *d = p2m->domain;
+    void *p;

+ mfn_t top_mfn;

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +12 -4

-    p2m_teardown(p2m);
-    p2m_initialise(p2m->domain, p2m);
-    p2m->write_p2m_entry = nestedp2m_write_p2m_entry;
-    return p2m_alloc_table(p2m);
+    p2m_lock(p2m);
+
+    /* "Host" p2m tables can have shared entries&c that need a bit more
+     * care when discarding them */
+    ASSERT(p2m_is_nestedp2m(p2m));
+    ASSERT(page_list_empty(&p2m->pod.super));
+    ASSERT(page_list_empty(&p2m->pod.single));
+
+    /* This is no longer a valid nested p2m for any address space */
+    p2m->cr3 = CR3_EADDR;
+
+    /* Zap the top level of the trie */

(quoted diff) +3 -0

+    top_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
+    top = mfn_to_page(top_mfn);
+    p = map_domain_page(mfn_x(top_mfn));

Quoted 75 lines

Quoted 75 lines

Quoted 75 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +18 -12

+    clear_page(p);
+    unmap_domain_page(p);
+
+    /* Make sure nobody else is using this p2m table */
+    nestedhvm_vmcx_flushtlb(p2m);
+
+    /* Free the rest of the trie pages back to the paging pool */
+    while ( (pg = page_list_remove_head(&p2m->pages)) )
+        if ( pg != top )
+            d->arch.paging.free_page(d, pg);
+    page_list_add(top,&p2m->pages);
+
+    p2m_unlock(p2m);
     }
 
     void

@@ -1074,9 +1095,8 @@ p2m_flush(struct vcpu *v, struct p2m_dom
         ASSERT(v->domain == d);
         vcpu_nestedhvm(v).nv_p2m = NULL;
         nestedp2m_lock(d);
-    BUG_ON(p2m_flush_locked(p2m) != 0);
+    p2m_flush_locked(p2m);
         hvm_asid_flush_vcpu(v);
-    nestedhvm_vmcx_flushtlb(p2m);
         nestedp2m_unlock(d);
     }
 

@@ -1086,12 +1106,8 @@ p2m_flush_nestedp2m(struct domain *d)
         int i;
 
         nestedp2m_lock(d);
-    for (i = 0; i<     MAX_NESTEDP2M; i++) {
-        struct p2m_domain *p2m = d->arch.nested_p2m[i];
-        BUG_ON(p2m_flush_locked(p2m) != 0);
-        cpus_clear(p2m->p2m_dirty_cpumask);
-    }
-    nestedhvm_vmcx_flushtlbdomain(d);
+    for ( i = 0; i<     MAX_NESTEDP2M; i++ )
+        p2m_flush_locked(d->arch.nested_p2m[i]);
         nestedp2m_unlock(d);
     }
 

@@ -1104,7 +1120,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
         volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
         struct domain *d;
         struct p2m_domain *p2m;
-    int i, rv;
+    int i;
 
         if (cr3 == 0 || cr3 == CR3_EADDR)
             cr3 = v->arch.hvm_vcpu.guest_cr[3];

@@ -1136,9 +1152,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
          */
         for (i = 0; i<     MAX_NESTEDP2M; i++) {
             p2m = p2m_getlru_nestedp2m(d, NULL);
-        rv = p2m_flush_locked(p2m);
-        if (rv == 0)
-            break;
+        p2m_flush_locked(p2m);
         }
         nv->nv_p2m = p2m;
         p2m->cr3 = cr3;

diff -r b7e5a2566332 -r c323e69a0a08 xen/include/asm-x86/hvm/nestedhvm.h

xen/include/asm-x86/hvm/nestedhvm.h Wed Jun 22 17:04:08 2011 +0100 renamed from xen/include/asm-x86/hvm/nestedhvm.h Tue Jun 21 18:28:53 2011 +0100 +0 -1

@@ -61,7 +61,6 @@ unsigned long *nestedhvm_vcpu_iomap_get(
         (!!vcpu_nestedhvm((v)).nv_vmswitch_in_progress)
 
     void nestedhvm_vmcx_flushtlb(struct p2m_domain *p2m);
-void nestedhvm_vmcx_flushtlbdomain(struct domain *d);
 
     bool_t nestedhvm_is_n2(struct vcpu *v);
 

Now I see a significant slowdown of the L2 guest with multiple vcpus. The reason is I see 10 times more IPIs for each vcpu with p2m_flush_nestedp2m().

Please add back nestedhvm_vmcx_flushtlbdomain().

Quoted 221 lines

I have a fix for this crash. See inline.

Christoph

Quoted 102 lines · Christoph Egger

Quoted 3 lines · Christoph Egger

This patch crashes the host (and it doesn't disappear with the other patches applied):

err.. it crashes the host when the l1 guest boots the hvmloader invokes the VGA BIOS.

Quoted 94 lines

(XEN) Assertion 'pfn_to_pdx(ma>> PAGE_SHIFT)< (DIRECTMAP_SIZE>>
PAGE_SHIFT)'
failed at xen/include/asm/x86_64/page.h:99
(XEN) ----[ Xen-4.2-unstable x86_64 debug=y Tainted: C ]----
(XEN) CPU: 1
(XEN) RIP: e008:[<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN) RFLAGS: 0000000000010212 CONTEXT: hypervisor

(XEN) rax: 0000000000000000   rbx: ffff830421db1d90   rcx: 0000000000000000
(XEN) rdx: f82f608076a60000   rsi: 000f82f608076a60   rdi: 0000000000000000
(XEN) rbp: ffff830425217a08   rsp: ffff8304252179c8   r8:  0000000000000000
(XEN) r9:  0000000000000004   r10: 00000000fffffffb   r11: 0000000000000003
(XEN) r12: ffff830421db1d90   r13: ffff82f608076a60   r14: ffff830403bbebe8
(XEN) r15: ffff830403bbe000   cr0: 000000008005003b   cr4: 00000000000406f0
(XEN) cr3: 0000000420f2b000   cr2: 000000000045f000
(XEN) ds: 0017   es: 0017   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen stack trace from rsp=ffff8304252179c8:
(XEN)    ffff830403bbebe8 ffff830421db1d90 ffff830421db1d90 0000000000000000
(XEN)    ffff830403bbe000 ffff830403bbebe8 ffff830403bbebe8 ffff830403bbeab0
(XEN)    ffff830425217a38 ffff82c4801d2599 0000000000000001 0000000000000067
(XEN)    ffff830403bbeab0 ffff830403af4000 ffff830425217a88 ffff82c480208a8c
(XEN)    0000000125941810 ffff830403bbe000 0000000000000000 ffff830425217b18
(XEN)    ffff830421db1cc0 00000000000ff000 ffffffffffffffff 0000000000000000
(XEN)    ffff830425217a98 ffff82c4801ce585 ffff830425217b68 ffff82c4801d4b4e
(XEN)    0000000000000200 1000000000000000 ffff830425217b28 ffff830425217b20
(XEN)    0000000000000001 ffff830425217ae8 ffff830425217ef8 ffff830403af4000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffff830403af4000
(XEN)    0000000000403af4 ffff82c4801d5ec1 ffff830425217bfc ffffffffffffffff
(XEN)    0000000000000000 0000000000000001 00000000000ff000 ffff830421db1cc0
(XEN)    ffff830425217bb8 ffff82c4801d02d1 0000000100000007 ffff830403bbe000
(XEN)    0000000125217bc8 ffff8280019fafc0 ffff82c400cfd7e0 000000000033f5f8
(XEN)    ffff830421db1cc0 0000000000000001 ffff830425217c28 ffff82c4801d053d
(XEN)    ffff830425217bfc ffff830425217bf8 0000000025217bf0 00000000000ff000
(XEN)    0000000000000001 ffff830425217c10 0000000000000007 ffff830421db1cc0
(XEN)    ffff830421db1cc0 ffff830425217f18 ffff82c4802e1260 0000000000000000
(XEN)    ffff830425217c78 ffff82c4801d11fa 000000000033f5f7 00000000000ff000
(XEN) Xen call trace:
(XEN)    [<ffff82c4801d2007>] p2m_flush_locked+0x1a8/0x2be
(XEN)    [<ffff82c4801d2599>] p2m_flush_nestedp2m+0xea/0x140
(XEN)    [<ffff82c480208a8c>] hap_write_p2m_entry+0x223/0x246
(XEN)    [<ffff82c4801ce585>] paging_write_p2m_entry+0x6e/0x75
(XEN)    [<ffff82c4801d4b4e>] p2m_set_entry+0x42b/0x6a8
(XEN)    [<ffff82c4801d02d1>] set_p2m_entry+0xf2/0x140
(XEN)    [<ffff82c4801d053d>] p2m_remove_page+0x1dd/0x1ec
(XEN)    [<ffff82c4801d11fa>] guest_physmap_remove_page+0x109/0x148
(XEN)    [<ffff82c480168000>] arch_memory_op+0x6af/0x1039
(XEN)    [<ffff82c4801134fb>] do_memory_op+0x1bd7/0x1c2c
(XEN)    [<ffff82c480217708>] syscall_enter+0xc8/0x122

(XEN)
(XEN)
(XEN) ****************************************

Christoph

Quoted 37 lines · Tim Deegan

# HG changeset patch
# User Tim Deegan<[email protected]>
# Date 1308758648 -3600
# Node ID c323e69a0a08ce9f1e54d2e2fa2edd9845bc8efe
# Parent b7e5a25663329254cba539e21f4fbd5b32c67556 Nested p2m: implement "flush" as a first-class action rather than using the teardown and init functions.
This makes the locking clearer and avoids an expensive scan of all pfns that's only needed for non-nested p2ms. It also moves the tlb flush into the proper place in the flush logic, avoiding a possible race against other CPUs.

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

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/hvm/nestedhvm.c

xen/arch/x86/hvm/nestedhvm.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/hvm/nestedhvm.c Tue Jun 21 18:28:53 2011 +0100 +0 -6

@@ -119,12 +119,6 @@ nestedhvm_vmcx_flushtlb(struct p2m_domai
        cpus_clear(p2m->p2m_dirty_cpumask);
    }
 
-void
-nestedhvm_vmcx_flushtlbdomain(struct domain *d)
-{
-    on_selected_cpus(d->domain_dirty_cpumask, nestedhvm_flushtlb_ipi, d, 1);
-}
-
    bool_t
    nestedhvm_is_n2(struct vcpu *v)
    {

diff -r b7e5a2566332 -r c323e69a0a08 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +0 -0

@@ -1050,20 +1050,41 @@ p2m_getlru_nestedp2m(struct domain *d, s
        return lrup2m;
    }
 

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +2 -2

-static int
+/* Reset this p2m table to be empty */
+static void
    p2m_flush_locked(struct p2m_domain *p2m)
    {
-    ASSERT(p2m);

Quoted 8 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 p2m_getlru_nestedp2m(struct domain *d, s · line 1059

Quoted 8 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +3 -5

-    if (p2m->cr3 == CR3_EADDR)
-        /* Microoptimisation: p2m is already empty.
-         * =>     about 0.3% speedup of overall system performance.
-         */
-        return 0;
+    struct page_info *top, *pg;
+    struct domain *d = p2m->domain;
+    void *p;

+ mfn_t top_mfn;

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +12 -4

-    p2m_teardown(p2m);
-    p2m_initialise(p2m->domain, p2m);
-    p2m->write_p2m_entry = nestedp2m_write_p2m_entry;
-    return p2m_alloc_table(p2m);
+    p2m_lock(p2m);
+
+    /* "Host" p2m tables can have shared entries&c that need a bit more
+     * care when discarding them */
+    ASSERT(p2m_is_nestedp2m(p2m));
+    ASSERT(page_list_empty(&p2m->pod.super));
+    ASSERT(page_list_empty(&p2m->pod.single));
+
+    /* This is no longer a valid nested p2m for any address space */
+    p2m->cr3 = CR3_EADDR;
+
+    /* Zap the top level of the trie */

(quoted diff) +3 -0

+    top_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m));
+    top = mfn_to_page(top_mfn);
+    p = map_domain_page(mfn_x(top_mfn));

Quoted 75 lines

Quoted 75 lines

Quoted 75 lines

xen/arch/x86/mm/p2m.c Wed Jun 22 17:04:08 2011 +0100 renamed from xen/arch/x86/mm/p2m.c Tue Jun 21 18:28:53 2011 +0100 +18 -12

+    clear_page(p);
+    unmap_domain_page(p);
+
+    /* Make sure nobody else is using this p2m table */
+    nestedhvm_vmcx_flushtlb(p2m);
+
+    /* Free the rest of the trie pages back to the paging pool */
+    while ( (pg = page_list_remove_head(&p2m->pages)) )
+        if ( pg != top )
+            d->arch.paging.free_page(d, pg);
+    page_list_add(top,&p2m->pages);
+
+    p2m_unlock(p2m);
    }
 
    void

@@ -1074,9 +1095,8 @@ p2m_flush(struct vcpu *v, struct p2m_dom
        ASSERT(v->domain == d);
        vcpu_nestedhvm(v).nv_p2m = NULL;
        nestedp2m_lock(d);
-    BUG_ON(p2m_flush_locked(p2m) != 0);
+    p2m_flush_locked(p2m);
        hvm_asid_flush_vcpu(v);
-    nestedhvm_vmcx_flushtlb(p2m);
        nestedp2m_unlock(d);
    }
 

@@ -1086,12 +1106,8 @@ p2m_flush_nestedp2m(struct domain *d)
        int i;
 
        nestedp2m_lock(d);
-    for (i = 0; i<     MAX_NESTEDP2M; i++) {
-        struct p2m_domain *p2m = d->arch.nested_p2m[i];
-        BUG_ON(p2m_flush_locked(p2m) != 0);
-        cpus_clear(p2m->p2m_dirty_cpumask);
-    }
-    nestedhvm_vmcx_flushtlbdomain(d);
+    for ( i = 0; i<     MAX_NESTEDP2M; i++ )
+        p2m_flush_locked(d->arch.nested_p2m[i]);
        nestedp2m_unlock(d);
    }
 

@@ -1104,7 +1120,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
        volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
        struct domain *d;
        struct p2m_domain *p2m;
-    int i, rv;
+    int i;
 
        if (cr3 == 0 || cr3 == CR3_EADDR)
            cr3 = v->arch.hvm_vcpu.guest_cr[3];

@@ -1136,9 +1152,7 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
         */
        for (i = 0; i<     MAX_NESTEDP2M; i++) {
            p2m = p2m_getlru_nestedp2m(d, NULL);
-        rv = p2m_flush_locked(p2m);
-        if (rv == 0)
-            break;
+        p2m_flush_locked(p2m);
        }
        nv->nv_p2m = p2m;
        p2m->cr3 = cr3;

diff -r b7e5a2566332 -r c323e69a0a08 xen/include/asm-x86/hvm/nestedhvm.h

xen/include/asm-x86/hvm/nestedhvm.h Wed Jun 22 17:04:08 2011 +0100 renamed from xen/include/asm-x86/hvm/nestedhvm.h Tue Jun 21 18:28:53 2011 +0100 +0 -1

@@ -61,7 +61,6 @@ unsigned long *nestedhvm_vcpu_iomap_get(
        (!!vcpu_nestedhvm((v)).nv_vmswitch_in_progress)
 
    void nestedhvm_vmcx_flushtlb(struct p2m_domain *p2m);
-void nestedhvm_vmcx_flushtlbdomain(struct domain *d);
 
    bool_t nestedhvm_is_n2(struct vcpu *v);
 

Signature · 12 lines

---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
[email protected]
http://lists.xensource.com/xen-devel

# HG changeset patch
# User Tim Deegan<[email protected]>
# Date 1308758648 -3600
# Node ID b265371addbbc8a58c95a269fe3cd0fdc866aaa3 # Parent dcb8ae5e3eaf6516c889087dfb15efa41a1ac3e9 Nested p2m: clarify logic in p2m_get_nestedp2m()

This just makes the behaviour of this function a bit more explicit. It may be that it also needs to be changed. :)

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

diff -r dcb8ae5e3eaf -r b265371addbb xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +0 -1

@@ -1131,11 +1131,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
 
      d = v->domain;
      nestedp2m_lock(d);
-    for (i = 0; i<  MAX_NESTEDP2M; i++) {

Quoted 9 lines

diff -r dcb8ae5e3eaf -r b265371addbb xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +0 -1

@@ -1131,11 +1131,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
 
      d = v->domain;
      nestedp2m_lock(d);
-    for (i = 0; i<  MAX_NESTEDP2M; i++) {

xen/arch/x86/mm/p2m.c p2m_get_nestedp2m(struct vcpu *v, uint64 · line 1139

Quoted 10 lines

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

-        p2m = d->arch.nested_p2m[i];
-        if ((p2m->cr3 != cr3&&  p2m->cr3 != CR3_EADDR) || (p2m != nv->nv_p2m))
-            continue;
-
+    p2m = nv->nv_p2m;
+    if ( p2m&&  (p2m->cr3 == cr3 || p2m->cr3 == CR3_EADDR) )
+    {
          nv->nv_flushp2m = 0;
          p2m_getlru_nestedp2m(d, p2m);
          nv->nv_p2m = p2m;

Ok, thanks.

In p2m_get_nestedp2m() replace this code hunk

     for (i = 0; i < MAX_NESTEDP2M; i++) {
         p2m = p2m_getlru_nestedp2m(d, NULL);
         p2m_flush_locked(p2m);
     }

with

     p2m = p2m_getlru_nestedp2m(d, NULL);
     p2m_flush_locked(p2m);

Quoted 30 lines · Christoph Egger

Quoted 9 lines

diff -r dcb8ae5e3eaf -r b265371addbb xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +0 -1

@@ -1131,11 +1131,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
 
       d = v->domain;
       nestedp2m_lock(d);
-    for (i = 0; i<   MAX_NESTEDP2M; i++) {

xen/arch/x86/mm/p2m.c p2m_get_nestedp2m(struct vcpu *v, uint64 · line 1139

Quoted 10 lines

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

-        p2m = d->arch.nested_p2m[i];
-        if ((p2m->cr3 != cr3&&   p2m->cr3 != CR3_EADDR) || (p2m != nv->nv_p2m))
-            continue;
-
+    p2m = nv->nv_p2m;
+    if ( p2m&&   (p2m->cr3 == cr3 || p2m->cr3 == CR3_EADDR) )
+    {
           nv->nv_flushp2m = 0;
           p2m_getlru_nestedp2m(d, p2m);
           nv->nv_p2m = p2m;

Ok, thanks.

In p2m_get_nestedp2m() replace this code hunk

      for (i = 0; i<  MAX_NESTEDP2M; i++) {
          p2m = p2m_getlru_nestedp2m(d, NULL);
          p2m_flush_locked(p2m);
      }

Quoted 4 lines

with

      p2m = p2m_getlru_nestedp2m(d, NULL);
      p2m_flush_locked(p2m);

That seems like an improvement. I'll put it into my queue.

More generally, I think that you need to figure out exactly what behaviour you want from this function. For example in the current code there's no way that two vcpus with the same ncr3 value can share a nested-p2m. Is that deliberate?

Quoted 4 lines

More generally, I think that you need to figure out exactly what behaviour you want from this function. For example in the current code there's no way that two vcpus with the same ncr3 value can share a nested-p2m. Is that deliberate?

By 'current code' do you mean with or w/o this patch ?

It is deliberate that two vcpus with the same ncr3 share a nested-p2m.

But fixing the p2m locking problem in upstream tree has a higher priority right now and we can work on that after the p2m locking issue is fixed upstream.

Hi,

Quoted 6 lines · Christoph Egger

Quoted 4 lines

More generally, I think that you need to figure out exactly what behaviour you want from this function. For example in the current code there's no way that two vcpus with the same ncr3 value can share a nested-p2m. Is that deliberate?

By 'current code' do you mean with or w/o this patch ?

Both, and all versions of the code from before my current series to the full series applied.

Quoted 1 line

It is deliberate that two vcpus with the same ncr3 share a nested-p2m.

But they don't. The code in current unstable tip does this:

     for (i = 0; i<  MAX_NESTEDP2M; i++) {
         p2m = d->arch.nested_p2m[i];
         if ((p2m->cr3 != cr3&&  p2m->cr3 != CR3_EADDR) || (p2m != nv->nv_p2m))
             continue;

	// ... return this p2m
     }

     /* All p2m's are or were in use. Take the least recent used one,
      * flush it and reuse.
      */
     for (i = 0; i<  MAX_NESTEDP2M; i++) {
         p2m = p2m_getlru_nestedp2m(d, NULL);
         rv = p2m_flush_locked(p2m);
         if (rv == 0)
             break;
     }

     // ... return this p2m

The first loop never returns a p2m that's != nv->nv_p2m. The second loop always returns a fresh, flushed p2m. So there's no way that two different vcpus, starting with nv->nv_p2m == NULL, can ever get the same p2m as each other.

The pseudocode is basically:
- If I have an existing nv_p2m and it hasn't been flushed, reuse it.
- Else flush all np2ms in LRU order and return the last one flushed.

My patch 3/4 doesn't change the logic at all (I think); your latest fix just avoids the over-aggressive flushing of all np2ms.

Quoted 3 lines

But fixing the p2m locking problem in upstream tree has a higher priority right now and we can work on that after the p2m locking issue is fixed upstream.

AFAICS the locking is fixed by the current set of patches

(though I'm still not able to run Xen-in-Xen well enough to test them).

I can send the full series again for clarity if you like.

The outstanding bug is that there are many more IPIs than previously; I suspect that your latest fix will reduce them quite a lot by avoiding a storm of mutually-destructive flush operations.

If the performance is still too bad we can add more IPI-avoidance strategies.

# HG changeset patch
# User Tim Deegan<[email protected]>
# 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<[email protected]>

diff -r b265371addbb -r a168af9b6d2f xen/arch/x86/mm/hap/nested_hap.c

xen/arch/x86/mm/hap/nested_hap.c +4 -4

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

xen/arch/x86/mm/hap/nested_hap.c +1 -3

      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

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

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

xen/arch/x86/mm/p2m.c +22 -19

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

xen/include/asm-x86/p2m.h +6 -1

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

Review

Quoted 7 lines

(quoted diff) +7 -0

+    /* 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:

Quoted 10 lines · Christoph Egger

Review

Quoted 7 lines

(quoted diff) +7 -0

+    /* 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:

D'oh!

diff -r b40d4bcca0d7 xen/arch/x86/mm/hap/nested_hap.c

This is what comes of not being able to test things properly. Can you describe your test setup for me? I'm having no luck getting xen-on-debian-on-xen-on-debian to work.