[PATCH RFC 00/12] Nested p2m: allow sharing between vCPUs

[PATCH RFC 00/12] 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 tables 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 arbitrary number of vCPUs per guest).

Known issues in current implementation:

    * AMD's nested SVM is likely broken. Unfortunately, I don't have any
      H/W currently to perform a proper testing.

Sergey Dyasli (12):
  x86/np2m: refactor p2m_get_nestedp2m()
  x86/np2m: add np2m_flush_eptp()
  x86/vvmx: use np2m_flush_eptp() for INVEPT_SINGLE_CONTEXT
  x86/np2m: remove np2m_base from p2m_get_nestedp2m()
  x86/np2m: add np2m_generation
  x86/vvmx: add stale_eptp flag
  x86/np2m: add np2m_schedule_in/out()
  x86/np2m: add p2m_get_nestedp2m_locked()
  x86/np2m: improve nestedhvm_hap_nested_page_fault()
  x86/np2m: implement sharing of np2m between vCPUs
  x86/np2m: add break to np2m_flush_eptp()
  x86/vvmx: remove EPTP write from ept_handle_violation()

 xen/arch/x86/domain.c              |   2 +
 xen/arch/x86/hvm/nestedhvm.c       |   2 +
 xen/arch/x86/hvm/svm/nestedsvm.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        |  29 ++++--
 xen/arch/x86/mm/hap/nested_hap.c   |  29 +++---
 xen/arch/x86/mm/p2m.c              | 180 +++++++++++++++++++++++++++++++------
 xen/include/asm-x86/hvm/vcpu.h     |   1 +
 xen/include/asm-x86/hvm/vmx/vvmx.h |   2 +
 xen/include/asm-x86/p2m.h          |  15 +++-
 11 files changed, 218 insertions(+), 64 deletions(-)

-- 
2.11.0


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

[PATCH RFC 01/12] 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>
---
 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 e8a57d118c..b8c8bba421 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1773,14 +1773,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 */
@@ -1790,7 +1800,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 ) 
@@ -1798,15 +1807,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);
@@ -1817,11 +1824,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 6395e8fd1d..9086bb35dc 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 RFC 02/12] x86/np2m: add np2m_flush_eptp()

[Sergey Dyasli]

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);
+}
+
 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);
-- 
2.11.0


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

[PATCH RFC 03/12] x86/vvmx: use np2m_flush_eptp() for INVEPT_SINGLE_CONTEXT

[Sergey Dyasli]

nvmx_handle_invept() updates current's np2m just to flush it. Instead,
use the new np2m_flush_eptp() directly for this purpose.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 2c8cf637a8..56678127e1 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1895,12 +1895,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_eptp(current, eptp);
         break;
     }
     case INVEPT_ALL_CONTEXT:
-- 
2.11.0


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

[PATCH RFC 04/12] x86/np2m: remove np2m_base from p2m_get_nestedp2m()

[Sergey Dyasli]

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

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 2 +-
 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, 9 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 8fd9c23a02..c3665aec01 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -411,7 +411,7 @@ 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);
+    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 56678127e1..1011829c15 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1094,8 +1094,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 bc330d8f52..e7bd0dbac8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1815,11 +1815,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);
@@ -1867,7 +1868,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,
@@ -1882,13 +1883,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 0e39999387..cc1bab9eb7 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 EPTP in VMCS12 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 RFC 05/12] x86/np2m: add np2m_generation

[Sergey Dyasli]

Add np2m_generation variable to both p2m_domain and nestedvcpu.

np2m's generation will be incremented each time the np2m is flushed.
This will allow to detect if a nested vcpu has the stale np2m.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/nestedhvm.c   | 1 +
 xen/arch/x86/mm/p2m.c          | 3 +++
 xen/include/asm-x86/hvm/vcpu.h | 1 +
 xen/include/asm-x86/p2m.h      | 1 +
 4 files changed, 6 insertions(+)

diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
index f2f7469d86..32b8acca6a 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->np2m_generation = 0;
 
     hvm_asid_flush_vcpu_asid(&nv->nv_n2asid);
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e7bd0dbac8..4fc2d94b46 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);
@@ -1811,6 +1813,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);
 }
 
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index 6c54773f1c..91651581db 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 */
+    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 cc1bab9eb7..eedc7fd412 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 
-- 
2.11.0


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

[PATCH RFC 06/12] x86/vvmx: add stale_eptp flag

[Sergey Dyasli]

The new variable will indicate if update of a shadow EPTP is needed
prior to vmentry. Update is required if a nested vcpu gets a new np2m
or if its np2m was flushed by an IPI.

Helper function nvcpu_flush() is added.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/nestedhvm.c       |  1 +
 xen/arch/x86/hvm/vmx/entry.S       |  6 ++++++
 xen/arch/x86/hvm/vmx/vmx.c         |  8 +++++++-
 xen/arch/x86/hvm/vmx/vvmx.c        | 15 +++++++++++++++
 xen/arch/x86/mm/p2m.c              | 10 ++++++++--
 xen/include/asm-x86/hvm/vmx/vvmx.h |  2 ++
 6 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
index 32b8acca6a..e9b1d8e628 100644
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -108,6 +108,7 @@ nestedhvm_flushtlb_ipi(void *info)
      */
     hvm_asid_flush_core();
     vcpu_nestedhvm(v).nv_p2m = NULL;
+    vcpu_2_nvmx(v).stale_eptp = true;
 }
 
 void
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 9f1755b31c..5480206cac 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -77,6 +77,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
@@ -115,6 +117,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 69ce3aae25..35aa57e24f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4236,13 +4236,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_2_nvmx(curr).stale_eptp )
+         return 1;
+
     if ( curr->domain->arch.hvm_domain.pi_ops.do_resume )
         curr->domain->arch.hvm_domain.pi_ops.do_resume(curr);
 
@@ -4303,6 +4307,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 1011829c15..7b193767cd 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -120,6 +120,7 @@ int nvmx_vcpu_initialise(struct vcpu *v)
     nvmx->iobitmap[1] = NULL;
     nvmx->msrbitmap = NULL;
     INIT_LIST_HEAD(&nvmx->launched_list);
+    nvmx->stale_eptp = false;
     return 0;
 }
  
@@ -1390,12 +1391,26 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
     vmsucceed(regs);
 }
 
+static void nvmx_eptp_update(void)
+{
+    if ( !nestedhvm_vcpu_in_guestmode(current) ||
+          vcpu_nestedhvm(current).nv_vmexit_pending ||
+         !vcpu_2_nvmx(current).stale_eptp ||
+         !nestedhvm_paging_mode_hap(current) )
+        return;
+
+    __vmwrite(EPT_POINTER, get_shadow_eptp(current));
+    vcpu_2_nvmx(current).stale_eptp = false;
+}
+
 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 4fc2d94b46..3d65899b05 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1817,6 +1817,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_2_nvmx(v).stale_eptp = true;
+}
+
 struct p2m_domain *
 p2m_get_nestedp2m(struct vcpu *v)
 {
@@ -1840,7 +1846,7 @@ p2m_get_nestedp2m(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);
             p2m_unlock(p2m);
@@ -1857,7 +1863,7 @@ p2m_get_nestedp2m(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);
     p2m_unlock(p2m);
     nestedp2m_unlock(d);
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index 3285b03bbb..ddc2569f64 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -51,6 +51,8 @@ struct nestedvmx {
     } ept;
     uint32_t guest_vpid;
     struct list_head launched_list;
+
+    bool stale_eptp; /* True, when EPTP in the shadow VMCS is no longer valid */
 };
 
 #define vcpu_2_nvmx(v)	(vcpu_nestedhvm(v).u.nvmx)
-- 
2.11.0


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

[PATCH RFC 07/12] x86/np2m: add np2m_schedule_in/out()

[Sergey Dyasli]

np2m maintenance is required for a nested vcpu during scheduling:

    1. On schedule-out: clear pCPU's bit in p2m->dirty_cpumask
                        to prevent useless IPIs.

    2. On schedule-in: check if np2m is up to date and wasn't flushed.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/mm/p2m.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/p2m.h |  3 +++
 2 files changed, 55 insertions(+)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 3d65899b05..4b83d4a4f1 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1880,6 +1880,58 @@ p2m_get_p2m(struct vcpu *v)
     return p2m_get_nestedp2m(v);
 }
 
+static void np2m_schedule(bool sched_out)
+{
+    struct nestedvcpu *nv = &vcpu_nestedhvm(current);
+    struct p2m_domain *p2m;
+    bool sched_in = !sched_out;
+
+    if ( !nestedhvm_enabled(current->domain) ||
+         !nestedhvm_vcpu_in_guestmode(current) ||
+         !nestedhvm_paging_mode_hap(current) )
+        return;
+
+    p2m = nv->nv_p2m;
+    if ( p2m )
+    {
+        bool np2m_valid;
+
+        p2m_lock(p2m);
+        np2m_valid = p2m->np2m_base == nhvm_vcpu_p2m_base(current) &&
+                     nv->np2m_generation == p2m->np2m_generation;
+        if ( sched_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(current->processor, p2m->dirty_cpumask);
+        }
+        else if ( sched_in )
+        {
+            if ( !np2m_valid )
+            {
+                /* This vCPU's np2m was flushed while it was not runnable */
+                hvm_asid_flush_core();
+                vcpu_nestedhvm(current).nv_p2m = NULL;
+            }
+            else
+                cpumask_set_cpu(current->processor, p2m->dirty_cpumask);
+        }
+        p2m_unlock(p2m);
+    }
+}
+
+void np2m_schedule_out(void)
+{
+    np2m_schedule(true);
+}
+
+void np2m_schedule_in(void)
+{
+    np2m_schedule(false);
+}
+
 unsigned long paging_gva_to_gfn(struct vcpu *v,
                                 unsigned long va,
                                 uint32_t *pfec)
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index eedc7fd412..801a11a960 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -370,6 +370,9 @@ struct p2m_domain *p2m_get_nestedp2m(struct vcpu *v);
  */
 struct p2m_domain *p2m_get_p2m(struct vcpu *v);
 
+void np2m_schedule_out(void);
+void np2m_schedule_in(void);
+
 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 RFC 08/12] x86/np2m: add p2m_get_nestedp2m_locked()

[Sergey Dyasli]

The new function returns still write-locked np2m.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/mm/p2m.c     | 12 +++++++++---
 xen/include/asm-x86/p2m.h |  2 ++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 4b83d4a4f1..364fdd8c13 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1824,7 +1824,7 @@ static void nvcpu_flush(struct vcpu *v)
 }
 
 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;
@@ -1849,7 +1849,6 @@ p2m_get_nestedp2m(struct vcpu *v)
                 nvcpu_flush(v);
             p2m->np2m_base = np2m_base;
             assign_np2m(v, p2m);
-            p2m_unlock(p2m);
             nestedp2m_unlock(d);
 
             return p2m;
@@ -1865,12 +1864,19 @@ p2m_get_nestedp2m(struct vcpu *v)
     p2m->np2m_base = np2m_base;
     nvcpu_flush(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 801a11a960..936d1142c8 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -364,6 +364,8 @@ struct p2m_domain {
  * Updates vCPU's n2pm to match its EPTP in VMCS12 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 RFC 09/12] x86/np2m: improve 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(). Simply
use p2m_get_nestedp2m_lock() to guarantee that correct np2m is used.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/mm/hap/nested_hap.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index ed137fa784..96afe632b5 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -101,28 +101,21 @@ 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);
+    gfn = (L2_gpa >> PAGE_SHIFT) & mask;
+    mfn = _mfn((L0_gpa >> PAGE_SHIFT) & mask);
 
-        rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
-    }
+    rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
 
     p2m_unlock(p2m);
 
@@ -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,6 +270,7 @@ 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);
 
-- 
2.11.0


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

[PATCH RFC 10/12] x86/np2m: implement sharing of np2m between vCPUs

[Sergey Dyasli]

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

np2m_schedule_in/out() callbacks are added to context_switch() as well
as pseudo schedule-out is performed during virtual_vmexit().

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/domain.c       |  2 ++
 xen/arch/x86/hvm/vmx/vvmx.c |  4 ++++
 xen/arch/x86/mm/p2m.c       | 29 +++++++++++++++++++++++++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index dd8bf1302f..38c86a5ded 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1642,6 +1642,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
     {
         _update_runstate_area(prev);
         vpmu_switch_from(prev);
+        np2m_schedule_out();
     }
 
     if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm_vcpu.tm_list) )
@@ -1690,6 +1691,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 
         /* Must be done with interrupts enabled */
         vpmu_switch_to(next);
+        np2m_schedule_in();
     }
 
     /* Ensure that the vcpu has an up-to-date time base. */
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 7b193767cd..2203d541ea 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1187,6 +1187,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));
@@ -1353,6 +1354,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_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 364fdd8c13..480459ae51 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1830,6 +1830,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);
@@ -1843,10 +1844,34 @@ 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 )
         {
-            if ( p2m->np2m_base == P2M_BASE_EADDR )
+            /* Check if np2m was flushed just before the lock */
+            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 if ( p2m->np2m_base != P2M_BASE_EADDR )
+        {
+            /* vCPU is switching from some other valid np2m */
+            cpumask_clear_cpu(v->processor, p2m->dirty_cpumask);
+        }
+        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);
-- 
2.11.0


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

[PATCH RFC 11/12] 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>
---
 xen/arch/x86/mm/p2m.c     | 3 +++
 xen/include/asm-x86/p2m.h | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 480459ae51..d0a2aef1f2 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1796,7 +1796,10 @@ void np2m_flush_eptp(struct vcpu *v, unsigned long eptp)
         p2m = d->arch.nested_p2m[i];
         p2m_lock(p2m);
         if ( p2m->np2m_base == eptp )
+        {
             p2m_flush_table_locked(p2m);
+            break;
+        }
         else
             p2m_unlock(p2m);
     }
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 936d1142c8..7cc44cc496 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -784,7 +784,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 eptp */
+/* Flushes the np2m specified by eptp (if it exists) */
 void np2m_flush_eptp(struct vcpu *v, unsigned long eptp);
 
 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

[PATCH RFC 12/12] x86/vvmx: remove EPTP write from ept_handle_violation()

[Sergey Dyasli]

Now there is no need to update shadow EPTP after handling L2 EPT
violation since all EPTP updates are handled by nvmx_eptp_update().

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 35aa57e24f..3a3e04bb0f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3282,12 +3282,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;
-- 
2.11.0


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

Re: [PATCH RFC 02/12] x86/np2m: add np2m_flush_eptp()

[Egger, Christoph]

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC 02/12] x86/np2m: add np2m_flush_eptp()

[Sergey Dyasli]

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH RFC 02/12] x86/np2m: add np2m_flush_eptp()

[Jan Beulich]

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


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

Re: [PATCH RFC 02/12] x86/np2m: add np2m_flush_eptp()

[George Dunlap]

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

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

Re: [PATCH RFC 05/12] x86/np2m: add np2m_generation

[George Dunlap]

On 07/18/2017 11:34 AM, Sergey Dyasli wrote:
> Add np2m_generation variable to both p2m_domain and nestedvcpu.

s/variable/element;

BTW still trying to get a feel for the whole series.  Patches w/o
comments (or with minor comments like this) look plausible but I won't
know what I think of the whole series until the end.

 -George

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

Re: [PATCH RFC 06/12] x86/vvmx: add stale_eptp flag

[George Dunlap]

On 07/18/2017 11:34 AM, Sergey Dyasli wrote:
> The new variable will indicate if update of a shadow EPTP is needed

*element

> prior to vmentry. Update is required if a nested vcpu gets a new np2m
> or if its np2m was flushed by an IPI.
> 
> Helper function nvcpu_flush() is added.

Passive voice in this situation is to be avoided. :-)

We normally say something like, "Add nvcpu_flush() helper function."

> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/nestedhvm.c       |  1 +
>  xen/arch/x86/hvm/vmx/entry.S       |  6 ++++++
>  xen/arch/x86/hvm/vmx/vmx.c         |  8 +++++++-
>  xen/arch/x86/hvm/vmx/vvmx.c        | 15 +++++++++++++++
>  xen/arch/x86/mm/p2m.c              | 10 ++++++++--
>  xen/include/asm-x86/hvm/vmx/vvmx.h |  2 ++
>  6 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/nestedhvm.c b/xen/arch/x86/hvm/nestedhvm.c
> index 32b8acca6a..e9b1d8e628 100644
> --- a/xen/arch/x86/hvm/nestedhvm.c
> +++ b/xen/arch/x86/hvm/nestedhvm.c
> @@ -108,6 +108,7 @@ nestedhvm_flushtlb_ipi(void *info)
>       */
>      hvm_asid_flush_core();
>      vcpu_nestedhvm(v).nv_p2m = NULL;
> +    vcpu_2_nvmx(v).stale_eptp = true;

Looks like a vmx-specific function in common code?

 -George


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

Re: [PATCH RFC 07/12] x86/np2m: add np2m_schedule_in/out()

[George Dunlap]

On 07/18/2017 11:34 AM, Sergey Dyasli wrote:
> np2m maintenance is required for a nested vcpu during scheduling:
> 
>     1. On schedule-out: clear pCPU's bit in p2m->dirty_cpumask
>                         to prevent useless IPIs.
> 
>     2. On schedule-in: check if np2m is up to date and wasn't flushed.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/mm/p2m.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/p2m.h |  3 +++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 3d65899b05..4b83d4a4f1 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1880,6 +1880,58 @@ p2m_get_p2m(struct vcpu *v)
>      return p2m_get_nestedp2m(v);
>  }
>  
> +static void np2m_schedule(bool sched_out)
> +{
> +    struct nestedvcpu *nv = &vcpu_nestedhvm(current);
> +    struct p2m_domain *p2m;
> +    bool sched_in = !sched_out;
> +
> +    if ( !nestedhvm_enabled(current->domain) ||
> +         !nestedhvm_vcpu_in_guestmode(current) ||
> +         !nestedhvm_paging_mode_hap(current) )
> +        return;
> +
> +    p2m = nv->nv_p2m;
> +    if ( p2m )
> +    {
> +        bool np2m_valid;
> +
> +        p2m_lock(p2m);
> +        np2m_valid = p2m->np2m_base == nhvm_vcpu_p2m_base(current) &&
> +                     nv->np2m_generation == p2m->np2m_generation;
> +        if ( sched_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(current->processor, p2m->dirty_cpumask);
> +        }
> +        else if ( sched_in )
> +        {
> +            if ( !np2m_valid )
> +            {
> +                /* This vCPU's np2m was flushed while it was not runnable */
> +                hvm_asid_flush_core();
> +                vcpu_nestedhvm(current).nv_p2m = NULL;
> +            }
> +            else
> +                cpumask_set_cpu(current->processor, p2m->dirty_cpumask);
> +        }
> +        p2m_unlock(p2m);
> +    }
> +}

This level of sharing seems a tad excessive to me; but if we're going to
do it, I think it would be more clear if the callers called a function
called np2m_schedule() with `dir`, then define things something like this:

#define NP2M_SCHEDLE_IN  0
#define NP2M_SCHEDLE_OUT 1

 -George

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

Re: [PATCH RFC 10/12] x86/np2m: implement sharing of np2m between vCPUs

[George Dunlap]

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

On 07/18/2017 11:34 AM, Sergey Dyasli wrote:
> Modify p2m_get_nestedp2m() to allow sharing a np2m between multiple
> vcpus with the same np2m_base (L1 EPTP value in VMCS12).
> 
> np2m_schedule_in/out() callbacks are added to context_switch() as well
> as pseudo schedule-out is performed during virtual_vmexit().
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/domain.c       |  2 ++
>  xen/arch/x86/hvm/vmx/vvmx.c |  4 ++++
>  xen/arch/x86/mm/p2m.c       | 29 +++++++++++++++++++++++++++--
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index dd8bf1302f..38c86a5ded 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1642,6 +1642,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>      {
>          _update_runstate_area(prev);
>          vpmu_switch_from(prev);
> +        np2m_schedule_out();
>      }
>  
>      if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm_vcpu.tm_list) )
> @@ -1690,6 +1691,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>  
>          /* Must be done with interrupts enabled */
>          vpmu_switch_to(next);
> +        np2m_schedule_in();
>      }
>  
>      /* Ensure that the vcpu has an up-to-date time base. */
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 7b193767cd..2203d541ea 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1187,6 +1187,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));
> @@ -1353,6 +1354,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_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 364fdd8c13..480459ae51 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1830,6 +1830,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);
> @@ -1843,10 +1844,34 @@ 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 )
>          {
> -            if ( p2m->np2m_base == P2M_BASE_EADDR )
> +            /* Check if np2m was flushed just before the lock */
> +            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);

This function now has three copies of the above four lines.  What about
folding in something like the attached?

 -George

[-- Attachment #2: code-sharing.diff --]
[-- Type: text/x-patch, Size: 1852 bytes --]

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 480459ae51..5804a8819b 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1831,6 +1831,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 vcpu_flush = true;
 
     /* Mask out low bits; this avoids collisions with P2M_BASE_EADDR */
     np2m_base &= ~(0xfffull);
@@ -1847,14 +1848,9 @@ 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);
-            /* np2m is up-to-date */
-            p2m->np2m_base = np2m_base;
-            assign_np2m(v, p2m);
-            nestedp2m_unlock(d);
-
-            return p2m;
+            if ( nv->np2m_generation == p2m->np2m_generation )
+                needs_flush = false;
+            goto found;
         }
         else if ( p2m->np2m_base != P2M_BASE_EADDR )
         {
@@ -1869,15 +1865,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);
     }
 
@@ -1886,8 +1877,10 @@ 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);
 

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

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

Re: [PATCH RFC 00/12] Nested p2m: allow sharing between vCPUs

[George Dunlap]

On 07/18/2017 11:34 AM, Sergey Dyasli wrote:
> 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 tables 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 arbitrary number of vCPUs per guest).

On the whole this looks like a decent approach.

Were you planning on re-sending it with the RFC removed, or would you
like me to do a detailed review of this series as it is?

 -George

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

Re: [PATCH RFC 00/12] Nested p2m: allow sharing between vCPUs

[Sergey Dyasli]

On Mon, 2017-08-28 at 18:03 +0100, George Dunlap wrote:
> On 07/18/2017 11:34 AM, Sergey Dyasli wrote:
> > 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 tables 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 arbitrary number of vCPUs per guest).
> 
> On the whole this looks like a decent approach.
> 
> Were you planning on re-sending it with the RFC removed, or would you
> like me to do a detailed review of this series as it is?

Thanks for review! My current plan is to re-send the series as v1 after
addressing your and Christoph's comments. Let's wait for that before
detailed review :)

Oh and there is a possibility I may do some AMD testing.

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