[PATCH v3 0/9] Nested p2m: allow sharing between vCPUs

[PATCH v3 0/9] Nested p2m: allow sharing between vCPUs

[Sergey Dyasli]

Nested p2m (shadow EPT) is an object that stores memory address
translations from L2 GPA directly to L0 HPA. This is achieved by
combining together L1 EPT with L0 EPT during L2 EPT violations.

In the usual case, L1 uses the same EPTP value in VMCS12 for all vCPUs
of a L2 guest. But unfortunately, in current Xen's implementation, each
vCPU has its own n2pm object which cannot be shared with other vCPUs.
This leads to the following issues if a nested guest has SMP:

    1. There will be multiple np2m objects (1 per nested vCPU) with
       the same np2m_base (L1 EPTP value in VMCS12).

    2. Same EPT violations will be processed independently by each vCPU

    3. Since MAX_NESTEDP2M is defined as 10, if a domain has more than
       10 nested vCPUs, performance will be extremely degraded due to
       constant np2m LRU list thrashing and np2m flushing.

This patch series makes it possible to share one np2m object between
different vCPUs that have the same np2m_base. Sharing of np2m objects
improves scalability of a domain from 10 nested vCPUs to 10 nested
guests (with an arbitrary number of vCPUs per guest).

v2 --> v3:
- "VMCX" is replaced with "VMCx" in comments and commit messages
- current pointer is now calculated only once in nvmx_eptp_update()
  and np2m_schedule()
- moved p2m_unlock() out of nestedhap_fix_p2m() for balanced lock/unlock
- Updated commit message in patch #2
- Replaced "shadow p2m" with "np2m" for consistency in commit message
  of patch #6

v1 --> v2 (by George):
- Fixed a race with stale_np2m and vmwrite
- Squashed 14 patches down to 9
- Updated commit messages

RFC --> v1:
- Some commit messages are updated based on George's comments
- Replaced VMX's terminology in common code with HVM's one
- Patch "x86/vvmx: add stale_eptp flag" is split into
  "x86/np2m: add stale_np2m flag" and
  "x86/vvmx: restart nested vmentry in case of stale_np2m"
- Added "x86/np2m: refactor p2m_get_nestedp2m_locked()" patch
- I've done some light nested SVM testing and fixed 1 regression
  (see patch #4)

Sergey Dyasli (9):
  x86/np2m: refactor p2m_get_nestedp2m()
  x86/np2m: flush all np2m objects on nested INVEPT
  x86/np2m: remove np2m_base from p2m_get_nestedp2m()
  x86/np2m: simplify nestedhvm_hap_nested_page_fault()
  x86/vvmx: make updating shadow EPTP value more efficient
  x86/np2m: send flush IPIs only when a vcpu is actively using an np2m
  x86/np2m: implement sharing of np2m between vCPUs
  x86/np2m: refactor p2m_get_nestedp2m_locked()
  x86/np2m: add break to np2m_flush_eptp()

 xen/arch/x86/domain.c            |   2 +
 xen/arch/x86/hvm/nestedhvm.c     |   3 +
 xen/arch/x86/hvm/svm/nestedsvm.c |   6 +-
 xen/arch/x86/hvm/vmx/entry.S     |   6 ++
 xen/arch/x86/hvm/vmx/vmx.c       |  14 ++--
 xen/arch/x86/hvm/vmx/vvmx.c      |  36 ++++++--
 xen/arch/x86/mm/hap/nested_hap.c |  34 ++++----
 xen/arch/x86/mm/p2m.c            | 175 ++++++++++++++++++++++++++++++++-------
 xen/include/asm-x86/hvm/vcpu.h   |   2 +
 xen/include/asm-x86/p2m.h        |  17 +++-
 10 files changed, 223 insertions(+), 72 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 1/9] x86/np2m: refactor p2m_get_nestedp2m()

[Sergey Dyasli]

1. Add a helper function assign_np2m()
2. Remove useless volatile
3. Update function's comment in the header
4. Minor style fixes ('\n' and d)

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/mm/p2m.c     | 31 ++++++++++++++++++-------------
 xen/include/asm-x86/p2m.h |  6 +++---
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 0b479105b9..27b90eb815 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1776,14 +1776,24 @@ p2m_flush_nestedp2m(struct domain *d)
         p2m_flush_table(d->arch.nested_p2m[i]);
 }
 
+static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m)
+{
+    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
+    struct domain *d = v->domain;
+
+    /* Bring this np2m to the top of the LRU list */
+    p2m_getlru_nestedp2m(d, p2m);
+
+    nv->nv_flushp2m = 0;
+    nv->nv_p2m = p2m;
+    cpumask_set_cpu(v->processor, p2m->dirty_cpumask);
+}
+
 struct p2m_domain *
 p2m_get_nestedp2m(struct vcpu *v, uint64_t np2m_base)
 {
-    /* Use volatile to prevent gcc to cache nv->nv_p2m in a cpu register as
-     * this may change within the loop by an other (v)cpu.
-     */
-    volatile struct nestedvcpu *nv = &vcpu_nestedhvm(v);
-    struct domain *d;
+    struct nestedvcpu *nv = &vcpu_nestedhvm(v);
+    struct domain *d = v->domain;
     struct p2m_domain *p2m;
 
     /* Mask out low bits; this avoids collisions with P2M_BASE_EADDR */
@@ -1793,7 +1803,6 @@ p2m_get_nestedp2m(struct vcpu *v, uint64_t np2m_base)
         nv->nv_p2m = NULL;
     }
 
-    d = v->domain;
     nestedp2m_lock(d);
     p2m = nv->nv_p2m;
     if ( p2m ) 
@@ -1801,15 +1810,13 @@ p2m_get_nestedp2m(struct vcpu *v, uint64_t np2m_base)
         p2m_lock(p2m);
         if ( p2m->np2m_base == np2m_base || p2m->np2m_base == P2M_BASE_EADDR )
         {
-            nv->nv_flushp2m = 0;
-            p2m_getlru_nestedp2m(d, p2m);
-            nv->nv_p2m = p2m;
             if ( p2m->np2m_base == P2M_BASE_EADDR )
                 hvm_asid_flush_vcpu(v);
             p2m->np2m_base = np2m_base;
-            cpumask_set_cpu(v->processor, p2m->dirty_cpumask);
+            assign_np2m(v, p2m);
             p2m_unlock(p2m);
             nestedp2m_unlock(d);
+
             return p2m;
         }
         p2m_unlock(p2m);
@@ -1820,11 +1827,9 @@ p2m_get_nestedp2m(struct vcpu *v, uint64_t np2m_base)
     p2m = p2m_getlru_nestedp2m(d, NULL);
     p2m_flush_table(p2m);
     p2m_lock(p2m);
-    nv->nv_p2m = p2m;
     p2m->np2m_base = np2m_base;
-    nv->nv_flushp2m = 0;
     hvm_asid_flush_vcpu(v);
-    cpumask_set_cpu(v->processor, p2m->dirty_cpumask);
+    assign_np2m(v, p2m);
     p2m_unlock(p2m);
     nestedp2m_unlock(d);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 8f3409b400..338317a782 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -359,9 +359,9 @@ struct p2m_domain {
 /* get host p2m table */
 #define p2m_get_hostp2m(d)      ((d)->arch.p2m)
 
-/* Get p2m table (re)usable for specified np2m base.
- * Automatically destroys and re-initializes a p2m if none found.
- * If np2m_base == 0 then v->arch.hvm_vcpu.guest_cr[3] is used.
+/*
+ * Assigns an np2m with the specified np2m_base to the specified vCPU
+ * and returns that np2m.
  */
 struct p2m_domain *p2m_get_nestedp2m(struct vcpu *v, uint64_t np2m_base);
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 2/9] x86/np2m: flush all np2m objects on nested INVEPT

[Sergey Dyasli]

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>
---
v2 --> v3:
- Commit message update
---
 xen/arch/x86/hvm/vmx/vvmx.c |  7 +------
 xen/arch/x86/mm/p2m.c       | 35 +++++++++++++++++++++++++++++------
 xen/include/asm-x86/p2m.h   |  2 ++
 3 files changed, 32 insertions(+), 12 deletions(-)

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 338317a782..ce50e37f46 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -772,6 +772,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);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 3/9] x86/np2m: remove np2m_base from p2m_get_nestedp2m()

[Sergey Dyasli]

Remove np2m_base parameter as it should always match the value of
np2m_base in VMCx12.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 6 +++++-
 xen/arch/x86/hvm/vmx/vvmx.c      | 3 +--
 xen/arch/x86/mm/hap/nested_hap.c | 2 +-
 xen/arch/x86/mm/p2m.c            | 8 ++++----
 xen/include/asm-x86/p2m.h        | 5 ++---
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 66a1777298..1de896e456 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -412,7 +412,11 @@ static void nestedsvm_vmcb_set_nestedp2m(struct vcpu *v,
     ASSERT(v != NULL);
     ASSERT(vvmcb != NULL);
     ASSERT(n2vmcb != NULL);
-    p2m = p2m_get_nestedp2m(v, vvmcb->_h_cr3);
+
+    /* This will allow nsvm_vcpu_hostcr3() to return correct np2m_base */
+    vcpu_nestedsvm(v).ns_vmcb_hostcr3 = vvmcb->_h_cr3;
+
+    p2m = p2m_get_nestedp2m(v);
     n2vmcb->_h_cr3 = pagetable_get_paddr(p2m_get_pagetable(p2m));
 }
 
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d333aa6d78..2f468e6ced 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1109,8 +1109,7 @@ static void load_shadow_guest_state(struct vcpu *v)
 
 uint64_t get_shadow_eptp(struct vcpu *v)
 {
-    uint64_t np2m_base = nvmx_vcpu_eptp_base(v);
-    struct p2m_domain *p2m = p2m_get_nestedp2m(v, np2m_base);
+    struct p2m_domain *p2m = p2m_get_nestedp2m(v);
     struct ept_data *ept = &p2m->ept;
 
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index 162afed46b..ed137fa784 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -212,7 +212,7 @@ nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t *L2_gpa,
     uint8_t p2ma_21 = p2m_access_rwx;
 
     p2m = p2m_get_hostp2m(d); /* L0 p2m */
-    nested_p2m = p2m_get_nestedp2m(v, nhvm_vcpu_p2m_base(v));
+    nested_p2m = p2m_get_nestedp2m(v);
 
     /* walk the L1 P2M table */
     rv = nestedhap_walk_L1_p2m(v, *L2_gpa, &L1_gpa, &page_order_21, &p2ma_21,
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b7588b2ec1..d3e602de22 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1813,11 +1813,12 @@ static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m)
 }
 
 struct p2m_domain *
-p2m_get_nestedp2m(struct vcpu *v, uint64_t np2m_base)
+p2m_get_nestedp2m(struct vcpu *v)
 {
     struct nestedvcpu *nv = &vcpu_nestedhvm(v);
     struct domain *d = v->domain;
     struct p2m_domain *p2m;
+    uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
 
     /* Mask out low bits; this avoids collisions with P2M_BASE_EADDR */
     np2m_base &= ~(0xfffull);
@@ -1865,7 +1866,7 @@ p2m_get_p2m(struct vcpu *v)
     if (!nestedhvm_is_n2(v))
         return p2m_get_hostp2m(v->domain);
 
-    return p2m_get_nestedp2m(v, nhvm_vcpu_p2m_base(v));
+    return p2m_get_nestedp2m(v);
 }
 
 unsigned long paging_gva_to_gfn(struct vcpu *v,
@@ -1880,13 +1881,12 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
         unsigned long l2_gfn, l1_gfn;
         struct p2m_domain *p2m;
         const struct paging_mode *mode;
-        uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
         uint8_t l1_p2ma;
         unsigned int l1_page_order;
         int rv;
 
         /* translate l2 guest va into l2 guest gfn */
-        p2m = p2m_get_nestedp2m(v, np2m_base);
+        p2m = p2m_get_nestedp2m(v);
         mode = paging_get_nestedmode(v);
         l2_gfn = mode->gva_to_gfn(v, p2m, va, pfec);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index ce50e37f46..798295ec12 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -360,10 +360,9 @@ struct p2m_domain {
 #define p2m_get_hostp2m(d)      ((d)->arch.p2m)
 
 /*
- * Assigns an np2m with the specified np2m_base to the specified vCPU
- * and returns that np2m.
+ * Updates vCPU's n2pm to match its np2m_base in VMCx12 and returns that np2m.
  */
-struct p2m_domain *p2m_get_nestedp2m(struct vcpu *v, uint64_t np2m_base);
+struct p2m_domain *p2m_get_nestedp2m(struct vcpu *v);
 
 /* If vcpu is in host mode then behaviour matches p2m_get_hostp2m().
  * If vcpu is in guest mode then behaviour matches p2m_get_nestedp2m().
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 4/9] x86/np2m: simplify nestedhvm_hap_nested_page_fault()

[Sergey Dyasli]

There is a possibility for nested_p2m to became stale between
nestedhvm_hap_nested_page_fault() and nestedhap_fix_p2m().  At the moment
this is handled by detecting such a race inside nestedhap_fix_p2m() and
special-casing it.

Instead, introduce p2m_get_nestedp2m_locked(), which will returned a
still-locked p2m.  This allows us to call nestedhap_fix_p2m() with the
lock held and remove the code detecting the special-case.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2 --> v3:
- Moved p2m_unlock() out of nestedhap_fix_p2m() for balanced lock/unlock
---
 xen/arch/x86/mm/hap/nested_hap.c | 34 ++++++++++++++--------------------
 xen/arch/x86/mm/p2m.c            | 12 +++++++++---
 xen/include/asm-x86/p2m.h        |  2 ++
 3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index ed137fa784..d7277cccdc 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -101,30 +101,23 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
                   unsigned int page_order, p2m_type_t p2mt, p2m_access_t p2ma)
 {
     int rc = 0;
+    unsigned long gfn, mask;
+    mfn_t mfn;
+
     ASSERT(p2m);
     ASSERT(p2m->set_entry);
+    ASSERT(p2m_locked_by_me(p2m));
 
-    p2m_lock(p2m);
-
-    /* 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->np2m_base == nhvm_vcpu_p2m_base(v) )
-    {
-        unsigned long gfn, mask;
-        mfn_t mfn;
+    /*
+     * If this is a superpage mapping, round down both addresses to
+     * the start of the superpage.
+     */
+    mask = ~((1UL << page_order) - 1);
 
-        /* If this is a superpage mapping, round down both addresses
-         * to the start of the superpage. */
-        mask = ~((1UL << page_order) - 1);
-
-        gfn = (L2_gpa >> PAGE_SHIFT) & mask;
-        mfn = _mfn((L0_gpa >> PAGE_SHIFT) & mask);
-
-        rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
-    }
+    gfn = (L2_gpa >> PAGE_SHIFT) & mask;
+    mfn = _mfn((L0_gpa >> PAGE_SHIFT) & mask);
 
-    p2m_unlock(p2m);
+    rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
 
     if ( rc )
     {
@@ -212,7 +205,6 @@ nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t *L2_gpa,
     uint8_t p2ma_21 = p2m_access_rwx;
 
     p2m = p2m_get_hostp2m(d); /* L0 p2m */
-    nested_p2m = p2m_get_nestedp2m(v);
 
     /* walk the L1 P2M table */
     rv = nestedhap_walk_L1_p2m(v, *L2_gpa, &L1_gpa, &page_order_21, &p2ma_21,
@@ -278,8 +270,10 @@ nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t *L2_gpa,
     p2ma_10 &= (p2m_access_t)p2ma_21;
 
     /* fix p2m_get_pagetable(nested_p2m) */
+    nested_p2m = p2m_get_nestedp2m_locked(v);
     nestedhap_fix_p2m(v, nested_p2m, *L2_gpa, L0_gpa, page_order_20,
         p2mt_10, p2ma_10);
+    p2m_unlock(nested_p2m);
 
     return NESTEDHVM_PAGEFAULT_DONE;
 }
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d3e602de22..aa3182dec6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1813,7 +1813,7 @@ static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m)
 }
 
 struct p2m_domain *
-p2m_get_nestedp2m(struct vcpu *v)
+p2m_get_nestedp2m_locked(struct vcpu *v)
 {
     struct nestedvcpu *nv = &vcpu_nestedhvm(v);
     struct domain *d = v->domain;
@@ -1838,7 +1838,6 @@ p2m_get_nestedp2m(struct vcpu *v)
                 hvm_asid_flush_vcpu(v);
             p2m->np2m_base = np2m_base;
             assign_np2m(v, p2m);
-            p2m_unlock(p2m);
             nestedp2m_unlock(d);
 
             return p2m;
@@ -1854,12 +1853,19 @@ p2m_get_nestedp2m(struct vcpu *v)
     p2m->np2m_base = np2m_base;
     hvm_asid_flush_vcpu(v);
     assign_np2m(v, p2m);
-    p2m_unlock(p2m);
     nestedp2m_unlock(d);
 
     return p2m;
 }
 
+struct p2m_domain *p2m_get_nestedp2m(struct vcpu *v)
+{
+    struct p2m_domain *p2m = p2m_get_nestedp2m_locked(v);
+    p2m_unlock(p2m);
+
+    return p2m;
+}
+
 struct p2m_domain *
 p2m_get_p2m(struct vcpu *v)
 {
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 798295ec12..9a757792ee 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -363,6 +363,8 @@ struct p2m_domain {
  * Updates vCPU's n2pm to match its np2m_base in VMCx12 and returns that np2m.
  */
 struct p2m_domain *p2m_get_nestedp2m(struct vcpu *v);
+/* Similar to the above except that returned p2m is still write-locked */
+struct p2m_domain *p2m_get_nestedp2m_locked(struct vcpu *v);
 
 /* If vcpu is in host mode then behaviour matches p2m_get_hostp2m().
  * If vcpu is in guest mode then behaviour matches p2m_get_nestedp2m().
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Sergey Dyasli]

At the moment, the shadow EPTP value is written unconditionally in
ept_handle_violation().

Instead, write the value on vmentry to the guest; but only write it if
the value needs updating.

To detect this, add a flag to the nestedvcpu struct, stale_np2m, to
indicate when such an action is necessary.  Set it when the nested p2m
changes or when the np2m is flushed by an IPI, and clear it when we
write the new value.

Since an IPI invalidating the p2m may happen between
nvmx_switch_guest() and vmx_vmenter, but we can't perform the vmwrite
with interrupts disabled, check the flag just before entering the
guest and restart the vmentry if it's set.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2 --> v3:
- current pointer is now calculated only once in nvmx_eptp_update()
---
 xen/arch/x86/hvm/nestedhvm.c   |  2 ++
 xen/arch/x86/hvm/vmx/entry.S   |  6 ++++++
 xen/arch/x86/hvm/vmx/vmx.c     | 14 +++++++-------
 xen/arch/x86/hvm/vmx/vvmx.c    | 22 ++++++++++++++++++++++
 xen/arch/x86/mm/p2m.c          | 10 ++++++++--
 xen/include/asm-x86/hvm/vcpu.h |  1 +
 6 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
index f2f7469d86..74a464d162 100644
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -56,6 +56,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
     nv->nv_vvmcxaddr = INVALID_PADDR;
     nv->nv_flushp2m = 0;
     nv->nv_p2m = NULL;
+    nv->stale_np2m = false;
 
     hvm_asid_flush_vcpu_asid(&nv->nv_n2asid);
 
@@ -107,6 +108,7 @@ nestedhvm_flushtlb_ipi(void *info)
      */
     hvm_asid_flush_core();
     vcpu_nestedhvm(v).nv_p2m = NULL;
+    vcpu_nestedhvm(v).stale_np2m = true;
 }
 
 void
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 53eedc6363..9fb8f89220 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -79,6 +79,8 @@ UNLIKELY_END(realmode)
 
         mov  %rsp,%rdi
         call vmx_vmenter_helper
+        cmp  $0,%eax
+        jne .Lvmx_vmentry_restart
         mov  VCPU_hvm_guest_cr2(%rbx),%rax
 
         pop  %r15
@@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry)
         GET_CURRENT(bx)
         jmp  .Lvmx_do_vmentry
 
+.Lvmx_vmentry_restart:
+        sti
+        jmp  .Lvmx_do_vmentry
+
 .Lvmx_goto_emulator:
         sti
         mov  %rsp,%rdi
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9cfa9b6965..c9a4111267 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3249,12 +3249,6 @@ static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
     case 0:         // Unhandled L1 EPT violation
         break;
     case 1:         // This violation is handled completly
-        /*Current nested EPT maybe flushed by other vcpus, so need
-         * to re-set its shadow EPTP pointer.
-         */
-        if ( nestedhvm_vcpu_in_guestmode(current) &&
-                        nestedhvm_paging_mode_hap(current ) )
-            __vmwrite(EPT_POINTER, get_shadow_eptp(current));
         return;
     case -1:        // This vioaltion should be injected to L1 VMM
         vcpu_nestedhvm(current).nv_vmexit_pending = 1;
@@ -4203,13 +4197,17 @@ static void lbr_fixup(void)
         bdw_erratum_bdf14_fixup();
 }
 
-void vmx_vmenter_helper(const struct cpu_user_regs *regs)
+int vmx_vmenter_helper(const struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     u32 new_asid, old_asid;
     struct hvm_vcpu_asid *p_asid;
     bool_t need_flush;
 
+    /* Shadow EPTP can't be updated here because irqs are disabled */
+     if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
+         return 1;
+
     if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
         curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
 
@@ -4270,6 +4268,8 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
     __vmwrite(GUEST_RIP,    regs->rip);
     __vmwrite(GUEST_RSP,    regs->rsp);
     __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS);
+
+    return 0;
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 2f468e6ced..3f596dc698 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1405,12 +1405,34 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
     vmsucceed(regs);
 }
 
+static void nvmx_eptp_update(void)
+{
+    struct vcpu *curr = current;
+
+    if ( !nestedhvm_vcpu_in_guestmode(curr) ||
+          vcpu_nestedhvm(curr).nv_vmexit_pending ||
+         !vcpu_nestedhvm(curr).stale_np2m ||
+         !nestedhvm_paging_mode_hap(curr) )
+        return;
+
+    /*
+     * Interrupts are enabled here, so we need to clear stale_np2m
+     * before we do the vmwrite.  If we do it in the other order, an
+     * and IPI comes in changing the shadow eptp after the vmwrite,
+     * we'll complete the vmenter with a stale eptp value.
+     */
+    vcpu_nestedhvm(curr).stale_np2m = false;
+    __vmwrite(EPT_POINTER, get_shadow_eptp(curr));
+}
+
 void nvmx_switch_guest(void)
 {
     struct vcpu *v = current;
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     struct cpu_user_regs *regs = guest_cpu_user_regs();
 
+    nvmx_eptp_update();
+
     /*
      * A pending IO emulation may still be not finished. In this case, no
      * virtual vmswitch is allowed. Or else, the following IO emulation will
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index aa3182dec6..fd48a3b9db 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1812,6 +1812,12 @@ static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m)
     cpumask_set_cpu(v->processor, p2m->dirty_cpumask);
 }
 
+static void nvcpu_flush(struct vcpu *v)
+{
+    hvm_asid_flush_vcpu(v);
+    vcpu_nestedhvm(v).stale_np2m = true;
+}
+
 struct p2m_domain *
 p2m_get_nestedp2m_locked(struct vcpu *v)
 {
@@ -1835,7 +1841,7 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
         if ( p2m->np2m_base == np2m_base || p2m->np2m_base == P2M_BASE_EADDR )
         {
             if ( p2m->np2m_base == P2M_BASE_EADDR )
-                hvm_asid_flush_vcpu(v);
+                nvcpu_flush(v);
             p2m->np2m_base = np2m_base;
             assign_np2m(v, p2m);
             nestedp2m_unlock(d);
@@ -1851,7 +1857,7 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
     p2m_flush_table(p2m);
     p2m_lock(p2m);
     p2m->np2m_base = np2m_base;
-    hvm_asid_flush_vcpu(v);
+    nvcpu_flush(v);
     assign_np2m(v, p2m);
     nestedp2m_unlock(d);
 
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index 6c54773f1c..27330242e3 100644
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -115,6 +115,7 @@ struct nestedvcpu {
 
     bool_t nv_flushp2m; /* True, when p2m table must be flushed */
     struct p2m_domain *nv_p2m; /* used p2m table for this vcpu */
+    bool stale_np2m; /* True when p2m_base in VMCx02 is no longer valid */
 
     struct hvm_vcpu_asid nv_n2asid;
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 6/9] x86/np2m: send flush IPIs only when a vcpu is actively using an np2m

[Sergey Dyasli]

Flush IPIs are sent to all cpus in an np2m's dirty_cpumask when
updated.  This mask however is far too broad.  A pcpu's bit is set in
the cpumask when a vcpu runs on that pcpu, but is only cleared when a
flush happens.  This means that the IPI includes the current pcpu of
vcpus that are not currently running, and also includes any pcpu that
has ever had a vcpu use this p2m since the last flush (which in turn
will cause spurious invalidations if a different vcpu is using an np2m).

Avoid these IPIs by keeping closer track of where an np2m is being used,
and when a vcpu needs to be flushed:

- On schedule-out, clear v->processor in p2m->dirty_cpumask
- Add a 'generation' counter to the p2m and nestedvcpu structs to
  detect changes that would require re-loads on re-entry
- On schedule-in or p2m change:
  - Set v->processor in p2m->dirty_cpumask
  - flush the vcpu's nested p2m pointer (and update nv->generation) if
    the generation changed

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2 --> v3:
- current pointer is now calculated only once in np2m_schedule()
- Replaced "shadow p2m" with "np2m" for consistency in commit message
---
 xen/arch/x86/domain.c          |  2 ++
 xen/arch/x86/hvm/nestedhvm.c   |  1 +
 xen/arch/x86/hvm/vmx/vvmx.c    |  3 +++
 xen/arch/x86/mm/p2m.c          | 56 +++++++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/hvm/vcpu.h |  1 +
 xen/include/asm-x86/p2m.h      |  6 +++++
 6 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 466a1a2fac..35ea0d2418 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1668,6 +1668,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
     {
         _update_runstate_area(prev);
         vpmu_switch_from(prev);
+        np2m_schedule(NP2M_SCHEDLE_OUT);
     }
 
     if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm_vcpu.tm_list) )
@@ -1716,6 +1717,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 
         /* Must be done with interrupts enabled */
         vpmu_switch_to(next);
+        np2m_schedule(NP2M_SCHEDLE_IN);
     }
 
     /* Ensure that the vcpu has an up-to-date time base. */
diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
index 74a464d162..ab50b2ab98 100644
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -57,6 +57,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
     nv->nv_flushp2m = 0;
     nv->nv_p2m = NULL;
     nv->stale_np2m = false;
+    nv->np2m_generation = 0;
 
     hvm_asid_flush_vcpu_asid(&nv->nv_n2asid);
 
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 3f596dc698..198ca72f2a 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1367,6 +1367,9 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
          !(v->arch.hvm_vcpu.guest_efer & EFER_LMA) )
         shadow_to_vvmcs_bulk(v, ARRAY_SIZE(gpdpte_fields), gpdpte_fields);
 
+    /* This will clear current pCPU bit in p2m->dirty_cpumask */
+    np2m_schedule(NP2M_SCHEDLE_OUT);
+
     vmx_vmcs_switch(v->arch.hvm_vmx.vmcs_pa, nvcpu->nv_n1vmcx_pa);
 
     nestedhvm_vcpu_exit_guestmode(v);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index fd48a3b9db..3c62292165 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -73,6 +73,7 @@ static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
     p2m->p2m_class = p2m_host;
 
     p2m->np2m_base = P2M_BASE_EADDR;
+    p2m->np2m_generation = 0;
 
     for ( i = 0; i < ARRAY_SIZE(p2m->pod.mrp.list); ++i )
         p2m->pod.mrp.list[i] = gfn_x(INVALID_GFN);
@@ -1735,6 +1736,7 @@ p2m_flush_table_locked(struct p2m_domain *p2m)
 
     /* This is no longer a valid nested p2m for any address space */
     p2m->np2m_base = P2M_BASE_EADDR;
+    p2m->np2m_generation++;
 
     /* Make sure nobody else is using this p2m table */
     nestedhvm_vmcx_flushtlb(p2m);
@@ -1809,6 +1811,7 @@ static void assign_np2m(struct vcpu *v, struct p2m_domain *p2m)
 
     nv->nv_flushp2m = 0;
     nv->nv_p2m = p2m;
+    nv->np2m_generation = p2m->np2m_generation;
     cpumask_set_cpu(v->processor, p2m->dirty_cpumask);
 }
 
@@ -1840,7 +1843,9 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
         p2m_lock(p2m);
         if ( p2m->np2m_base == np2m_base || p2m->np2m_base == P2M_BASE_EADDR )
         {
-            if ( p2m->np2m_base == P2M_BASE_EADDR )
+            /* Check if np2m was flushed just before the lock */
+            if ( p2m->np2m_base == P2M_BASE_EADDR ||
+                 nv->np2m_generation != p2m->np2m_generation )
                 nvcpu_flush(v);
             p2m->np2m_base = np2m_base;
             assign_np2m(v, p2m);
@@ -1848,6 +1853,11 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
 
             return p2m;
         }
+        else
+        {
+            /* vCPU is switching from some other valid np2m */
+            cpumask_clear_cpu(v->processor, p2m->dirty_cpumask);
+        }
         p2m_unlock(p2m);
     }
 
@@ -1881,6 +1891,50 @@ p2m_get_p2m(struct vcpu *v)
     return p2m_get_nestedp2m(v);
 }
 
+void np2m_schedule(int dir)
+{
+    struct vcpu *curr = current;
+    struct nestedvcpu *nv = &vcpu_nestedhvm(curr);
+    struct p2m_domain *p2m;
+
+    ASSERT(dir == NP2M_SCHEDLE_IN || dir == NP2M_SCHEDLE_OUT);
+
+    if ( !nestedhvm_enabled(curr->domain) ||
+         !nestedhvm_vcpu_in_guestmode(curr) ||
+         !nestedhvm_paging_mode_hap(curr) )
+        return;
+
+    p2m = nv->nv_p2m;
+    if ( p2m )
+    {
+        bool np2m_valid;
+
+        p2m_lock(p2m);
+        np2m_valid = p2m->np2m_base == nhvm_vcpu_p2m_base(curr) &&
+                     nv->np2m_generation == p2m->np2m_generation;
+        if ( dir == NP2M_SCHEDLE_OUT && np2m_valid )
+        {
+            /*
+             * The np2m is up to date but this vCPU will no longer use it,
+             * which means there are no reasons to send a flush IPI.
+             */
+            cpumask_clear_cpu(curr->processor, p2m->dirty_cpumask);
+        }
+        else if ( dir == NP2M_SCHEDLE_IN )
+        {
+            if ( !np2m_valid )
+            {
+                /* This vCPU's np2m was flushed while it was not runnable */
+                hvm_asid_flush_core();
+                vcpu_nestedhvm(curr).nv_p2m = NULL;
+            }
+            else
+                cpumask_set_cpu(curr->processor, p2m->dirty_cpumask);
+        }
+        p2m_unlock(p2m);
+    }
+}
+
 unsigned long paging_gva_to_gfn(struct vcpu *v,
                                 unsigned long va,
                                 uint32_t *pfec)
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index 27330242e3..d93166fb92 100644
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -116,6 +116,7 @@ struct nestedvcpu {
     bool_t nv_flushp2m; /* True, when p2m table must be flushed */
     struct p2m_domain *nv_p2m; /* used p2m table for this vcpu */
     bool stale_np2m; /* True when p2m_base in VMCx02 is no longer valid */
+    uint64_t np2m_generation;
 
     struct hvm_vcpu_asid nv_n2asid;
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 9a757792ee..182463b247 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -209,6 +209,7 @@ struct p2m_domain {
      * to set it to any other value. */
 #define P2M_BASE_EADDR     (~0ULL)
     uint64_t           np2m_base;
+    uint64_t           np2m_generation;
 
     /* Nested p2ms: linked list of n2pms allocated to this domain. 
      * The host p2m hasolds the head of the list and the np2ms are 
@@ -371,6 +372,11 @@ struct p2m_domain *p2m_get_nestedp2m_locked(struct vcpu *v);
  */
 struct p2m_domain *p2m_get_p2m(struct vcpu *v);
 
+#define NP2M_SCHEDLE_IN  0
+#define NP2M_SCHEDLE_OUT 1
+
+void np2m_schedule(int dir);
+
 static inline bool_t p2m_is_hostp2m(const struct p2m_domain *p2m)
 {
     return p2m->p2m_class == p2m_host;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 7/9] x86/np2m: implement sharing of np2m between vCPUs

[Sergey Dyasli]

At the moment, nested p2ms are not shared between vcpus even if they
share the same base pointer.

Modify p2m_get_nestedp2m() to allow sharing a np2m between multiple
vcpus with the same np2m_base (L1 np2m_base value in VMCx12).

If the current np2m doesn't match the current base pointer, first look
for another nested p2m in the same domain with the same base pointer,
before reclaiming one from the LRU.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |  1 +
 xen/arch/x86/mm/p2m.c       | 26 ++++++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 198ca72f2a..dde02c076b 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1201,6 +1201,7 @@ static void virtual_vmentry(struct cpu_user_regs *regs)
 
     /* Setup virtual ETP for L2 guest*/
     if ( nestedhvm_paging_mode_hap(v) )
+        /* This will setup the initial np2m for the nested vCPU */
         __vmwrite(EPT_POINTER, get_shadow_eptp(v));
     else
         __vmwrite(EPT_POINTER, get_host_eptp(v));
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 3c62292165..90bf382a49 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1828,6 +1828,7 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
     struct domain *d = v->domain;
     struct p2m_domain *p2m;
     uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
+    unsigned int i;
 
     /* Mask out low bits; this avoids collisions with P2M_BASE_EADDR */
     np2m_base &= ~(0xfffull);
@@ -1841,19 +1842,19 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
     if ( p2m ) 
     {
         p2m_lock(p2m);
-        if ( p2m->np2m_base == np2m_base || p2m->np2m_base == P2M_BASE_EADDR )
+        if ( p2m->np2m_base == np2m_base )
         {
             /* Check if np2m was flushed just before the lock */
-            if ( p2m->np2m_base == P2M_BASE_EADDR ||
-                 nv->np2m_generation != p2m->np2m_generation )
+            if ( nv->np2m_generation != p2m->np2m_generation )
                 nvcpu_flush(v);
+            /* np2m is up-to-date */
             p2m->np2m_base = np2m_base;
             assign_np2m(v, p2m);
             nestedp2m_unlock(d);
 
             return p2m;
         }
-        else
+        else if ( p2m->np2m_base != P2M_BASE_EADDR )
         {
             /* vCPU is switching from some other valid np2m */
             cpumask_clear_cpu(v->processor, p2m->dirty_cpumask);
@@ -1861,6 +1862,23 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
         p2m_unlock(p2m);
     }
 
+    /* Share a np2m if possible */
+    for ( i = 0; i < MAX_NESTEDP2M; i++ )
+    {
+        p2m = d->arch.nested_p2m[i];
+        p2m_lock(p2m);
+        if ( p2m->np2m_base == np2m_base )
+        {
+            nvcpu_flush(v);
+            p2m->np2m_base = np2m_base;
+            assign_np2m(v, 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);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 8/9] x86/np2m: refactor p2m_get_nestedp2m_locked()

[Sergey Dyasli]

Remove some code duplication.

Suggested-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/mm/p2m.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 90bf382a49..6c937c9e17 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1829,6 +1829,7 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
     struct p2m_domain *p2m;
     uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
     unsigned int i;
+    bool needs_flush = true;
 
     /* Mask out low bits; this avoids collisions with P2M_BASE_EADDR */
     np2m_base &= ~(0xfffull);
@@ -1845,14 +1846,10 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
         if ( p2m->np2m_base == np2m_base )
         {
             /* Check if np2m was flushed just before the lock */
-            if ( nv->np2m_generation != p2m->np2m_generation )
-                nvcpu_flush(v);
+            if ( nv->np2m_generation == p2m->np2m_generation )
+                needs_flush = false;
             /* np2m is up-to-date */
-            p2m->np2m_base = np2m_base;
-            assign_np2m(v, p2m);
-            nestedp2m_unlock(d);
-
-            return p2m;
+            goto found;
         }
         else if ( p2m->np2m_base != P2M_BASE_EADDR )
         {
@@ -1867,15 +1864,10 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
     {
         p2m = d->arch.nested_p2m[i];
         p2m_lock(p2m);
+
         if ( p2m->np2m_base == np2m_base )
-        {
-            nvcpu_flush(v);
-            p2m->np2m_base = np2m_base;
-            assign_np2m(v, p2m);
-            nestedp2m_unlock(d);
+            goto found;
 
-            return p2m;
-        }
         p2m_unlock(p2m);
     }
 
@@ -1884,8 +1876,11 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
     p2m = p2m_getlru_nestedp2m(d, NULL);
     p2m_flush_table(p2m);
     p2m_lock(p2m);
+
+ found:
+    if ( needs_flush )
+        nvcpu_flush(v);
     p2m->np2m_base = np2m_base;
-    nvcpu_flush(v);
     assign_np2m(v, p2m);
     nestedp2m_unlock(d);
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 9/9] x86/np2m: add break to np2m_flush_eptp()

[Sergey Dyasli]

Now that np2m sharing is implemented, there can be only one np2m object
with the same np2m_base. Break from loop if the required np2m was found
during np2m_flush_eptp().

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/mm/p2m.c     | 4 ++++
 xen/include/asm-x86/p2m.h | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6c937c9e17..d36eee7ae0 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1795,7 +1795,11 @@ void np2m_flush_base(struct vcpu *v, unsigned long np2m_base)
         p2m = d->arch.nested_p2m[i];
         p2m_lock(p2m);
         if ( p2m->np2m_base == np2m_base )
+        {
             p2m_flush_table_locked(p2m);
+            p2m_unlock(p2m);
+            break;
+        }
         p2m_unlock(p2m);
     }
     nestedp2m_unlock(d);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 182463b247..a26070957f 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -779,7 +779,7 @@ 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 */
+/* Flushes the np2m specified by np2m_base (if it exists) */
 void np2m_flush_base(struct vcpu *v, unsigned long np2m_base);
 
 void nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 3/9] x86/np2m: remove np2m_base from p2m_get_nestedp2m()

[Boris Ostrovsky]

On 10/03/2017 11:20 AM, Sergey Dyasli wrote:
> Remove np2m_base parameter as it should always match the value of
> np2m_base in VMCx12.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

SVM bits:

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 2/9] x86/np2m: flush all np2m objects on nested INVEPT

[George Dunlap]

On 10/03/2017 04:20 PM, Sergey Dyasli wrote:
> 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>
> ---
> v2 --> v3:
> - Commit message update
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c |  7 +------
>  xen/arch/x86/mm/p2m.c       | 35 +++++++++++++++++++++++++++++------
>  xen/include/asm-x86/p2m.h   |  2 ++

This needs a VMX maintainer's Ack.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 4/9] x86/np2m: simplify nestedhvm_hap_nested_page_fault()

[George Dunlap]

On 10/03/2017 04:20 PM, Sergey Dyasli wrote:
> There is a possibility for nested_p2m to became stale between
> nestedhvm_hap_nested_page_fault() and nestedhap_fix_p2m().  At the moment
> this is handled by detecting such a race inside nestedhap_fix_p2m() and
> special-casing it.
> 
> Instead, introduce p2m_get_nestedp2m_locked(), which will returned a
> still-locked p2m.  This allows us to call nestedhap_fix_p2m() with the
> lock held and remove the code detecting the special-case.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

It seems really strange to have two functions named _locked() which have
different semantics (one of which assumes that the p2m has already been
locked, one which returns a p2m which is locked).  But I can't think of
a better option, so:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[George Dunlap]

On 10/03/2017 04:21 PM, Sergey Dyasli wrote:
> At the moment, the shadow EPTP value is written unconditionally in
> ept_handle_violation().
> 
> Instead, write the value on vmentry to the guest; but only write it if
> the value needs updating.
> 
> To detect this, add a flag to the nestedvcpu struct, stale_np2m, to
> indicate when such an action is necessary.  Set it when the nested p2m
> changes or when the np2m is flushed by an IPI, and clear it when we
> write the new value.
> 
> Since an IPI invalidating the p2m may happen between
> nvmx_switch_guest() and vmx_vmenter, but we can't perform the vmwrite
> with interrupts disabled, check the flag just before entering the
> guest and restart the vmentry if it's set.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Looks good to me; just one question...

> ---
> v2 --> v3:
> - current pointer is now calculated only once in nvmx_eptp_update()
> ---
>  xen/arch/x86/hvm/nestedhvm.c   |  2 ++
>  xen/arch/x86/hvm/vmx/entry.S   |  6 ++++++
>  xen/arch/x86/hvm/vmx/vmx.c     | 14 +++++++-------
>  xen/arch/x86/hvm/vmx/vvmx.c    | 22 ++++++++++++++++++++++
>  xen/arch/x86/mm/p2m.c          | 10 ++++++++--
>  xen/include/asm-x86/hvm/vcpu.h |  1 +
>  6 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
> index f2f7469d86..74a464d162 100644
> --- a/xen/arch/x86/hvm/nestedhvm.c
> +++ b/xen/arch/x86/hvm/nestedhvm.c
> @@ -56,6 +56,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
>      nv->nv_vvmcxaddr = INVALID_PADDR;
>      nv->nv_flushp2m = 0;
>      nv->nv_p2m = NULL;
> +    nv->stale_np2m = false;
>  
>      hvm_asid_flush_vcpu_asid(&nv->nv_n2asid);
>  
> @@ -107,6 +108,7 @@ nestedhvm_flushtlb_ipi(void *info)
>       */
>      hvm_asid_flush_core();
>      vcpu_nestedhvm(v).nv_p2m = NULL;
> +    vcpu_nestedhvm(v).stale_np2m = true;
>  }
>  
>  void
> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
> index 53eedc6363..9fb8f89220 100644
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -79,6 +79,8 @@ UNLIKELY_END(realmode)
>  
>          mov  %rsp,%rdi
>          call vmx_vmenter_helper
> +        cmp  $0,%eax
> +        jne .Lvmx_vmentry_restart
>          mov  VCPU_hvm_guest_cr2(%rbx),%rax
>  
>          pop  %r15
> @@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry)
>          GET_CURRENT(bx)
>          jmp  .Lvmx_do_vmentry
>  
> +.Lvmx_vmentry_restart:
> +        sti
> +        jmp  .Lvmx_do_vmentry
> +
>  .Lvmx_goto_emulator:
>          sti
>          mov  %rsp,%rdi
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 9cfa9b6965..c9a4111267 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3249,12 +3249,6 @@ static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
>      case 0:         // Unhandled L1 EPT violation
>          break;
>      case 1:         // This violation is handled completly
> -        /*Current nested EPT maybe flushed by other vcpus, so need
> -         * to re-set its shadow EPTP pointer.
> -         */
> -        if ( nestedhvm_vcpu_in_guestmode(current) &&
> -                        nestedhvm_paging_mode_hap(current ) )
> -            __vmwrite(EPT_POINTER, get_shadow_eptp(current));
>          return;
>      case -1:        // This vioaltion should be injected to L1 VMM
>          vcpu_nestedhvm(current).nv_vmexit_pending = 1;
> @@ -4203,13 +4197,17 @@ static void lbr_fixup(void)
>          bdw_erratum_bdf14_fixup();
>  }
>  
> -void vmx_vmenter_helper(const struct cpu_user_regs *regs)
> +int vmx_vmenter_helper(const struct cpu_user_regs *regs)

...Andy, did you want a comment here explaining what the return value is
supposed to mean? (And/or changing this to a bool?)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 6/9] x86/np2m: send flush IPIs only when a vcpu is actively using an np2m

[George Dunlap]

On 10/03/2017 04:21 PM, Sergey Dyasli wrote:
> Flush IPIs are sent to all cpus in an np2m's dirty_cpumask when
> updated.  This mask however is far too broad.  A pcpu's bit is set in
> the cpumask when a vcpu runs on that pcpu, but is only cleared when a
> flush happens.  This means that the IPI includes the current pcpu of
> vcpus that are not currently running, and also includes any pcpu that
> has ever had a vcpu use this p2m since the last flush (which in turn
> will cause spurious invalidations if a different vcpu is using an np2m).
> 
> Avoid these IPIs by keeping closer track of where an np2m is being used,
> and when a vcpu needs to be flushed:
> 
> - On schedule-out, clear v->processor in p2m->dirty_cpumask
> - Add a 'generation' counter to the p2m and nestedvcpu structs to
>   detect changes that would require re-loads on re-entry
> - On schedule-in or p2m change:
>   - Set v->processor in p2m->dirty_cpumask
>   - flush the vcpu's nested p2m pointer (and update nv->generation) if
>     the generation changed
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> v2 --> v3:
> - current pointer is now calculated only once in np2m_schedule()
> - Replaced "shadow p2m" with "np2m" for consistency in commit message

Looks good, thanks!
 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Andrew Cooper]

On 04/10/17 15:38, George Dunlap wrote:
> On 10/03/2017 04:21 PM, Sergey Dyasli wrote:
>> At the moment, the shadow EPTP value is written unconditionally in
>> ept_handle_violation().
>>
>> Instead, write the value on vmentry to the guest; but only write it if
>> the value needs updating.
>>
>> To detect this, add a flag to the nestedvcpu struct, stale_np2m, to
>> indicate when such an action is necessary.  Set it when the nested p2m
>> changes or when the np2m is flushed by an IPI, and clear it when we
>> write the new value.
>>
>> Since an IPI invalidating the p2m may happen between
>> nvmx_switch_guest() and vmx_vmenter, but we can't perform the vmwrite
>> with interrupts disabled, check the flag just before entering the
>> guest and restart the vmentry if it's set.
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Looks good to me; just one question...
>
>> ---
>> v2 --> v3:
>> - current pointer is now calculated only once in nvmx_eptp_update()
>> ---
>>  xen/arch/x86/hvm/nestedhvm.c   |  2 ++
>>  xen/arch/x86/hvm/vmx/entry.S   |  6 ++++++
>>  xen/arch/x86/hvm/vmx/vmx.c     | 14 +++++++-------
>>  xen/arch/x86/hvm/vmx/vvmx.c    | 22 ++++++++++++++++++++++
>>  xen/arch/x86/mm/p2m.c          | 10 ++++++++--
>>  xen/include/asm-x86/hvm/vcpu.h |  1 +
>>  6 files changed, 46 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
>> index f2f7469d86..74a464d162 100644
>> --- a/xen/arch/x86/hvm/nestedhvm.c
>> +++ b/xen/arch/x86/hvm/nestedhvm.c
>> @@ -56,6 +56,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
>>      nv->nv_vvmcxaddr = INVALID_PADDR;
>>      nv->nv_flushp2m = 0;
>>      nv->nv_p2m = NULL;
>> +    nv->stale_np2m = false;
>>  
>>      hvm_asid_flush_vcpu_asid(&nv->nv_n2asid);
>>  
>> @@ -107,6 +108,7 @@ nestedhvm_flushtlb_ipi(void *info)
>>       */
>>      hvm_asid_flush_core();
>>      vcpu_nestedhvm(v).nv_p2m = NULL;
>> +    vcpu_nestedhvm(v).stale_np2m = true;
>>  }
>>  
>>  void
>> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
>> index 53eedc6363..9fb8f89220 100644
>> --- a/xen/arch/x86/hvm/vmx/entry.S
>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>> @@ -79,6 +79,8 @@ UNLIKELY_END(realmode)
>>  
>>          mov  %rsp,%rdi
>>          call vmx_vmenter_helper
>> +        cmp  $0,%eax
>> +        jne .Lvmx_vmentry_restart
>>          mov  VCPU_hvm_guest_cr2(%rbx),%rax
>>  
>>          pop  %r15
>> @@ -117,6 +119,10 @@ ENTRY(vmx_asm_do_vmentry)
>>          GET_CURRENT(bx)
>>          jmp  .Lvmx_do_vmentry
>>  
>> +.Lvmx_vmentry_restart:
>> +        sti
>> +        jmp  .Lvmx_do_vmentry
>> +
>>  .Lvmx_goto_emulator:
>>          sti
>>          mov  %rsp,%rdi
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 9cfa9b6965..c9a4111267 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3249,12 +3249,6 @@ static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
>>      case 0:         // Unhandled L1 EPT violation
>>          break;
>>      case 1:         // This violation is handled completly
>> -        /*Current nested EPT maybe flushed by other vcpus, so need
>> -         * to re-set its shadow EPTP pointer.
>> -         */
>> -        if ( nestedhvm_vcpu_in_guestmode(current) &&
>> -                        nestedhvm_paging_mode_hap(current ) )
>> -            __vmwrite(EPT_POINTER, get_shadow_eptp(current));
>>          return;
>>      case -1:        // This vioaltion should be injected to L1 VMM
>>          vcpu_nestedhvm(current).nv_vmexit_pending = 1;
>> @@ -4203,13 +4197,17 @@ static void lbr_fixup(void)
>>          bdw_erratum_bdf14_fixup();
>>  }
>>  
>> -void vmx_vmenter_helper(const struct cpu_user_regs *regs)
>> +int vmx_vmenter_helper(const struct cpu_user_regs *regs)
> ...Andy, did you want a comment here explaining what the return value is
> supposed to mean? (And/or changing this to a bool?)

Definitely a comment please (especially as it is logically inverted from
what I would have expected originally).

Bool depending on whether it actually has boolean properties or not
(which will depend on how the comment ends up looking).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Sergey Dyasli]

On Wed, 2017-10-04 at 15:55 +0100, Andrew Cooper wrote:
> > >  
> > > -void vmx_vmenter_helper(const struct cpu_user_regs *regs)
> > > +int vmx_vmenter_helper(const struct cpu_user_regs *regs)
> > 
> > ...Andy, did you want a comment here explaining what the return value is
> > supposed to mean? (And/or changing this to a bool?)
> 
> Definitely a comment please (especially as it is logically inverted from
> what I would have expected originally).
> 
> Bool depending on whether it actually has boolean properties or not
> (which will depend on how the comment ends up looking).
> 
> ~Andrew

Andrew,

Are you happy with the following fixup?

diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 9fb8f89220..24265ebc08 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -80,7 +80,7 @@ UNLIKELY_END(realmode)
         mov  %rsp,%rdi
         call vmx_vmenter_helper
         cmp  $0,%eax
-        jne .Lvmx_vmentry_restart
+        je .Lvmx_vmentry_restart
         mov  VCPU_hvm_guest_cr2(%rbx),%rax
 
         pop  %r15
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c9a4111267..d9b35202f9 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4197,7 +4197,8 @@ static void lbr_fixup(void)
         bdw_erratum_bdf14_fixup();
 }
 
-int vmx_vmenter_helper(const struct cpu_user_regs *regs)
+/* Return false if the vmentry has to be restarted */
+bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     u32 new_asid, old_asid;
@@ -4206,7 +4207,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
 
     /* Shadow EPTP can't be updated here because irqs are disabled */
      if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
-         return 1;
+         return false;
 
     if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
         curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
@@ -4269,7 +4270,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
     __vmwrite(GUEST_RSP,    regs->rsp);
     __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS);
 
-    return 0;
+    return true;
 }
 
 /*

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Jan Beulich]

>>> On 05.10.17 at 10:18, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -80,7 +80,7 @@ UNLIKELY_END(realmode)
>          mov  %rsp,%rdi
>          call vmx_vmenter_helper
>          cmp  $0,%eax
> -        jne .Lvmx_vmentry_restart
> +        je .Lvmx_vmentry_restart

If you make the function return bool, the cmp above also needs
changing (and then preferably to "test %al, %al", in which case
it would then also better be "jz" instead of "je").

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Sergey Dyasli]

On Thu, 2017-10-05 at 03:27 -0600, Jan Beulich wrote:
> > > > On 05.10.17 at 10:18, <sergey.dyasli@citrix.com> wrote:
> > 
> > --- a/xen/arch/x86/hvm/vmx/entry.S
> > +++ b/xen/arch/x86/hvm/vmx/entry.S
> > @@ -80,7 +80,7 @@ UNLIKELY_END(realmode)
> >          mov  %rsp,%rdi
> >          call vmx_vmenter_helper
> >          cmp  $0,%eax
> > -        jne .Lvmx_vmentry_restart
> > +        je .Lvmx_vmentry_restart
> 
> If you make the function return bool, the cmp above also needs
> changing (and then preferably to "test %al, %al", in which case
> it would then also better be "jz" instead of "je").

Here's the updated delta:

diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 9fb8f89220..47cd674260 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -79,8 +79,8 @@ UNLIKELY_END(realmode)
 
         mov  %rsp,%rdi
         call vmx_vmenter_helper
-        cmp  $0,%eax
-        jne .Lvmx_vmentry_restart
+        test %al, %al
+        jz .Lvmx_vmentry_restart
         mov  VCPU_hvm_guest_cr2(%rbx),%rax
 
         pop  %r15
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c9a4111267..a5c2bd71cd 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4197,7 +4197,8 @@ static void lbr_fixup(void)
         bdw_erratum_bdf14_fixup();
 }
 
-int vmx_vmenter_helper(const struct cpu_user_regs *regs)
+/* Returns false if the vmentry has to be restarted */
+bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     u32 new_asid, old_asid;
@@ -4206,7 +4207,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
 
     /* Shadow EPTP can't be updated here because irqs are disabled */
      if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
-         return 1;
+         return false;
 
     if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
         curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
@@ -4269,7 +4270,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
     __vmwrite(GUEST_RSP,    regs->rsp);
     __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS);
 
-    return 0;
+    return true;
 }
 
 /*

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Andrew Cooper]

On 05/10/17 14:04, Sergey Dyasli wrote:
> On Thu, 2017-10-05 at 03:27 -0600, Jan Beulich wrote:
>>>>> On 05.10.17 at 10:18, <sergey.dyasli@citrix.com> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>> @@ -80,7 +80,7 @@ UNLIKELY_END(realmode)
>>>          mov  %rsp,%rdi
>>>          call vmx_vmenter_helper
>>>          cmp  $0,%eax
>>> -        jne .Lvmx_vmentry_restart
>>> +        je .Lvmx_vmentry_restart
>> If you make the function return bool, the cmp above also needs
>> changing (and then preferably to "test %al, %al", in which case
>> it would then also better be "jz" instead of "je").
> Here's the updated delta:
>
> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
> index 9fb8f89220..47cd674260 100644
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -79,8 +79,8 @@ UNLIKELY_END(realmode)
>  
>          mov  %rsp,%rdi
>          call vmx_vmenter_helper
> -        cmp  $0,%eax
> -        jne .Lvmx_vmentry_restart
> +        test %al, %al
> +        jz .Lvmx_vmentry_restart
>          mov  VCPU_hvm_guest_cr2(%rbx),%rax
>  
>          pop  %r15
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c9a4111267..a5c2bd71cd 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4197,7 +4197,8 @@ static void lbr_fixup(void)
>          bdw_erratum_bdf14_fixup();
>  }
>  
> -int vmx_vmenter_helper(const struct cpu_user_regs *regs)
> +/* Returns false if the vmentry has to be restarted */
> +bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
>  {
>      struct vcpu *curr = current;
>      u32 new_asid, old_asid;
> @@ -4206,7 +4207,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
>  
>      /* Shadow EPTP can't be updated here because irqs are disabled */
>       if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
> -         return 1;
> +         return false;
>  
>      if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
>          curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
> @@ -4269,7 +4270,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
>      __vmwrite(GUEST_RSP,    regs->rsp);
>      __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS);
>  
> -    return 0;
> +    return true;
>  }

With this, the whole series is Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 5/9] x86/vvmx: make updating shadow EPTP value more efficient

[Nakajima, Jun]

On 10/5/17, 6:13 AM, "Andrew Cooper" wrote:

    On 05/10/17 14:04, Sergey Dyasli wrote:
    > On Thu, 2017-10-05 at 03:27 -0600, Jan Beulich wrote:
    >>>>> On 05.10.17 at 10:18, <sergey.dyasli@citrix.com> wrote:
    >>> --- a/xen/arch/x86/hvm/vmx/entry.S
    >>> +++ b/xen/arch/x86/hvm/vmx/entry.S
    >>> @@ -80,7 +80,7 @@ UNLIKELY_END(realmode)
    >>>          mov  %rsp,%rdi
    >>>          call vmx_vmenter_helper
    >>>          cmp  $0,%eax
    >>> -        jne .Lvmx_vmentry_restart
    >>> +        je .Lvmx_vmentry_restart
    >> If you make the function return bool, the cmp above also needs
    >> changing (and then preferably to "test %al, %al", in which case
    >> it would then also better be "jz" instead of "je").
    > Here's the updated delta:
    >
    > diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
    > index 9fb8f89220..47cd674260 100644
    > --- a/xen/arch/x86/hvm/vmx/entry.S
    > +++ b/xen/arch/x86/hvm/vmx/entry.S
    > @@ -79,8 +79,8 @@ UNLIKELY_END(realmode)
    >  
    >          mov  %rsp,%rdi
    >          call vmx_vmenter_helper
    > -        cmp  $0,%eax
    > -        jne .Lvmx_vmentry_restart
    > +        test %al, %al
    > +        jz .Lvmx_vmentry_restart
    >          mov  VCPU_hvm_guest_cr2(%rbx),%rax
    >  
    >          pop  %r15
    > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
    > index c9a4111267..a5c2bd71cd 100644
    > --- a/xen/arch/x86/hvm/vmx/vmx.c
    > +++ b/xen/arch/x86/hvm/vmx/vmx.c
    > @@ -4197,7 +4197,8 @@ static void lbr_fixup(void)
    >          bdw_erratum_bdf14_fixup();
    >  }
    >  
    > -int vmx_vmenter_helper(const struct cpu_user_regs *regs)
    > +/* Returns false if the vmentry has to be restarted */
    > +bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
    >  {
    >      struct vcpu *curr = current;
    >      u32 new_asid, old_asid;
    > @@ -4206,7 +4207,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
    >  
    >      /* Shadow EPTP can't be updated here because irqs are disabled */
    >       if ( nestedhvm_vcpu_in_guestmode(curr) && vcpu_nestedhvm(curr).stale_np2m )
    > -         return 1;
    > +         return false;
    >  
    >      if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
    >          curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
    > @@ -4269,7 +4270,7 @@ int vmx_vmenter_helper(const struct cpu_user_regs *regs)
    >      __vmwrite(GUEST_RSP,    regs->rsp);
    >      __vmwrite(GUEST_RFLAGS, regs->rflags | X86_EFLAGS_MBS);
    >  
    > -    return 0;
    > +    return true;
    >  }
    
    With this, the whole series is Acked-by: Andrew Cooper
    <andrew.cooper3@citrix.com>

Acked-by: Jun Nakajima <jun.nakajima@intel.com>
  
---
Jun
Intel Open Source Technology Center
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel