* [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes @ 2011-06-27 10:46 Tim Deegan 2011-06-27 10:46 ` [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action Tim Deegan ` (5 more replies) 0 siblings, 6 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 10:46 UTC (permalink / raw) To: xen-devel; +Cc: Christoph Egger 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action 2011-06-27 10:46 [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan @ 2011-06-27 10:46 ` Tim Deegan 2011-06-30 9:51 ` Olaf Hering 2011-06-27 10:46 ` [PATCH 2 of 5] Nested p2m: remove bogus check of CR3 value Tim Deegan ` (4 subsequent siblings) 5 siblings, 1 reply; 21+ messages in thread From: Tim Deegan @ 2011-06-27 10:46 UTC (permalink / raw) To: xen-devel; +Cc: Christoph Egger # 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); ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action 2011-06-27 10:46 ` [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action Tim Deegan @ 2011-06-30 9:51 ` Olaf Hering 2011-06-30 10:02 ` Tim Deegan 0 siblings, 1 reply; 21+ messages in thread From: Olaf Hering @ 2011-06-30 9:51 UTC (permalink / raw) To: Tim Deegan; +Cc: Christoph Egger, xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action 2011-06-30 9:51 ` Olaf Hering @ 2011-06-30 10:02 ` Tim Deegan 0 siblings, 0 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-30 10:02 UTC (permalink / raw) To: Olaf Hering; +Cc: Christoph Egger, xen-devel 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2 of 5] Nested p2m: remove bogus check of CR3 value 2011-06-27 10:46 [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan 2011-06-27 10:46 ` [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action Tim Deegan @ 2011-06-27 10:46 ` Tim Deegan 2011-06-27 10:46 ` [PATCH 3 of 5] Nested p2m: clarify logic in p2m_get_nestedp2m() Tim Deegan ` (3 subsequent siblings) 5 siblings, 0 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 10:46 UTC (permalink / raw) To: xen-devel; +Cc: Christoph Egger # 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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3 of 5] Nested p2m: clarify logic in p2m_get_nestedp2m() 2011-06-27 10:46 [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan 2011-06-27 10:46 ` [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action Tim Deegan 2011-06-27 10:46 ` [PATCH 2 of 5] Nested p2m: remove bogus check of CR3 value Tim Deegan @ 2011-06-27 10:46 ` Tim Deegan 2011-06-27 10:46 ` [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating Tim Deegan ` (2 subsequent siblings) 5 siblings, 0 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 10:46 UTC (permalink / raw) To: xen-devel; +Cc: Christoph Egger # 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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating 2011-06-27 10:46 [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan ` (2 preceding siblings ...) 2011-06-27 10:46 ` [PATCH 3 of 5] Nested p2m: clarify logic in p2m_get_nestedp2m() Tim Deegan @ 2011-06-27 10:46 ` Tim Deegan 2011-06-27 13:44 ` Christoph Egger 2011-06-27 10:46 ` [PATCH 5 of 5] Nested p2m: rework locking around nested-p2m flushes and updates Tim Deegan 2011-06-27 10:56 ` [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan 5 siblings, 1 reply; 21+ messages in thread From: Tim Deegan @ 2011-06-27 10:46 UTC (permalink / raw) To: xen-devel; +Cc: Christoph Egger # 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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating 2011-06-27 10:46 ` [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating Tim Deegan @ 2011-06-27 13:44 ` Christoph Egger 2011-06-27 14:01 ` Tim Deegan 0 siblings, 1 reply; 21+ messages in thread From: Christoph Egger @ 2011-06-27 13:44 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Re: [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating 2011-06-27 13:44 ` Christoph Egger @ 2011-06-27 14:01 ` Tim Deegan 0 siblings, 0 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 14:01 UTC (permalink / raw) To: Christoph Egger; +Cc: xen-devel 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5 of 5] Nested p2m: rework locking around nested-p2m flushes and updates 2011-06-27 10:46 [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan ` (3 preceding siblings ...) 2011-06-27 10:46 ` [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating Tim Deegan @ 2011-06-27 10:46 ` Tim Deegan 2011-06-27 10:56 ` [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan 5 siblings, 0 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 10:46 UTC (permalink / raw) To: xen-devel; +Cc: Christoph Egger # 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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 10:46 [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan ` (4 preceding siblings ...) 2011-06-27 10:46 ` [PATCH 5 of 5] Nested p2m: rework locking around nested-p2m flushes and updates Tim Deegan @ 2011-06-27 10:56 ` Tim Deegan 2011-06-27 12:23 ` Christoph Egger 5 siblings, 1 reply; 21+ messages in thread From: Tim Deegan @ 2011-06-27 10:56 UTC (permalink / raw) To: xen-devel; +Cc: Christoph Egger 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 10:56 ` [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan @ 2011-06-27 12:23 ` Christoph Egger 2011-06-27 13:15 ` Tim Deegan 0 siblings, 1 reply; 21+ messages in thread From: Christoph Egger @ 2011-06-27 12:23 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 12:23 ` Christoph Egger @ 2011-06-27 13:15 ` Tim Deegan 2011-06-27 13:20 ` Tim Deegan 2011-06-27 15:48 ` Tim Deegan 0 siblings, 2 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 13:15 UTC (permalink / raw) To: Christoph Egger; +Cc: xen-devel 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 13:15 ` Tim Deegan @ 2011-06-27 13:20 ` Tim Deegan 2011-06-27 13:24 ` Christoph Egger 2011-06-27 15:48 ` Tim Deegan 1 sibling, 1 reply; 21+ messages in thread From: Tim Deegan @ 2011-06-27 13:20 UTC (permalink / raw) To: Christoph Egger; +Cc: xen-devel 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 13:20 ` Tim Deegan @ 2011-06-27 13:24 ` Christoph Egger 2011-06-27 13:55 ` Tim Deegan 0 siblings, 1 reply; 21+ messages in thread From: Christoph Egger @ 2011-06-27 13:24 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 13:24 ` Christoph Egger @ 2011-06-27 13:55 ` Tim Deegan 0 siblings, 0 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 13:55 UTC (permalink / raw) To: Christoph Egger; +Cc: xen-devel 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 13:15 ` Tim Deegan 2011-06-27 13:20 ` Tim Deegan @ 2011-06-27 15:48 ` Tim Deegan 2011-06-28 11:04 ` Christoph Egger ` (2 more replies) 1 sibling, 3 replies; 21+ messages in thread From: Tim Deegan @ 2011-06-27 15:48 UTC (permalink / raw) To: Christoph Egger; +Cc: xen-devel [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 15:48 ` Tim Deegan @ 2011-06-28 11:04 ` Christoph Egger 2011-06-28 13:47 ` Christoph Egger 2011-06-30 9:49 ` Tim Deegan 2 siblings, 0 replies; 21+ messages in thread From: Christoph Egger @ 2011-06-28 11:04 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 15:48 ` Tim Deegan 2011-06-28 11:04 ` Christoph Egger @ 2011-06-28 13:47 ` Christoph Egger 2011-06-30 9:49 ` Tim Deegan 2 siblings, 0 replies; 21+ messages in thread From: Christoph Egger @ 2011-06-28 13:47 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-27 15:48 ` Tim Deegan 2011-06-28 11:04 ` Christoph Egger 2011-06-28 13:47 ` Christoph Egger @ 2011-06-30 9:49 ` Tim Deegan 2011-07-01 10:00 ` Christoph Egger 2 siblings, 1 reply; 21+ messages in thread From: Tim Deegan @ 2011-06-30 9:49 UTC (permalink / raw) To: Christoph Egger; +Cc: xen-devel 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes 2011-06-30 9:49 ` Tim Deegan @ 2011-07-01 10:00 ` Christoph Egger 0 siblings, 0 replies; 21+ messages in thread From: Christoph Egger @ 2011-07-01 10:00 UTC (permalink / raw) To: Tim Deegan; +Cc: xen-devel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2011-07-01 10:00 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-27 10:46 [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan 2011-06-27 10:46 ` [PATCH 1 of 5] Nested p2m: implement "flush" as a first-class action Tim Deegan 2011-06-30 9:51 ` Olaf Hering 2011-06-30 10:02 ` Tim Deegan 2011-06-27 10:46 ` [PATCH 2 of 5] Nested p2m: remove bogus check of CR3 value Tim Deegan 2011-06-27 10:46 ` [PATCH 3 of 5] Nested p2m: clarify logic in p2m_get_nestedp2m() Tim Deegan 2011-06-27 10:46 ` [PATCH 4 of 5] Nested p2m: flush only one p2m table when reallocating Tim Deegan 2011-06-27 13:44 ` Christoph Egger 2011-06-27 14:01 ` Tim Deegan 2011-06-27 10:46 ` [PATCH 5 of 5] Nested p2m: rework locking around nested-p2m flushes and updates Tim Deegan 2011-06-27 10:56 ` [PATCH 0 of 5] v2: Nested-p2m cleanups and locking changes Tim Deegan 2011-06-27 12:23 ` Christoph Egger 2011-06-27 13:15 ` Tim Deegan 2011-06-27 13:20 ` Tim Deegan 2011-06-27 13:24 ` Christoph Egger 2011-06-27 13:55 ` Tim Deegan 2011-06-27 15:48 ` Tim Deegan 2011-06-28 11:04 ` Christoph Egger 2011-06-28 13:47 ` Christoph Egger 2011-06-30 9:49 ` Tim Deegan 2011-07-01 10:00 ` Christoph Egger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.