| Message ID | 20170718103429.25020-3-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18.07.17 12:34, Sergey Dyasli wrote: > The new function finds all np2m objects with the specified eptp and > flushes them. p2m_flush_table_locked() is added in order not to release > the p2m lock after np2m_base check. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > --- > xen/arch/x86/mm/p2m.c | 34 +++++++++++++++++++++++++++++++--- > xen/include/asm-x86/p2m.h | 2 ++ > 2 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index b8c8bba421..bc330d8f52 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1708,15 +1708,14 @@ p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m) > return p2m; > } > > -/* Reset this p2m table to be empty */ > static void > -p2m_flush_table(struct p2m_domain *p2m) > +p2m_flush_table_locked(struct p2m_domain *p2m) > { > struct page_info *top, *pg; > struct domain *d = p2m->domain; > mfn_t mfn; > > - p2m_lock(p2m); > + ASSERT(p2m_locked_by_me(p2m)); > > /* > * "Host" p2m tables can have shared entries &c that need a bit more care > @@ -1756,6 +1755,14 @@ p2m_flush_table(struct p2m_domain *p2m) > p2m_unlock(p2m); > } > > +/* Reset this p2m table to be empty */ > +static void > +p2m_flush_table(struct p2m_domain *p2m) > +{ > + p2m_lock(p2m); > + p2m_flush_table_locked(p2m); > +} > + > void > p2m_flush(struct vcpu *v, struct p2m_domain *p2m) > { > @@ -1773,6 +1780,27 @@ p2m_flush_nestedp2m(struct domain *d) > p2m_flush_table(d->arch.nested_p2m[i]); > } > > +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp) > +{ > + struct domain *d = v->domain; > + struct p2m_domain *p2m; > + unsigned int i; > + > + eptp &= ~(0xfffull); > + > + nestedp2m_lock(d); > + for ( i = 0; i < MAX_NESTEDP2M; i++ ) > + { > + p2m = d->arch.nested_p2m[i]; > + p2m_lock(p2m); > + if ( p2m->np2m_base == eptp ) > + p2m_flush_table_locked(p2m); > + else > + p2m_unlock(p2m); > + } > + nestedp2m_unlock(d); > +} > + What exactly is eptp specific in this function ? Christoph > static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m) > { > struct nestedvcpu *nv = &vcpu_nestedhvm(v); > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > index 9086bb35dc..0e39999387 100644 > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -779,6 +779,8 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa); > void p2m_flush(struct vcpu *v, struct p2m_domain *p2m); > /* Flushes all nested p2m tables */ > void p2m_flush_nestedp2m(struct domain *d); > +/* Flushes all np2m objects with the specified eptp */ > +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp); > > void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, > l1_pgentry_t *p, l1_pgentry_t new, unsigned int level); > Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
On Tue, 2017-08-01 at 09:55 +0200, Egger, Christoph wrote: > On 18.07.17 12:34, Sergey Dyasli wrote: > > The new function finds all np2m objects with the specified eptp and > > flushes them. p2m_flush_table_locked() is added in order not to release > > the p2m lock after np2m_base check. > > > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > > --- > > xen/arch/x86/mm/p2m.c | 34 +++++++++++++++++++++++++++++++--- > > xen/include/asm-x86/p2m.h | 2 ++ > > 2 files changed, 33 insertions(+), 3 deletions(-) > > > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > > index b8c8bba421..bc330d8f52 100644 > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -1708,15 +1708,14 @@ p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m) > > return p2m; > > } > > > > -/* Reset this p2m table to be empty */ > > static void > > -p2m_flush_table(struct p2m_domain *p2m) > > +p2m_flush_table_locked(struct p2m_domain *p2m) > > { > > struct page_info *top, *pg; > > struct domain *d = p2m->domain; > > mfn_t mfn; > > > > - p2m_lock(p2m); > > + ASSERT(p2m_locked_by_me(p2m)); > > > > /* > > * "Host" p2m tables can have shared entries &c that need a bit more care > > @@ -1756,6 +1755,14 @@ p2m_flush_table(struct p2m_domain *p2m) > > p2m_unlock(p2m); > > } > > > > +/* Reset this p2m table to be empty */ > > +static void > > +p2m_flush_table(struct p2m_domain *p2m) > > +{ > > + p2m_lock(p2m); > > + p2m_flush_table_locked(p2m); > > +} > > + > > void > > p2m_flush(struct vcpu *v, struct p2m_domain *p2m) > > { > > @@ -1773,6 +1780,27 @@ p2m_flush_nestedp2m(struct domain *d) > > p2m_flush_table(d->arch.nested_p2m[i]); > > } > > > > +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp) > > +{ > > + struct domain *d = v->domain; > > + struct p2m_domain *p2m; > > + unsigned int i; > > + > > + eptp &= ~(0xfffull); > > + > > + nestedp2m_lock(d); > > + for ( i = 0; i < MAX_NESTEDP2M; i++ ) > > + { > > + p2m = d->arch.nested_p2m[i]; > > + p2m_lock(p2m); > > + if ( p2m->np2m_base == eptp ) > > + p2m_flush_table_locked(p2m); > > + else > > + p2m_unlock(p2m); > > + } > > + nestedp2m_unlock(d); > > +} > > + > > What exactly is eptp specific in this function ? Yes, good point. I seem to be too focused on Intel. The correct parameter name should be np2m_base, of course. > > static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m) > > { > > struct nestedvcpu *nv = &vcpu_nestedhvm(v); > > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h > > index 9086bb35dc..0e39999387 100644 > > --- a/xen/include/asm-x86/p2m.h > > +++ b/xen/include/asm-x86/p2m.h > > @@ -779,6 +779,8 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa); > > void p2m_flush(struct vcpu *v, struct p2m_domain *p2m); > > /* Flushes all nested p2m tables */ > > void p2m_flush_nestedp2m(struct domain *d); > > +/* Flushes all np2m objects with the specified eptp */ > > +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp); > > > > void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, > > l1_pgentry_t *p, l1_pgentry_t new, unsigned int level); > > > > Amazon Development Center Germany GmbH > Berlin - Dresden - Aachen > main office: Krausenstr. 38, 10117 Berlin > Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger > Ust-ID: DE289237879 > Eingetragen am Amtsgericht Charlottenburg HRB 149173 B -- Thanks, Sergey
>>> Sergey Dyasli <sergey.dyasli@citrix.com> 08/03/17 4:20 PM >>> >On Tue, 2017-08-01 at 09:55 +0200, Egger, Christoph wrote: >> On 18.07.17 12:34, Sergey Dyasli wrote: >> > @@ -1773,6 +1780,27 @@ p2m_flush_nestedp2m(struct domain *d) >> > p2m_flush_table(d->arch.nested_p2m[i]); >> > } >> > >> > +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp) >> > +{ >> > + struct domain *d = v->domain; >> > + struct p2m_domain *p2m; >> > + unsigned int i; >> > + >> > + eptp &= ~(0xfffull); >> > + >> > + nestedp2m_lock(d); >> > + for ( i = 0; i < MAX_NESTEDP2M; i++ ) >> > + { >> > + p2m = d->arch.nested_p2m[i]; >> > + p2m_lock(p2m); >> > + if ( p2m->np2m_base == eptp ) >> > + p2m_flush_table_locked(p2m); >> > + else >> > + p2m_unlock(p2m); >> > + } >> > + nestedp2m_unlock(d); >> > +} >> > + >> >> What exactly is eptp specific in this function ? > >Yes, good point. I seem to be too focused on Intel. The correct parameter >name should be np2m_base, of course. And (at the risk of stating the obvious) the function name shouldn't include "ept" then either. Jan
On 07/18/2017 11:34 AM, Sergey Dyasli wrote: > The new function finds all np2m objects with the specified eptp and > flushes them. p2m_flush_table_locked() is added in order not to release > the p2m lock after np2m_base check. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> This patch looks plausible except for... > --- > xen/arch/x86/mm/p2m.c | 34 +++++++++++++++++++++++++++++++--- > xen/include/asm-x86/p2m.h | 2 ++ > 2 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index b8c8bba421..bc330d8f52 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1708,15 +1708,14 @@ p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m) > return p2m; > } > > -/* Reset this p2m table to be empty */ > static void > -p2m_flush_table(struct p2m_domain *p2m) > +p2m_flush_table_locked(struct p2m_domain *p2m) > { > struct page_info *top, *pg; > struct domain *d = p2m->domain; > mfn_t mfn; > > - p2m_lock(p2m); > + ASSERT(p2m_locked_by_me(p2m)); > > /* > * "Host" p2m tables can have shared entries &c that need a bit more care > @@ -1756,6 +1755,14 @@ p2m_flush_table(struct p2m_domain *p2m) > p2m_unlock(p2m); ...this. Why on earth would we unlock this at the end of the function, instead of letting the caller do it? If we were to do that, at very least it should be called "p2m_flush_and_unlock_table()". But I think we just want to leave the unlock out, because... > } > > +/* Reset this p2m table to be empty */ > +static void > +p2m_flush_table(struct p2m_domain *p2m) > +{ > + p2m_lock(p2m); > + p2m_flush_table_locked(p2m); ..this looks wrong -- a balanced lock/unlock is easier to verify, and... > +} > + > void > p2m_flush(struct vcpu *v, struct p2m_domain *p2m) > { > @@ -1773,6 +1780,27 @@ p2m_flush_nestedp2m(struct domain *d) > p2m_flush_table(d->arch.nested_p2m[i]); > } > > +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp) > +{ > + struct domain *d = v->domain; > + struct p2m_domain *p2m; > + unsigned int i; > + > + eptp &= ~(0xfffull); > + > + nestedp2m_lock(d); > + for ( i = 0; i < MAX_NESTEDP2M; i++ ) > + { > + p2m = d->arch.nested_p2m[i]; > + p2m_lock(p2m); > + if ( p2m->np2m_base == eptp ) > + p2m_flush_table_locked(p2m); > + else > + p2m_unlock(p2m); ...here we have essentially a pointless 'else'. Lock/unlock around the whole conditional would make much more sense. -George
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index b8c8bba421..bc330d8f52 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1708,15 +1708,14 @@ p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m) return p2m; } -/* Reset this p2m table to be empty */ static void -p2m_flush_table(struct p2m_domain *p2m) +p2m_flush_table_locked(struct p2m_domain *p2m) { struct page_info *top, *pg; struct domain *d = p2m->domain; mfn_t mfn; - p2m_lock(p2m); + ASSERT(p2m_locked_by_me(p2m)); /* * "Host" p2m tables can have shared entries &c that need a bit more care @@ -1756,6 +1755,14 @@ p2m_flush_table(struct p2m_domain *p2m) p2m_unlock(p2m); } +/* Reset this p2m table to be empty */ +static void +p2m_flush_table(struct p2m_domain *p2m) +{ + p2m_lock(p2m); + p2m_flush_table_locked(p2m); +} + void p2m_flush(struct vcpu *v, struct p2m_domain *p2m) { @@ -1773,6 +1780,27 @@ p2m_flush_nestedp2m(struct domain *d) p2m_flush_table(d->arch.nested_p2m[i]); } +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp) +{ + struct domain *d = v->domain; + struct p2m_domain *p2m; + unsigned int i; + + eptp &= ~(0xfffull); + + nestedp2m_lock(d); + for ( i = 0; i < MAX_NESTEDP2M; i++ ) + { + p2m = d->arch.nested_p2m[i]; + p2m_lock(p2m); + if ( p2m->np2m_base == eptp ) + p2m_flush_table_locked(p2m); + else + p2m_unlock(p2m); + } + nestedp2m_unlock(d); +} + static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m) { struct nestedvcpu *nv = &vcpu_nestedhvm(v); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 9086bb35dc..0e39999387 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -779,6 +779,8 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa); void p2m_flush(struct vcpu *v, struct p2m_domain *p2m); /* Flushes all nested p2m tables */ void p2m_flush_nestedp2m(struct domain *d); +/* Flushes all np2m objects with the specified eptp */ +void np2m_flush_eptp(struct vcpu *v, unsigned long eptp); void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, l1_pgentry_t *p, l1_pgentry_t new, unsigned int level);
The new function finds all np2m objects with the specified eptp and flushes them. p2m_flush_table_locked() is added in order not to release the p2m lock after np2m_base check. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- xen/arch/x86/mm/p2m.c | 34 +++++++++++++++++++++++++++++++--- xen/include/asm-x86/p2m.h | 2 ++ 2 files changed, 33 insertions(+), 3 deletions(-)