| Message ID | 20170929150144.7602-2-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-09-29 at 16:01 +0100, George Dunlap wrote: > nvmx_handle_invept() updates current's np2m just to flush it. This is > not only wasteful, but ineffective: if several L2 vcpus share the same > np2m base pointer, they all need to be flushed (not only the current > one). I don't follow this completely. L1 will use INVEPT on each vCPU that shares the same np2m pointer. The main idea here was not to update current's np2m just to flush it. > > Introduce a new function, np2m_flush_base() which will flush all > shadow p2m's that match a given base pointer. > > Convert p2m_flush_table() into p2m_flush_table_locked() in order not > to release the p2m_lock after np2m_base check. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > Changes since v1: > - Combine patches 2 and 3 ("x86/np2m: add np2m_flush_base()" and > "x86/vvmx: use np2m_flush_base() for INVEPT_SINGLE_CONTEXT") > - Reword commit text > -- Thanks, Sergey
On 10/02/2017 10:37 AM, Sergey Dyasli wrote: > On Fri, 2017-09-29 at 16:01 +0100, George Dunlap wrote: >> nvmx_handle_invept() updates current's np2m just to flush it. This is >> not only wasteful, but ineffective: if several L2 vcpus share the same >> np2m base pointer, they all need to be flushed (not only the current >> one). > > I don't follow this completely. L1 will use INVEPT on each vCPU that > shares the same np2m pointer. The main idea here was not to update > current's np2m just to flush it. Hmm, yes the INVEPT thing is true. But if that's the case, why do we need np2m_flush_base() to loop over the whole list and flush all np2ms with the same pointer? -George
On 10/02/2017 10:40 AM, George Dunlap wrote: > On 10/02/2017 10:37 AM, Sergey Dyasli wrote: >> On Fri, 2017-09-29 at 16:01 +0100, George Dunlap wrote: >>> nvmx_handle_invept() updates current's np2m just to flush it. This is >>> not only wasteful, but ineffective: if several L2 vcpus share the same >>> np2m base pointer, they all need to be flushed (not only the current >>> one). >> >> I don't follow this completely. L1 will use INVEPT on each vCPU that >> shares the same np2m pointer. The main idea here was not to update >> current's np2m just to flush it. > > Hmm, yes the INVEPT thing is true. But if that's the case, why do we > need np2m_flush_base() to loop over the whole list and flush all np2ms > with the same pointer? Oh, nevermind -- you don't know which np2m is being used by this vcpu, so you have to flush all of the np2ms that match that base pointer. What about this changelog: --- x86/np2m: Flush p2m rather than switching on nested invept At the moment, nvmx_handle_invept() updates the current np2m just to flush it. Instead introduce a function, np2m_flush_base(), which will look up the np2m base pointer and call p2m_flush_table() instead. Unfortunately, since we don't know which p2m a given vcpu is using, we must flush all p2ms that share that base pointer. Convert p2m_flush_table() into p2m_flush_table_locked() in order not to release the p2m_lock after np2m_base check. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- -George
On Mon, 2017-10-02 at 11:07 +0100, George Dunlap wrote: > On 10/02/2017 10:40 AM, George Dunlap wrote: > > On 10/02/2017 10:37 AM, Sergey Dyasli wrote: > > > On Fri, 2017-09-29 at 16:01 +0100, George Dunlap wrote: > > > > nvmx_handle_invept() updates current's np2m just to flush it. This is > > > > not only wasteful, but ineffective: if several L2 vcpus share the same > > > > np2m base pointer, they all need to be flushed (not only the current > > > > one). > > > > > > I don't follow this completely. L1 will use INVEPT on each vCPU that > > > shares the same np2m pointer. The main idea here was not to update > > > current's np2m just to flush it. > > > > Hmm, yes the INVEPT thing is true. But if that's the case, why do we > > need np2m_flush_base() to loop over the whole list and flush all np2ms > > with the same pointer? > > Oh, nevermind -- you don't know which np2m is being used by this vcpu, > so you have to flush all of the np2ms that match that base pointer. > > What about this changelog: > > --- > x86/np2m: Flush p2m rather than switching on nested invept It's not entirely clear what "switching" means here. But I fail to think of any other good alternatives for the patch's subject. > > At the moment, nvmx_handle_invept() updates the current np2m just to > flush it. Instead introduce a function, np2m_flush_base(), which will > look up the np2m base pointer and call p2m_flush_table() instead. > > Unfortunately, since we don't know which p2m a given vcpu is using, we > must flush all p2ms that share that base pointer. My reasoning was the same: INVEPT from L1 happens outside of L02 vCPU's context and currently it's impossible (because of scheduling) to detect the exact np2m object that needs to be flushed. > > Convert p2m_flush_table() into p2m_flush_table_locked() in order not > to release the p2m_lock after np2m_base check. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> -- Thanks, Sergey
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index cd0ee0a307..d333aa6d78 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1910,12 +1910,7 @@ int nvmx_handle_invept(struct cpu_user_regs *regs) { case INVEPT_SINGLE_CONTEXT: { - struct p2m_domain *p2m = p2m_get_nestedp2m(current, eptp); - if ( p2m ) - { - p2m_flush(current, p2m); - ept_sync_domain(p2m); - } + np2m_flush_base(current, eptp); break; } case INVEPT_ALL_CONTEXT: diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 27b90eb815..b7588b2ec1 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1711,15 +1711,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 @@ -1732,10 +1731,7 @@ p2m_flush_table(struct p2m_domain *p2m) /* No need to flush if it's already empty */ if ( p2m_is_nestedp2m(p2m) && p2m->np2m_base == P2M_BASE_EADDR ) - { - p2m_unlock(p2m); return; - } /* This is no longer a valid nested p2m for any address space */ p2m->np2m_base = P2M_BASE_EADDR; @@ -1755,7 +1751,14 @@ p2m_flush_table(struct p2m_domain *p2m) d->arch.paging.free_page(d, pg); } page_list_add(top, &p2m->pages); +} +/* Reset this p2m table to be empty */ +static void +p2m_flush_table(struct p2m_domain *p2m) +{ + p2m_lock(p2m); + p2m_flush_table_locked(p2m); p2m_unlock(p2m); } @@ -1776,6 +1779,26 @@ p2m_flush_nestedp2m(struct domain *d) p2m_flush_table(d->arch.nested_p2m[i]); } +void np2m_flush_base(struct vcpu *v, unsigned long np2m_base) +{ + struct domain *d = v->domain; + struct p2m_domain *p2m; + unsigned int i; + + np2m_base &= ~(0xfffull); + + nestedp2m_lock(d); + for ( i = 0; i < MAX_NESTEDP2M; i++ ) + { + p2m = d->arch.nested_p2m[i]; + p2m_lock(p2m); + if ( p2m->np2m_base == np2m_base ) + p2m_flush_table_locked(p2m); + 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 78f51a6ce6..5501ccc455 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 np2m_base */ +void np2m_flush_base(struct vcpu *v, unsigned long np2m_base); void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, l1_pgentry_t *p, l1_pgentry_t new, unsigned int level);