diff mbox

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

Message ID 20171003152104.1432-5-sergey.dyasli@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sergey Dyasli Oct. 3, 2017, 3:20 p.m. UTC 
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(-)

Comments

George Dunlap Oct. 4, 2017, 2:26 p.m. UTC | #1 
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>
diff mbox

Patch

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().