[PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Tim Deegan]

This patch series tidies up a few bits ofthe nested p2m code.
The main thing it does is reorganize the locking so that most of the
changes to nested p2m tables happen only under the p2m lock, and the
nestedp2m lock is only needed to reassign p2m tables to new cr3 values.

Changes since v1:

 - a few minor fixes
 - more sensible flushing policy in p2m_get_nestedp2m()
 - smoke-tested this time!

Tim.

[PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

[Tim Deegan]

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 44c7b977302663487922a5d822d1d4032badfebc
# Parent  b240183197720129a8d83847bc5592d6dff3d530
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 <Tim.Deegan@citrix.com>

diff -r b24018319772 -r 44c7b9773026 xen/arch/x86/hvm/nestedhvm.c
--- a/xen/arch/x86/hvm/nestedhvm.c	Thu Jun 23 18:34:55 2011 +0100
+++ b/xen/arch/x86/hvm/nestedhvm.c	Fri Jun 24 16:24:44 2011 +0100
@@ -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 b24018319772 -r 44c7b9773026 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Thu Jun 23 18:34:55 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -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 b24018319772 -r 44c7b9773026 xen/include/asm-x86/hvm/nestedhvm.h
--- a/xen/include/asm-x86/hvm/nestedhvm.h	Thu Jun 23 18:34:55 2011 +0100
+++ b/xen/include/asm-x86/hvm/nestedhvm.h	Fri Jun 24 16:24:44 2011 +0100
@@ -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);

[PATCH 2 of 5] Nested p2m: remove bogus check of CR3 value

[Tim Deegan]

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 3c756243b74302daee24fa77b9000e4039c897e3
# Parent  44c7b977302663487922a5d822d1d4032badfebc
Nested p2m: remove bogus check of CR3 value.

0 is a valid CR3 value; CR3_EADDR isn't but there's nothing stopping a
guest from putting it in its VMCB.  The special case was broken anyway
since AFAICT "p2m->cr3" is a nester-cr3 (i.e. p2m-table) value and
guest_cr[3] is an actual-cr3 (pagetable) value.

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 44c7b9773026 -r 3c756243b743 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -1122,8 +1122,8 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     struct p2m_domain *p2m;
     int i;
 
-    if (cr3 == 0 || cr3 == CR3_EADDR)
-        cr3 = v->arch.hvm_vcpu.guest_cr[3];
+    /* Mask out low bits; this avoids collisions with CR3_EADDR */
+    cr3 &= ~(0xfffull);
 
     if (nv->nv_flushp2m && nv->nv_p2m) {
         nv->nv_p2m = NULL;

[PATCH 3 of 5] Nested p2m: clarify logic in p2m_get_nestedp2m()

[Tim Deegan]

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 97e15368260c093078e1f1bc04521de30c1792cc
# Parent  3c756243b74302daee24fa77b9000e4039c897e3
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 <Tim.Deegan@citrix.com>

diff -r 3c756243b743 -r 97e15368260c xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -1131,11 +1131,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
 
     d = v->domain;
     nestedp2m_lock(d);
-    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;
-
+    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;

[PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

[Tim Deegan]

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929084 -3600
# Node ID 0753351afbbe1c3fdde3a72dfb5a67105524f813
# Parent  97e15368260c093078e1f1bc04521de30c1792cc
Nested p2m: flush only one p2m table when reallocating.
It's unhelpful to flush all of them when we only need one.

Reported-by: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 97e15368260c -r 0753351afbbe xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
@@ -1120,7 +1120,6 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     volatile struct nestedvcpu *nv = &vcpu_nestedhvm(v);
     struct domain *d;
     struct p2m_domain *p2m;
-    int i;
 
     /* Mask out low bits; this avoids collisions with CR3_EADDR */
     cr3 &= ~(0xfffull);
@@ -1146,12 +1145,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
     }
 
     /* 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);
-        p2m_flush_locked(p2m);
-    }
+     * flush it and reuse. */
+    p2m = p2m_getlru_nestedp2m(d, NULL);
+    p2m_flush_locked(p2m);
     nv->nv_p2m = p2m;
     p2m->cr3 = cr3;
     nv->nv_flushp2m = 0;

[PATCH 5 of 5] Nested p2m: rework locking around nested-p2m flushes and updates

[Tim Deegan]

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1308929085 -3600
# Node ID c82cebcfec2546260e4c3b75bb6a47cfdf8bc162
# Parent  0753351afbbe1c3fdde3a72dfb5a67105524f813
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@citrix.com>

diff -r 0753351afbbe -r c82cebcfec25 xen/arch/x86/mm/hap/nested_hap.c
--- a/xen/arch/x86/mm/hap/nested_hap.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/hap/nested_hap.c	Fri Jun 24 16:24:45 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 = 1;
     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);
     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 0753351afbbe -r c82cebcfec25 xen/arch/x86/mm/mm-locks.h
--- a/xen/arch/x86/mm/mm-locks.h	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/mm-locks.h	Fri Jun 24 16:24:45 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 0753351afbbe -r c82cebcfec25 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:45 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 *
@@ -1131,29 +1126,37 @@ 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,
      * flush it and reuse. */
     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 0753351afbbe -r c82cebcfec25 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h	Fri Jun 24 16:24:44 2011 +0100
+++ b/xen/include/asm-x86/p2m.h	Fri Jun 24 16:24:45 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;

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Tim Deegan]

At 11:46 +0100 on 27 Jun (1309175170), Tim Deegan wrote:
> This patch series tidies up a few bits ofthe nested p2m code.
> The main thing it does is reorganize the locking so that most of the
> changes to nested p2m tables happen only under the p2m lock, and the
> nestedp2m lock is only needed to reassign p2m tables to new cr3 values.

There are still a few things I'm not convinced about in the nested NPT
code:

 - The function that allocates new nested p2ms probably needs an
   overhaul, as I said in my last email. 
 - The flushing policy is a bit confusing: e.g., what exactly ought to 
   happen when the guest sets the tlb-control bits?  AFAICS the nested-p2ms 
   are already kept in sync with host-p2m changes, and we flush all 
   TLBs when we update nested-p2ms, so can we skip this extra flush?
 - Why is there a 10x increase in IPIs after this series?  I don't see
   what sequence of events sets the relevant cpumask bits to make this
   happen.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Christoph Egger]

On 06/27/11 12:56, Tim Deegan wrote:
> At 11:46 +0100 on 27 Jun (1309175170), Tim Deegan wrote:
>> This patch series tidies up a few bits ofthe nested p2m code.
>> The main thing it does is reorganize the locking so that most of the
>> changes to nested p2m tables happen only under the p2m lock, and the
>> nestedp2m lock is only needed to reassign p2m tables to new cr3 values.
>
> There are still a few things I'm not convinced about in the nested NPT
> code:
>
>   - The function that allocates new nested p2ms probably needs an
>     overhaul, as I said in my last email.

Ack.

>   - The flushing policy is a bit confusing: e.g., what exactly ought to
>     happen when the guest sets the tlb-control bits?  AFAICS the nested-p2ms
>     are already kept in sync with host-p2m changes, and we flush all
>     TLBs when we update nested-p2ms, so can we skip this extra flush?

Yes, we can.

>   - Why is there a 10x increase in IPIs after this series?  I don't see
>     what sequence of events sets the relevant cpumask bits to make this
>     happen.

In patch 1 the code that sends the IPIs was outside of the loop and 
moved into the loop.

Christoph


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

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Tim Deegan]

At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> >  - Why is there a 10x increase in IPIs after this series?  I don't see
> >    what sequence of events sets the relevant cpumask bits to make this
> >    happen.
> 
> In patch 1 the code that sends the IPIs was outside of the loop and
> moved into the loop.

Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
are burning through np2m tables very quickly indeed.  Maybe removing the
extra flushes for TLB control will do the trick.  I'll make a patch...

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Tim Deegan]

At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> > >  - Why is there a 10x increase in IPIs after this series?  I don't see
> > >    what sequence of events sets the relevant cpumask bits to make this
> > >    happen.
> > 
> > In patch 1 the code that sends the IPIs was outside of the loop and
> > moved into the loop.
> 
> Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
> are burning through np2m tables very quickly indeed.  Maybe removing the
> extra flushes for TLB control will do the trick.  I'll make a patch...

Hmmm, on second thoughts, we can't remove those flushes after all. 
The np2m is in sync with the p2m but not with the guest-supplied p2m, 
so we do need to flush it when the guest asks for that to happen.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Christoph Egger]

On 06/27/11 15:20, Tim Deegan wrote:
> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>   - Why is there a 10x increase in IPIs after this series?  I don't see
>>>>     what sequence of events sets the relevant cpumask bits to make this
>>>>     happen.
>>>
>>> In patch 1 the code that sends the IPIs was outside of the loop and
>>> moved into the loop.
>>
>> Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
>> are burning through np2m tables very quickly indeed.  Maybe removing the
>> extra flushes for TLB control will do the trick.  I'll make a patch...
>
> Hmmm, on second thoughts, we can't remove those flushes after all.
> The np2m is in sync with the p2m but not with the guest-supplied p2m,
> so we do need to flush it when the guest asks for that to happen.

Right. I thought on that when I developped the code but then I forgot.
We should add a comment.

Christoph


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

Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

[Christoph Egger]

I think, this patch can be folded into the first one.
Otherwise:

Ack-by: Christoph Egger <Christoph.Egger@amd.com>

On 06/27/11 12:46, Tim Deegan wrote:
> # HG changeset patch
> # User Tim Deegan<Tim.Deegan@citrix.com>
> # Date 1308929084 -3600
> # Node ID 0753351afbbe1c3fdde3a72dfb5a67105524f813
> # Parent  97e15368260c093078e1f1bc04521de30c1792cc
> Nested p2m: flush only one p2m table when reallocating.
> It's unhelpful to flush all of them when we only need one.
>
> Reported-by: Christoph Egger<Christoph.Egger@amd.com>
> Signed-off-by: Tim Deegan<Tim.Deegan@citrix.com>
>
> diff -r 97e15368260c -r 0753351afbbe xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> +++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> @@ -1120,7 +1120,6 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
>       volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
>       struct domain *d;
>       struct p2m_domain *p2m;
> -    int i;
>
>       /* Mask out low bits; this avoids collisions with CR3_EADDR */
>       cr3&= ~(0xfffull);
> @@ -1146,12 +1145,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
>       }
>
>       /* 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);
> -        p2m_flush_locked(p2m);
> -    }
> +     * flush it and reuse. */
> +    p2m = p2m_getlru_nestedp2m(d, NULL);
> +    p2m_flush_locked(p2m);
>       nv->nv_p2m = p2m;
>       p2m->cr3 = cr3;
>       nv->nv_flushp2m = 0;
>


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

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Tim Deegan]

At 15:24 +0200 on 27 Jun (1309188245), Christoph Egger wrote:
> On 06/27/11 15:20, Tim Deegan wrote:
> >At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
> >>At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> >>>>  - Why is there a 10x increase in IPIs after this series?  I don't see
> >>>>    what sequence of events sets the relevant cpumask bits to make this
> >>>>    happen.
> >>>
> >>>In patch 1 the code that sends the IPIs was outside of the loop and
> >>>moved into the loop.
> >>
> >>Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
> >>are burning through np2m tables very quickly indeed.  Maybe removing the
> >>extra flushes for TLB control will do the trick.  I'll make a patch...
> >
> >Hmmm, on second thoughts, we can't remove those flushes after all.
> >The np2m is in sync with the p2m but not with the guest-supplied p2m,
> >so we do need to flush it when the guest asks for that to happen.

And futhermore we can't share np2ms between vcpus because that could 
violate the TLB's coherence rules. E.g.:
 - vcpu 1 uses ncr3 A, gets np2m A', A' is populated from A;
 - vcpu 1 switches to ncr3 B;
 - guest updates p2m @ A, knows there are no users so doesn't flush it;
 - vcpu 2 uses ncr3 A, gets np2m A' with stale data.

So in fact we have to have per-vcpu np2ms, unless we want a lot of
implicit flushes.  Which means we can discard the 'cr3' field in the
nested p2m.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating

[Tim Deegan]

At 15:44 +0200 on 27 Jun (1309189494), Christoph Egger wrote:
> 
> I think, this patch can be folded into the first one.

I don't see which patch it would be folded into.  It's a distinct change
from the change to the flush implementation. 

> Otherwise:
> 
> Ack-by: Christoph Egger <Christoph.Egger@amd.com>

Thank you.  I'll apply them all this afternoon.

Tim.

> On 06/27/11 12:46, Tim Deegan wrote:
> ># HG changeset patch
> ># User Tim Deegan<Tim.Deegan@citrix.com>
> ># Date 1308929084 -3600
> ># Node ID 0753351afbbe1c3fdde3a72dfb5a67105524f813
> ># Parent  97e15368260c093078e1f1bc04521de30c1792cc
> >Nested p2m: flush only one p2m table when reallocating.
> >It's unhelpful to flush all of them when we only need one.
> >
> >Reported-by: Christoph Egger<Christoph.Egger@amd.com>
> >Signed-off-by: Tim Deegan<Tim.Deegan@citrix.com>
> >
> >diff -r 97e15368260c -r 0753351afbbe xen/arch/x86/mm/p2m.c
> >--- a/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> >+++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> >@@ -1120,7 +1120,6 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
> >      volatile struct nestedvcpu *nv =&vcpu_nestedhvm(v);
> >      struct domain *d;
> >      struct p2m_domain *p2m;
> >-    int i;
> >
> >      /* Mask out low bits; this avoids collisions with CR3_EADDR */
> >      cr3&= ~(0xfffull);
> >@@ -1146,12 +1145,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64
> >      }
> >
> >      /* 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);
> >-        p2m_flush_locked(p2m);
> >-    }
> >+     * flush it and reuse. */
> >+    p2m = p2m_getlru_nestedp2m(d, NULL);
> >+    p2m_flush_locked(p2m);
> >      nv->nv_p2m = p2m;
> >      p2m->cr3 = cr3;
> >      nv->nv_flushp2m = 0;
> >
> 
> 
> -- 
> ---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@lists.xensource.com
> http://lists.xensource.com/xen-devel

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Tim Deegan]

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

At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> > >  - Why is there a 10x increase in IPIs after this series?  I don't see
> > >    what sequence of events sets the relevant cpumask bits to make this
> > >    happen.
> > 
> > In patch 1 the code that sends the IPIs was outside of the loop and
> > moved into the loop.
> 
> Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
> are burning through np2m tables very quickly indeed.  Maybe removing the
> extra flushes for TLB control will do the trick.  I'll make a patch...

I think I get it - it's a race between p2m_flush_nestedp2m() on one CPU
flushing all the nested P2M tables and a VCPU on another CPU repeatedly
getting fresh ones.  Try the attached patch, which should cut back the
major source of p2m_flush_nestedp2m() calls. 

Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
isn't safe because it can run in parallel with p2m_get_nestedp2m, which
reorders the array it walks.  I'll have to make the LRU-fu independent
of the array order; should be easy enough but I'll hold off committing
the current series until I've done it. 

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

[-- Attachment #2: np2m-range-update --]
[-- Type: text/plain, Size: 5459 bytes --]

x86/p2m: Add p2m_change_type_range() operation 
that defers the nested-p2m flush until the entire batch has been updated.
Use it in the HAP log-dirty operations for tracking VRAM changes.
This should avoid a lot of unpleasant IPI storms as the log-dirty code 
on one CPU repeatedly shoots down the nested p2m of another CPU. 

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 6b6a348ed670 -r 9a8fc3f73ff3 xen/arch/x86/mm/hap/hap.c
--- a/xen/arch/x86/mm/hap/hap.c	Mon Jun 27 15:03:06 2011 +0100
+++ b/xen/arch/x86/mm/hap/hap.c	Mon Jun 27 16:29:18 2011 +0100
@@ -58,7 +58,6 @@
 
 static int hap_enable_vram_tracking(struct domain *d)
 {
-    int i;
     struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
 
     if ( !dirty_vram )
@@ -70,8 +69,8 @@ static int hap_enable_vram_tracking(stru
     paging_unlock(d);
 
     /* set l1e entries of P2M table to be read-only. */
-    for (i = dirty_vram->begin_pfn; i < dirty_vram->end_pfn; i++)
-        p2m_change_type(d, i, p2m_ram_rw, p2m_ram_logdirty);
+    p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
+                          p2m_ram_rw, p2m_ram_logdirty);
 
     flush_tlb_mask(d->domain_dirty_cpumask);
     return 0;
@@ -79,7 +78,6 @@ static int hap_enable_vram_tracking(stru
 
 static int hap_disable_vram_tracking(struct domain *d)
 {
-    int i;
     struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
 
     if ( !dirty_vram )
@@ -90,8 +88,8 @@ static int hap_disable_vram_tracking(str
     paging_unlock(d);
 
     /* set l1e entries of P2M table with normal mode */
-    for (i = dirty_vram->begin_pfn; i < dirty_vram->end_pfn; i++)
-        p2m_change_type(d, i, p2m_ram_logdirty, p2m_ram_rw);
+    p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
+                          p2m_ram_logdirty, p2m_ram_rw);
 
     flush_tlb_mask(d->domain_dirty_cpumask);
     return 0;
@@ -99,15 +97,14 @@ static int hap_disable_vram_tracking(str
 
 static void hap_clean_vram_tracking(struct domain *d)
 {
-    int i;
     struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram;
 
     if ( !dirty_vram )
         return;
 
     /* set l1e entries of P2M table to be read-only. */
-    for (i = dirty_vram->begin_pfn; i < dirty_vram->end_pfn; i++)
-        p2m_change_type(d, i, p2m_ram_rw, p2m_ram_logdirty);
+    p2m_change_type_range(d, dirty_vram->begin_pfn, dirty_vram->end_pfn, 
+                          p2m_ram_rw, p2m_ram_logdirty);
 
     flush_tlb_mask(d->domain_dirty_cpumask);
 }
@@ -863,7 +860,8 @@ hap_write_p2m_entry(struct vcpu *v, unsi
     paging_lock(d);
     old_flags = l1e_get_flags(*p);
 
-    if ( nestedhvm_enabled(d) && (old_flags & _PAGE_PRESENT) ) {
+    if ( nestedhvm_enabled(d) && (old_flags & _PAGE_PRESENT) 
+         && !p2m_get_hostp2m(d)->defer_nested_flush ) {
         /* We are replacing a valid entry so we need to flush nested p2ms,
          * unless the only change is an increase in access rights. */
         mfn_t omfn = _mfn(l1e_get_pfn(*p));
diff -r 6b6a348ed670 -r 9a8fc3f73ff3 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Mon Jun 27 15:03:06 2011 +0100
+++ b/xen/arch/x86/mm/p2m.c	Mon Jun 27 16:29:18 2011 +0100
@@ -537,6 +537,37 @@ p2m_type_t p2m_change_type(struct domain
     return pt;
 }
 
+/* Modify the p2m type of a range of gfns from ot to nt.
+ * Resets the access permissions. */
+void p2m_change_type_range(struct domain *d, 
+                           unsigned long start, unsigned long end,
+                           p2m_type_t ot, p2m_type_t nt)
+{
+    p2m_type_t pt;
+    unsigned long gfn;
+    mfn_t mfn;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    BUG_ON(p2m_is_grant(ot) || p2m_is_grant(nt));
+
+    p2m_lock(p2m);
+    p2m->defer_nested_flush = 1;
+
+    for ( gfn = start; gfn < end; gfn++ )
+    {
+        mfn = gfn_to_mfn_query(d, gfn, &pt);
+        if ( pt == ot )
+            set_p2m_entry(p2m, gfn, mfn, 0, nt, p2m->default_access);
+    }
+
+    p2m->defer_nested_flush = 0;
+    if ( nestedhvm_enabled(d) )
+        p2m_flush_nestedp2m(d);
+    p2m_unlock(p2m);
+}
+
+
+
 int
 set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
 {
diff -r 6b6a348ed670 -r 9a8fc3f73ff3 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h	Mon Jun 27 15:03:06 2011 +0100
+++ b/xen/include/asm-x86/p2m.h	Mon Jun 27 16:29:18 2011 +0100
@@ -209,6 +209,12 @@ struct p2m_domain {
 #define CR3_EADDR     (~0ULL)
     uint64_t           cr3;
 
+    /* Host p2m: when this flag is set, don't flush all the nested-p2m 
+     * tables on every host-p2m change.  The setter of this flag 
+     * is responsible for performing the full flush before releasing the
+     * host p2m's lock. */
+    int                defer_nested_flush;
+
     /* Pages used to construct the p2m */
     struct page_list_head pages;
 
@@ -408,6 +414,11 @@ int guest_physmap_mark_populate_on_deman
 void p2m_change_entry_type_global(struct domain *d, 
                                   p2m_type_t ot, p2m_type_t nt);
 
+/* Change types across a range of p2m entries (start ... end-1) */
+void p2m_change_type_range(struct domain *d, 
+                           unsigned long start, unsigned long end,
+                           p2m_type_t ot, p2m_type_t nt);
+
 /* Compare-exchange the type of a single p2m entry */
 p2m_type_t p2m_change_type(struct domain *d, unsigned long gfn,
                            p2m_type_t ot, p2m_type_t nt);

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

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

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Christoph Egger]

On 06/27/11 17:48, Tim Deegan wrote:
> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>   - Why is there a 10x increase in IPIs after this series?  I don't see
>>>>     what sequence of events sets the relevant cpumask bits to make this
>>>>     happen.
>>>
>>> In patch 1 the code that sends the IPIs was outside of the loop and
>>> moved into the loop.
>>
>> Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
>> are burning through np2m tables very quickly indeed.  Maybe removing the
>> extra flushes for TLB control will do the trick.  I'll make a patch...
>
> I think I get it - it's a race between p2m_flush_nestedp2m() on one CPU
> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
> getting fresh ones.  Try the attached patch, which should cut back the
> major source of p2m_flush_nestedp2m() calls.

This patch is great! It gives the speed up the XP mode needs to continue 
booting. We talked about this at the Xen-Hackathon you remember?

> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
> isn't safe because it can run in parallel with p2m_get_nestedp2m, which
> reorders the array it walks.  I'll have to make the LRU-fu independent
> of the array order; should be easy enough but I'll hold off committing
> the current series until I've done it.

Ok.

Christoph


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

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Christoph Egger]

On 06/27/11 17:48, Tim Deegan wrote:
> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>   - Why is there a 10x increase in IPIs after this series?  I don't see
>>>>     what sequence of events sets the relevant cpumask bits to make this
>>>>     happen.
>>>
>>> In patch 1 the code that sends the IPIs was outside of the loop and
>>> moved into the loop.
>>
>> Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
>> are burning through np2m tables very quickly indeed.  Maybe removing the
>> extra flushes for TLB control will do the trick.  I'll make a patch...
>
> I think I get it - it's a race between p2m_flush_nestedp2m() on one CPU
> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
> getting fresh ones.  Try the attached patch, which should cut back the
> major source of p2m_flush_nestedp2m() calls.
>
> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
> isn't safe because it can run in parallel with p2m_get_nestedp2m, which
> reorders the array it walks.  I'll have to make the LRU-fu independent
> of the array order; should be easy enough but I'll hold off committing
> the current series until I've done it.


With Windows 7 XP mode I see a xen crash where p2m_get_nestedp2m() calls
nestedhvm_vmcx_flushtlb(nv->nv_p2m) with nv->nv_p2m being NULL.

nestedhvm_vmcx_flushtlb() assumes the parameter is not NULL.

(XEN) Xen call trace:
(XEN)    [<ffff82c4801b1d19>] nestedhvm_vmcx_flushtlb+0xf/0x52
(XEN)    [<ffff82c4801d270c>] p2m_get_nestedp2m+0x406/0x497
(XEN)    [<ffff82c4801bad7a>] nestedsvm_vmcb_set_nestedp2m+0x54/0x6a
(XEN)    [<ffff82c4801bc602>] nsvm_vcpu_vmrun+0x984/0xf14
(XEN)    [<ffff82c4801bcd53>] nsvm_vcpu_switch+0x1c1/0x226

Christoph


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

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Tim Deegan]

At 16:48 +0100 on 27 Jun (1309193311), Tim Deegan wrote:
> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
> > At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
> > > >  - Why is there a 10x increase in IPIs after this series?  I don't see
> > > >    what sequence of events sets the relevant cpumask bits to make this
> > > >    happen.
> > > 
> > > In patch 1 the code that sends the IPIs was outside of the loop and
> > > moved into the loop.
> > 
> > Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
> > are burning through np2m tables very quickly indeed.  Maybe removing the
> > extra flushes for TLB control will do the trick.  I'll make a patch...
> 
> I think I get it - it's a race between p2m_flush_nestedp2m() on one CPU
> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
> getting fresh ones.  Try the attached patch, which should cut back the
> major source of p2m_flush_nestedp2m() calls. 
> 
> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
> isn't safe because it can run in parallel with p2m_get_nestedp2m, which
> reorders the array it walks.  I'll have to make the LRU-fu independent
> of the array order; should be easy enough but I'll hold off committing
> the current series until I've done it. 

I've just pushed 23633 - 26369, which is this series plus the change to
the LRU code (and a fix to the NULL deref you reported is folded in).
Hopefully that puts nested SVM back in at least as good a state as it
was before my locking-order patch broke it! :)

Cheers,

Tim

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

[Olaf Hering]

On Mon, Jun 27, Tim Deegan wrote:

> diff -r b24018319772 -r 44c7b9773026 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c	Thu Jun 23 18:34:55 2011 +0100
> +++ b/xen/arch/x86/mm/p2m.c	Fri Jun 24 16:24:44 2011 +0100
> @@ -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;

Tim,

one of these changes causes a build error:
p2m.c:1105:20: error: unused variable 'd' [-Werror=unused-variable]

Olaf

Re: [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action

[Tim Deegan]

At 11:51 +0200 on 30 Jun (1309434665), Olaf Hering wrote:
> one of these changes causes a build error:
> p2m.c:1105:20: error: unused variable 'd' [-Werror=unused-variable]

My apologies; I left a variable that's only used in an ASSERT() and
didn't check the non-debug build.   Fixed in 23640:4a1f7441095d.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes

[Christoph Egger]

On 06/30/11 11:49, Tim Deegan wrote:
> At 16:48 +0100 on 27 Jun (1309193311), Tim Deegan wrote:
>> At 14:15 +0100 on 27 Jun (1309184128), Tim Deegan wrote:
>>> At 14:23 +0200 on 27 Jun (1309184586), Christoph Egger wrote:
>>>>>   - Why is there a 10x increase in IPIs after this series?  I don't see
>>>>>     what sequence of events sets the relevant cpumask bits to make this
>>>>>     happen.
>>>>
>>>> In patch 1 the code that sends the IPIs was outside of the loop and
>>>> moved into the loop.
>>>
>>> Well, yes, but I don't see what that causes 10x IPIs, unless the vcpus
>>> are burning through np2m tables very quickly indeed.  Maybe removing the
>>> extra flushes for TLB control will do the trick.  I'll make a patch...
>>
>> I think I get it - it's a race between p2m_flush_nestedp2m() on one CPU
>> flushing all the nested P2M tables and a VCPU on another CPU repeatedly
>> getting fresh ones.  Try the attached patch, which should cut back the
>> major source of p2m_flush_nestedp2m() calls.
>>
>> Writing it, I realised that after my locking fix, p2m_flush_nestedp2m()
>> isn't safe because it can run in parallel with p2m_get_nestedp2m, which
>> reorders the array it walks.  I'll have to make the LRU-fu independent
>> of the array order; should be easy enough but I'll hold off committing
>> the current series until I've done it.
>
> I've just pushed 23633 - 26369, which is this series plus the change to
> the LRU code (and a fix to the NULL deref you reported is folded in).
> Hopefully that puts nested SVM back in at least as good a state as it
> was before my locking-order patch broke it! :)

I run some tests and nested SVM is now in an even better state as
it was before. Performance is a lot better now, particularly MMIO 
performance. The e1000 driver needed several minutes to read the
e1000 mac address before and now it takes less than a second.

Christoph


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