[PATCH V10 0/5] Fix VGA logdirty related display freezes with altp2m

[PATCH V10 0/5] Fix VGA logdirty related display freezes with altp2m

[Razvan Cojocaru]

This series aims to prevent the display from freezing when
enabling altp2m and switching to a new view (and assorted problems
when resizing the display).

The series allocates a new logdirty rangeset for each new altp2m,
and propagates (under lock) changes to all p2ms.

[PATCH V10 1/5] x86/p2m: allocate logdirty_ranges for altp2m
[PATCH V10 2/5] x86/p2m: refactor p2m_reset_altp2m()
[PATCH V10 3/5] x86/altp2m: fix display frozen when switching to a new view early
[PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
[PATCH V10 5/5] p2m: change_type_range: Only invalidate mapped gfns


Thanks,
Razvan

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

[PATCH V10 1/5] x86/p2m: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

For now, only do allocation/deallocation; keeping them in sync
will be done in subsequent patches.

Logdirty synchronization will only be done for active altp2ms;
so allocate logdirty rangesets (copying the host logdirty
rangeset) when an altp2m is activated, and free it when
deactivated.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>

---
Changes since V9:
 - None.
---
 xen/arch/x86/mm/p2m.c | 46 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index fea4497..96a6d3e 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2265,6 +2265,40 @@ void p2m_flush_altp2m(struct domain *d)
     altp2m_list_unlock(d);
 }
 
+static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
+{
+    struct p2m_domain *hostp2m, *p2m;
+    int rc;
+
+    ASSERT(idx < MAX_ALTP2M);
+
+    p2m = d->arch.altp2m_p2m[idx];
+    hostp2m = p2m_get_hostp2m(d);
+
+    p2m_lock(p2m);
+
+    rc = p2m_init_logdirty(p2m);
+
+    if ( rc )
+        goto out;
+
+    /* The following is really just a rangeset copy. */
+    rc = rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
+
+    if ( rc )
+    {
+        p2m_free_logdirty(p2m);
+        goto out;
+    }
+
+    p2m_init_altp2m_ept(d, idx);
+
+ out:
+    p2m_unlock(p2m);
+
+    return rc;
+}
+
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
     int rc = -EINVAL;
@@ -2275,10 +2309,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
-    {
-        p2m_init_altp2m_ept(d, idx);
-        rc = 0;
-    }
+        rc = p2m_activate_altp2m(d, idx);
 
     altp2m_list_unlock(d);
     return rc;
@@ -2296,9 +2327,10 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
         if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
             continue;
 
-        p2m_init_altp2m_ept(d, i);
-        *idx = i;
-        rc = 0;
+        rc = p2m_activate_altp2m(d, i);
+
+        if ( !rc )
+            *idx = i;
 
         break;
     }
-- 
2.7.4


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

[PATCH V10 2/5] x86/p2m: refactor p2m_reset_altp2m()

[Razvan Cojocaru]

Refactor p2m_reset_altp2m() so that it can be used to remove
redundant codepaths, fixing the locking while we're at it.

The previous code now replaced by p2m_reset_altp2m(d, i,
ALTP2M_DEACTIVATE) calls did not set p2m->min_remapped_gfn
and p2m->max_remapped_gfn because in those cases the altp2m
idx was disabled; so before getting used again,
p2m_init_altp2m_ept() would get called, which resets them.
Always setting them in p2m_reset_altp2m(), while redundant,
is preferable to an extra conditional.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>

---
Changes since V9:
 - None.
---
 xen/arch/x86/mm/p2m.c | 57 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 96a6d3e..7c6aae7 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2247,6 +2247,36 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
     return 1;
 }
 
+enum altp2m_reset_type {
+    ALTP2M_RESET,
+    ALTP2M_DEACTIVATE
+};
+
+static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
+                             enum altp2m_reset_type reset_type)
+{
+    struct p2m_domain *p2m;
+
+    ASSERT(idx < MAX_ALTP2M);
+    p2m = d->arch.altp2m_p2m[idx];
+
+    p2m_lock(p2m);
+
+    p2m_flush_table_locked(p2m);
+
+    if ( reset_type == ALTP2M_DEACTIVATE )
+        p2m_free_logdirty(p2m);
+
+    /* Uninit and reinit ept to force TLB shootdown */
+    ept_p2m_uninit(p2m);
+    ept_p2m_init(p2m);
+
+    p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
+    p2m->max_remapped_gfn = 0;
+
+    p2m_unlock(p2m);
+}
+
 void p2m_flush_altp2m(struct domain *d)
 {
     unsigned int i;
@@ -2255,10 +2285,7 @@ void p2m_flush_altp2m(struct domain *d)
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        p2m_flush_table(d->arch.altp2m_p2m[i]);
-        /* Uninit and reinit ept to force TLB shootdown */
-        ept_p2m_uninit(d->arch.altp2m_p2m[i]);
-        ept_p2m_init(d->arch.altp2m_p2m[i]);
+        p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE);
         d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
     }
 
@@ -2357,10 +2384,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
 
         if ( !_atomic_read(p2m->active_vcpus) )
         {
-            p2m_flush_table(d->arch.altp2m_p2m[idx]);
-            /* Uninit and reinit ept to force TLB shootdown */
-            ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
-            ept_p2m_init(d->arch.altp2m_p2m[idx]);
+            p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
             d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
             rc = 0;
         }
@@ -2485,16 +2509,6 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     return rc;
 }
 
-static void p2m_reset_altp2m(struct p2m_domain *p2m)
-{
-    p2m_flush_table(p2m);
-    /* Uninit and reinit ept to force TLB shootdown */
-    ept_p2m_uninit(p2m);
-    ept_p2m_init(p2m);
-    p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
-    p2m->max_remapped_gfn = 0;
-}
-
 int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                                 mfn_t mfn, unsigned int page_order,
                                 p2m_type_t p2mt, p2m_access_t p2ma)
@@ -2528,7 +2542,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
         {
             if ( !reset_count++ )
             {
-                p2m_reset_altp2m(p2m);
+                p2m_reset_altp2m(d, i, ALTP2M_RESET);
                 last_reset_idx = i;
             }
             else
@@ -2542,10 +2556,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                          d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
                         continue;
 
-                    p2m = d->arch.altp2m_p2m[i];
-                    p2m_lock(p2m);
-                    p2m_reset_altp2m(p2m);
-                    p2m_unlock(p2m);
+                    p2m_reset_altp2m(d, i, ALTP2M_RESET);
                 }
 
                 ret = 0;
-- 
2.7.4


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

[PATCH V10 3/5] x86/altp2m: fix display frozen when switching to a new view early

[Razvan Cojocaru]

When an new altp2m view is created very early on guest boot, the
display will freeze (although the guest will run normally). This
may also happen on resizing the display. The reason is the way
Xen currently (mis)handles logdirty VGA: it intentionally
misconfigures VGA pages so that they will fault.

The problem is that it only does this in the host p2m. Once we
switch to a new altp2m, the misconfigured entries will no longer
fault, so the display will not be updated.

This patch:
* updates ept_handle_misconfig() to use the active altp2m instead
  of the hostp2m;
* modifies p2m_change_entry_type_global(),
  p2m_memory_type_changed(), p2m_change_type_range() and
  p2m_finish_type_change() to propagate their changes to all
  valid altp2ms.

With the introduction of altp2m fields in p2m_memory_type_changed()
the whole function has been put under CONFIG_HVM.

Suggested-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>

---
Changes since V9:
 - Moved the ->defer_nested_flush assignments and the
   p2m_flush_nestedp2m(d) call to p2m_change_type_range(), since
   these are per-domain (not per p2m) changes really.
---
 xen/arch/x86/mm/p2m-ept.c |   9 ++-
 xen/arch/x86/mm/p2m-pt.c  |   8 +++
 xen/arch/x86/mm/p2m.c     | 169 ++++++++++++++++++++++++++++++++++++++--------
 xen/include/asm-x86/p2m.h |   6 +-
 4 files changed, 158 insertions(+), 34 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 6e4e375..00fb82d 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -657,6 +657,9 @@ bool_t ept_handle_misconfig(uint64_t gpa)
     bool_t spurious;
     int rc;
 
+    if ( altp2m_active(curr->domain) )
+        p2m = p2m_get_altp2m(curr);
+
     p2m_lock(p2m);
 
     spurious = curr->arch.hvm.vmx.ept_spurious_misconfig;
@@ -1416,9 +1419,13 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
+    p2m->default_access = hostp2m->default_access;
+    p2m->domain = hostp2m->domain;
+
+    p2m->global_logdirty = hostp2m->global_logdirty;
     p2m->ept.ad = hostp2m->ept.ad;
     p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
-    p2m->max_remapped_gfn = 0;
+    p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0;
     ept = &p2m->ept;
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
     d->arch.altp2m_eptp[i] = ept->eptp;
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 17a6b61..b5c19df 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -29,6 +29,7 @@
 #include <xen/event.h>
 #include <xen/trace.h>
 #include <public/vm_event.h>
+#include <asm/altp2m.h>
 #include <asm/domain.h>
 #include <asm/page.h>
 #include <asm/paging.h>
@@ -464,6 +465,13 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
     struct p2m_domain *p2m = p2m_get_hostp2m(current->domain);
     int rc;
 
+    /*
+     * Should altp2m ever be enabled for NPT / shadow use, this code
+     * should be updated to make use of the active altp2m, like
+     * ept_handle_misconfig().
+     */
+    ASSERT(!altp2m_active(current->domain));
+
     p2m_lock(p2m);
     rc = do_recalc(p2m, PFN_DOWN(gpa));
     p2m_unlock(p2m);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 7c6aae7..d145850 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -277,7 +277,6 @@ int p2m_init(struct domain *d)
 int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
                           unsigned long end)
 {
-    ASSERT(p2m_is_hostp2m(p2m));
     if ( p2m->global_logdirty ||
          rangeset_contains_range(p2m->logdirty_ranges, start, end) )
         return 1;
@@ -286,31 +285,79 @@ int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
     return 0;
 }
 
+static void change_entry_type_global(struct p2m_domain *p2m,
+                                     p2m_type_t ot, p2m_type_t nt)
+{
+    p2m->change_entry_type_global(p2m, ot, nt);
+    p2m->global_logdirty = (nt == p2m_ram_logdirty);
+}
+
 void p2m_change_entry_type_global(struct domain *d,
                                   p2m_type_t ot, p2m_type_t nt)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
 
     ASSERT(ot != nt);
     ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
 
-    p2m_lock(p2m);
-    p2m->change_entry_type_global(p2m, ot, nt);
-    p2m->global_logdirty = (nt == p2m_ram_logdirty);
-    p2m_unlock(p2m);
+    p2m_lock(hostp2m);
+
+    change_entry_type_global(hostp2m, ot, nt);
+
+#ifdef CONFIG_HVM
+    if ( unlikely(altp2m_active(d)) )
+    {
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            {
+                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(altp2m);
+                change_entry_type_global(altp2m, ot, nt);
+                p2m_unlock(altp2m);
+            }
+    }
+#endif
+
+    p2m_unlock(hostp2m);
+}
+
+#ifdef CONFIG_HVM
+/* There's already a memory_type_changed() in asm/mtrr.h. */
+static void _memory_type_changed(struct p2m_domain *p2m)
+{
+    if ( p2m->memory_type_changed )
+        p2m->memory_type_changed(p2m);
 }
 
 void p2m_memory_type_changed(struct domain *d)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
 
-    if ( p2m->memory_type_changed )
+    p2m_lock(hostp2m);
+
+    _memory_type_changed(hostp2m);
+
+    if ( unlikely(altp2m_active(d)) )
     {
-        p2m_lock(p2m);
-        p2m->memory_type_changed(p2m);
-        p2m_unlock(p2m);
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            {
+                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(altp2m);
+                _memory_type_changed(altp2m);
+                p2m_unlock(altp2m);
+            }
     }
+
+    p2m_unlock(hostp2m);
 }
+#endif
 
 int p2m_set_ioreq_server(struct domain *d,
                          unsigned int flags,
@@ -956,20 +1003,14 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
 }
 
 /* Modify the p2m type of a range of gfns from ot to nt. */
-void p2m_change_type_range(struct domain *d, 
-                           unsigned long start, unsigned long end,
-                           p2m_type_t ot, p2m_type_t nt)
+static void change_type_range(struct p2m_domain *p2m,
+                              unsigned long start, unsigned long end,
+                              p2m_type_t ot, p2m_type_t nt)
 {
     unsigned long gfn = start;
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct domain *d = p2m->domain;
     int rc = 0;
 
-    ASSERT(ot != nt);
-    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
-
-    p2m_lock(p2m);
-    p2m->defer_nested_flush = 1;
-
     if ( unlikely(end > p2m->max_mapped_pfn) )
     {
         if ( !gfn )
@@ -1007,27 +1048,58 @@ void p2m_change_type_range(struct domain *d,
                rc, d->domain_id);
         domain_crash(d);
     }
+}
 
-    p2m->defer_nested_flush = 0;
+void p2m_change_type_range(struct domain *d,
+                           unsigned long start, unsigned long end,
+                           p2m_type_t ot, p2m_type_t nt)
+{
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
+
+    ASSERT(ot != nt);
+    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
+
+    p2m_lock(hostp2m);
+    hostp2m->defer_nested_flush = 1;
+
+    change_type_range(hostp2m, start, end, ot, nt);
+
+#ifdef CONFIG_HVM
+    if ( unlikely(altp2m_active(d)) )
+    {
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            {
+                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(altp2m);
+                change_type_range(altp2m, start, end, ot, nt);
+                p2m_unlock(altp2m);
+            }
+    }
+#endif
+    hostp2m->defer_nested_flush = 0;
     if ( nestedhvm_enabled(d) )
         p2m_flush_nestedp2m(d);
-    p2m_unlock(p2m);
+
+    p2m_unlock(hostp2m);
 }
 
 /*
  * Finish p2m type change for gfns which are marked as need_recalc in a range.
+ * Uses the current p2m's max_mapped_pfn to further clip the invalidation
+ * range for alternate p2ms.
  * Returns: 0/1 for success, negative for failure
  */
-int p2m_finish_type_change(struct domain *d,
-                           gfn_t first_gfn, unsigned long max_nr)
+static int finish_type_change(struct p2m_domain *p2m,
+                              gfn_t first_gfn, unsigned long max_nr)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
     unsigned long gfn = gfn_x(first_gfn);
     unsigned long last_gfn = gfn + max_nr - 1;
     int rc = 0;
 
-    p2m_lock(p2m);
-
     last_gfn = min(last_gfn, p2m->max_mapped_pfn);
     while ( gfn <= last_gfn )
     {
@@ -1042,14 +1114,51 @@ int p2m_finish_type_change(struct domain *d,
         else if ( rc < 0 )
         {
             gdprintk(XENLOG_ERR, "p2m->recalc failed! Dom%d gfn=%lx\n",
-                     d->domain_id, gfn);
+                     p2m->domain->domain_id, gfn);
             break;
         }
 
         gfn++;
     }
 
-    p2m_unlock(p2m);
+    return rc;
+}
+
+int p2m_finish_type_change(struct domain *d,
+                           gfn_t first_gfn, unsigned long max_nr)
+{
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
+    int rc;
+
+    p2m_lock(hostp2m);
+
+    rc = finish_type_change(hostp2m, first_gfn, max_nr);
+
+    if ( !rc )
+        goto out;
+
+#ifdef CONFIG_HVM
+    if ( unlikely(altp2m_active(d)) )
+    {
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            {
+                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(altp2m);
+                rc = finish_type_change(altp2m, first_gfn, max_nr);
+                p2m_unlock(altp2m);
+
+                if ( !rc )
+                    goto out;
+            }
+    }
+#endif
+
+out:
+    p2m_unlock(hostp2m);
 
     return rc;
 }
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 3304921..2095076 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -626,9 +626,6 @@ int p2m_finish_type_change(struct domain *d,
                            gfn_t first_gfn,
                            unsigned long max_nr);
 
-/* Report a change affecting memory types. */
-void p2m_memory_type_changed(struct domain *d);
-
 int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
                           unsigned long end);
 
@@ -659,6 +656,9 @@ void p2m_pod_dump_data(struct domain *d);
 
 #ifdef CONFIG_HVM
 
+/* Report a change affecting memory types. */
+void p2m_memory_type_changed(struct domain *d);
+
 /* Called by p2m code when demand-populating a PoD page */
 bool
 p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn, unsigned int order);
-- 
2.7.4


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

[PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Razvan Cojocaru]

The logdirty rangesets of the altp2ms need to be kept in sync with the
hostp2m.  This means when iterating through the altp2ms, we need to
use the host p2m to clip the rangeset, not the indiviual altp2m's
value.

This change also:

- Documents that the end is non-inclusive

- Calculates an "inclusive" value for the end once, rather than
  open-coding the modification, and (worse) back-modifying updates so
  that the calculation ends up correct

- Clarifies the logic deciding whether to call
  change_entry_type_global() or change_entry_type_range()

- Handles the case where start >= hostp2m->max_mapped_pfn

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>

---
Changes since V9:
 - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING).
 - Reused start and end in change_type_range() and removed the
   intermediary variables range_start and range_end.
 - Added an extra explanation for the if ( start > end ) return;
   code in the comment.
 - Changed ""Error %d changing Dom%d GFNs [%lx,%lx]" to
   "Error %d changing Dom%d GFNs [%lx,%lx)".
---
 xen/arch/x86/mm/p2m.c | 51 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d145850..fde2012 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1002,30 +1002,47 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
     return rc;
 }
 
-/* Modify the p2m type of a range of gfns from ot to nt. */
+/* Modify the p2m type of [start, end) from ot to nt. */
 static void change_type_range(struct p2m_domain *p2m,
                               unsigned long start, unsigned long end,
                               p2m_type_t ot, p2m_type_t nt)
 {
-    unsigned long gfn = start;
     struct domain *d = p2m->domain;
+    const unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
     int rc = 0;
 
-    if ( unlikely(end > p2m->max_mapped_pfn) )
-    {
-        if ( !gfn )
-        {
-            p2m->change_entry_type_global(p2m, ot, nt);
-            gfn = end;
-        }
-        end = p2m->max_mapped_pfn + 1;
-    }
-    if ( gfn < end )
-        rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
+    --end;
+
+    if ( start >= host_max_pfn )
+        printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n",
+               d->domain_id);
+
+    /* Always clip the rangeset down to the host p2m */
+    if ( unlikely(end > host_max_pfn) )
+        end = host_max_pfn;
+
+    /*
+     * If the requested range is out of scope, return doing nothing.
+     * Attempting to continue will crash the hypervisor in rangeset_add_range()
+     * which ASSERT()s that start <= end.
+     */
+    if ( start > end )
+        return;
+
+    /*
+     * If all valid gfns are in the invalidation range, just do a
+     * global type change. Otherwise, invalidate only the range we
+     * need.
+     */
+    if ( !start && end >= p2m->max_mapped_pfn)
+        p2m->change_entry_type_global(p2m, ot, nt);
+    else
+        rc = p2m->change_entry_type_range(p2m, ot, nt, start, end);
+
     if ( rc )
     {
-        printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
-               rc, d->domain_id, start, end - 1, ot, nt);
+        printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx) from %d to %d\n",
+               rc, d->domain_id, start, end, ot, nt);
         domain_crash(d);
     }
 
@@ -1033,11 +1050,11 @@ static void change_type_range(struct p2m_domain *p2m,
     {
     case p2m_ram_rw:
         if ( ot == p2m_ram_logdirty )
-            rc = rangeset_remove_range(p2m->logdirty_ranges, start, end - 1);
+            rc = rangeset_remove_range(p2m->logdirty_ranges, start, end);
         break;
     case p2m_ram_logdirty:
         if ( ot == p2m_ram_rw )
-            rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
+            rc = rangeset_add_range(p2m->logdirty_ranges, start, end);
         break;
     default:
         break;
-- 
2.7.4


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

[PATCH V10 5/5] p2m: change_type_range: Only invalidate mapped gfns

[Razvan Cojocaru]

change_range_type() invalidates gfn ranges to lazily change the type
of a range of gfns, and also modifies the logdirty rangesets of that
p2m. At the moment, it clips both down by the hostp2m.

While this will result in correct behavior, it's not entirely efficient,
since invalidated entries outside that range will, on fault, simply be
modified back to "empty" before faulting normally again.

Separate out the calculation of the two ranges.  Keep using the
hostp2m's max_mapped_pfn to clip the logdirty ranges, but use the
current p2m's max_mapped_pfn to further clip the invalidation range
for alternate p2ms.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>

---
Changes since V9:
 - Corrected function name in patch subject.
 - Updated the patch to take into account the changes in the
   previous two patches (no functional changes).
 - Added Jan's Reviewed-by.
---
 xen/arch/x86/mm/p2m.c | 55 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index fde2012..1907c29 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1007,8 +1007,10 @@ static void change_type_range(struct p2m_domain *p2m,
                               unsigned long start, unsigned long end,
                               p2m_type_t ot, p2m_type_t nt)
 {
+    unsigned long invalidate_start, invalidate_end;
     struct domain *d = p2m->domain;
     const unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
+    const unsigned long max_pfn = p2m->max_mapped_pfn;
     int rc = 0;
 
     --end;
@@ -1017,9 +1019,18 @@ static void change_type_range(struct p2m_domain *p2m,
         printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n",
                d->domain_id);
 
-    /* Always clip the rangeset down to the host p2m */
+    /*
+     * If we have an altp2m, the logdirty rangeset range needs to
+     * match that of the hostp2m, but for efficiency, we want to clip
+     * down the the invalidation range according to the mapped values
+     * in the altp2m.  Keep track of and clip the ranges separately.
+     */
+	invalidate_start = start;
+	invalidate_end   = end;
+
+	/* Clip down to the host p2m */
     if ( unlikely(end > host_max_pfn) )
-        end = host_max_pfn;
+        end = invalidate_end = host_max_pfn;
 
     /*
      * If the requested range is out of scope, return doing nothing.
@@ -1029,21 +1040,37 @@ static void change_type_range(struct p2m_domain *p2m,
     if ( start > end )
         return;
 
+    if ( p2m_is_altp2m(p2m) )
+        invalidate_end = min(invalidate_end, max_pfn);
+
     /*
-     * If all valid gfns are in the invalidation range, just do a
-     * global type change. Otherwise, invalidate only the range we
-     * need.
+     * If the p2m is empty, or the range is outside the currently
+     * mapped range, no need to do the invalidation; just update the
+     * rangeset.
      */
-    if ( !start && end >= p2m->max_mapped_pfn)
-        p2m->change_entry_type_global(p2m, ot, nt);
-    else
-        rc = p2m->change_entry_type_range(p2m, ot, nt, start, end);
-
-    if ( rc )
+	if ( invalidate_start < invalidate_end )
     {
-        printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx) from %d to %d\n",
-               rc, d->domain_id, start, end, ot, nt);
-        domain_crash(d);
+        /*
+         * If all valid gfns are in the invalidation range, just do a
+         * global type change.  Otherwise, invalidate only the range
+         * we need.
+         *
+         * NB that invalidate_end can't logically be >max_pfn at this
+         * point.  If this changes, the == will need to be changed to
+         * >=.
+         */
+        ASSERT(invalidate_end <= max_pfn);
+        if ( !invalidate_start && invalidate_end == max_pfn)
+            p2m->change_entry_type_global(p2m, ot, nt);
+        else
+            rc = p2m->change_entry_type_range(p2m, ot, nt,
+                                              invalidate_start, invalidate_end);
+        if ( rc )
+        {
+            printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx) from %d to %d\n",
+                   rc, d->domain_id, invalidate_start, invalidate_end, ot, nt);
+            domain_crash(d);
+        }
     }
 
     switch ( nt )
-- 
2.7.4


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

Re: [PATCH V10 5/5] p2m: change_type_range: Only invalidate mapped gfns

[Razvan Cojocaru]

On 11/28/18 11:56 PM, Razvan Cojocaru wrote:
> change_range_type() invalidates gfn ranges to lazily change the type
> of a range of gfns, and also modifies the logdirty rangesets of that
> p2m. At the moment, it clips both down by the hostp2m.
> 
> While this will result in correct behavior, it's not entirely efficient,
> since invalidated entries outside that range will, on fault, simply be
> modified back to "empty" before faulting normally again.
> 
> Separate out the calculation of the two ranges.  Keep using the
> hostp2m's max_mapped_pfn to clip the logdirty ranges, but use the
> current p2m's max_mapped_pfn to further clip the invalidation range
> for alternate p2ms.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> 
> ---
> Changes since V9:
>  - Corrected function name in patch subject.
>  - Updated the patch to take into account the changes in the
>    previous two patches (no functional changes).
>  - Added Jan's Reviewed-by.
> ---
>  xen/arch/x86/mm/p2m.c | 55 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index fde2012..1907c29 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1007,8 +1007,10 @@ static void change_type_range(struct p2m_domain *p2m,
>                                unsigned long start, unsigned long end,
>                                p2m_type_t ot, p2m_type_t nt)
>  {
> +    unsigned long invalidate_start, invalidate_end;
>      struct domain *d = p2m->domain;
>      const unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
> +    const unsigned long max_pfn = p2m->max_mapped_pfn;
>      int rc = 0;
>  
>      --end;
> @@ -1017,9 +1019,18 @@ static void change_type_range(struct p2m_domain *p2m,
>          printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n",
>                 d->domain_id);
>  
> -    /* Always clip the rangeset down to the host p2m */
> +    /*
> +     * If we have an altp2m, the logdirty rangeset range needs to
> +     * match that of the hostp2m, but for efficiency, we want to clip
> +     * down the the invalidation range according to the mapped values
> +     * in the altp2m.  Keep track of and clip the ranges separately.
> +     */
> +	invalidate_start = start;
> +	invalidate_end   = end;
> +
> +	/* Clip down to the host p2m */
>      if ( unlikely(end > host_max_pfn) )
> -        end = host_max_pfn;
> +        end = invalidate_end = host_max_pfn;

Sorry, there seem to be some indentation issues that my editor has
missed here. If the patch is otherwise acceptable and these can't be
fixed on commit I'll resend a corrected version.


Thanks,
Razvan

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

Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Jan Beulich]

>>> On 28.11.18 at 22:56, <rcojocaru@bitdefender.com> wrote:
> Changes since V9:
>  - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING).
>  - Reused start and end in change_type_range() and removed the
>    intermediary variables range_start and range_end.
>  - Added an extra explanation for the if ( start > end ) return;
>    code in the comment.

This last item isn't really taking care of the comments I gave on v9.
The _incoming_ start being larger than the _incoming_ end is
something worth to point out. But you put that check after clipping
end. Furthermore it looks like you continue to break the case
where ->max_mapped_pfn increases subsequently, i.e. you still
don't update the rangeset with the unmodified incoming values.
Or otherwise I would have expected an explanation (as a reply
to my comments, not necessarily by adding to description or
comments of the patch here) why either this is not an issue or I'm
misreading anything.

Jan



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

Re: [PATCH V10 5/5] p2m: change_type_range: Only invalidate mapped gfns

[Jan Beulich]

>>> On 28.11.18 at 22:56, <rcojocaru@bitdefender.com> wrote:
> change_range_type() invalidates gfn ranges to lazily change the type
> of a range of gfns, and also modifies the logdirty rangesets of that
> p2m. At the moment, it clips both down by the hostp2m.
> 
> While this will result in correct behavior, it's not entirely efficient,
> since invalidated entries outside that range will, on fault, simply be
> modified back to "empty" before faulting normally again.
> 
> Separate out the calculation of the two ranges.  Keep using the
> hostp2m's max_mapped_pfn to clip the logdirty ranges, but use the
> current p2m's max_mapped_pfn to further clip the invalidation range
> for alternate p2ms.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> 
> ---
> Changes since V9:
>  - Corrected function name in patch subject.

Funny: I didn't even notice the issue in the patch title. I had
pointed it out against the start of the description, where it
still exists.

Jan


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

Re: [PATCH V10 5/5] p2m: change_type_range: Only invalidate mapped gfns

[Razvan Cojocaru]

On 11/29/18 12:07 PM, Jan Beulich wrote:
>>>> On 28.11.18 at 22:56, <rcojocaru@bitdefender.com> wrote:
>> change_range_type() invalidates gfn ranges to lazily change the type
>> of a range of gfns, and also modifies the logdirty rangesets of that
>> p2m. At the moment, it clips both down by the hostp2m.
>>
>> While this will result in correct behavior, it's not entirely efficient,
>> since invalidated entries outside that range will, on fault, simply be
>> modified back to "empty" before faulting normally again.
>>
>> Separate out the calculation of the two ranges.  Keep using the
>> hostp2m's max_mapped_pfn to clip the logdirty ranges, but use the
>> current p2m's max_mapped_pfn to further clip the invalidation range
>> for alternate p2ms.
>>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: "Roger Pau Monné" <roger.pau@citrix.com>
>>
>> ---
>> Changes since V9:
>>  - Corrected function name in patch subject.
> 
> Funny: I didn't even notice the issue in the patch title. I had
> pointed it out against the start of the description, where it
> still exists.

Right, I'll correct that as well.


Thanks,
Razvan

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

[PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Razvan Cojocaru]

On 11/29/18 12:04 PM, Jan Beulich wrote:
>>>> On 28.11.18 at 22:56, <rcojocaru@bitdefender.com> wrote:
>> Changes since V9:
>>  - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING).
>>  - Reused start and end in change_type_range() and removed the
>>    intermediary variables range_start and range_end.
>>  - Added an extra explanation for the if ( start > end ) return;
>>    code in the comment.
> 
> This last item isn't really taking care of the comments I gave on v9.
> The _incoming_ start being larger than the _incoming_ end is
> something worth to point out. But you put that check after clipping
> end. Furthermore it looks like you continue to break the case
> where ->max_mapped_pfn increases subsequently, i.e. you still
> don't update the rangeset with the unmodified incoming values.
> Or otherwise I would have expected an explanation (as a reply
> to my comments, not necessarily by adding to description or
> comments of the patch here) why either this is not an issue or I'm
> misreading anything.

max_mapped_pfn _should_ end up being >= the logdirty range upper bound,
since AFAICT the logdirty ranges are tied to ept_set_entry() calls,
which always end up calling p2m_altp2m_propagate_change() when they
occur on the hostp2m (which in turn calls p2m_set_entry() on the
altp2ms, and so on).

Long story short, all modifications to the hostp2m's max_mapped_pfn will
end up updating it for all active altp2ms. The other way around is not
true if I understand the code correctly, so it is theoretically possible
for altp2m->max_mapped_pfn > hostp2m->max_mapped_pfn (although, again
AFAICT, this should not really affect the logdirty case where we're now
doing our best to keep the hostp2m in sync with altp2ms).

So this is why I believe that this is not an issue, however I might be
missing something or am (quite likely) possibly misunderstanding the
question.

Also, apologies if I'm speaking out of turn and the question was really
addressed to George (who is the proper author of the patch).


Thanks,
Razvan

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

Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Jan Beulich]

>>> On 29.11.18 at 14:23, <rcojocaru@bitdefender.com> wrote:
> On 11/29/18 12:04 PM, Jan Beulich wrote:
>>>>> On 28.11.18 at 22:56, <rcojocaru@bitdefender.com> wrote:
>>> Changes since V9:
>>>  - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING).
>>>  - Reused start and end in change_type_range() and removed the
>>>    intermediary variables range_start and range_end.
>>>  - Added an extra explanation for the if ( start > end ) return;
>>>    code in the comment.
>> 
>> This last item isn't really taking care of the comments I gave on v9.
>> The _incoming_ start being larger than the _incoming_ end is
>> something worth to point out. But you put that check after clipping
>> end. Furthermore it looks like you continue to break the case
>> where ->max_mapped_pfn increases subsequently, i.e. you still
>> don't update the rangeset with the unmodified incoming values.
>> Or otherwise I would have expected an explanation (as a reply
>> to my comments, not necessarily by adding to description or
>> comments of the patch here) why either this is not an issue or I'm
>> misreading anything.
> 
> max_mapped_pfn _should_ end up being >= the logdirty range upper bound,
> since AFAICT the logdirty ranges are tied to ept_set_entry() calls,
> which always end up calling p2m_altp2m_propagate_change() when they
> occur on the hostp2m (which in turn calls p2m_set_entry() on the
> altp2ms, and so on).

Altp2m-s don't matter here at all. My point is that the present,
unpatched p2m_change_type_range() updates the log-dirty
ranges with the unclipped [start,end), but calls
p2m->change_entry_type_range() with a possibly reduced
range. Any subsequent caller of p2m_is_logdirty_range() may
thus be mislead if the rangeset update now also used only the
clipped range.

> Also, apologies if I'm speaking out of turn and the question was really
> addressed to George (who is the proper author of the patch).

That's okay, since you've made the change in question. I did
expect George to reply to my comments, but you volunteering
to take care of addressing them is fine with me.

Jan



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

Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Razvan Cojocaru]

On 11/29/18 3:58 PM, Jan Beulich wrote:
>>>> On 29.11.18 at 14:23, <rcojocaru@bitdefender.com> wrote:
>> On 11/29/18 12:04 PM, Jan Beulich wrote:
>>>>>> On 28.11.18 at 22:56, <rcojocaru@bitdefender.com> wrote:
>>>> Changes since V9:
>>>>  - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING).
>>>>  - Reused start and end in change_type_range() and removed the
>>>>    intermediary variables range_start and range_end.
>>>>  - Added an extra explanation for the if ( start > end ) return;
>>>>    code in the comment.
>>>
>>> This last item isn't really taking care of the comments I gave on v9.
>>> The _incoming_ start being larger than the _incoming_ end is
>>> something worth to point out. But you put that check after clipping
>>> end. Furthermore it looks like you continue to break the case
>>> where ->max_mapped_pfn increases subsequently, i.e. you still
>>> don't update the rangeset with the unmodified incoming values.
>>> Or otherwise I would have expected an explanation (as a reply
>>> to my comments, not necessarily by adding to description or
>>> comments of the patch here) why either this is not an issue or I'm
>>> misreading anything.
>>
>> max_mapped_pfn _should_ end up being >= the logdirty range upper bound,
>> since AFAICT the logdirty ranges are tied to ept_set_entry() calls,
>> which always end up calling p2m_altp2m_propagate_change() when they
>> occur on the hostp2m (which in turn calls p2m_set_entry() on the
>> altp2ms, and so on).
> 
> Altp2m-s don't matter here at all. My point is that the present,
> unpatched p2m_change_type_range() updates the log-dirty
> ranges with the unclipped [start,end), but calls
> p2m->change_entry_type_range() with a possibly reduced
> range. Any subsequent caller of p2m_is_logdirty_range() may
> thus be mislead if the rangeset update now also used only the
> clipped range.

I've been reading and re-reading the code and I'm still not sure I follow:

 973     if ( unlikely(end > p2m->max_mapped_pfn) )
 974     {
 975         if ( !gfn )
 976         {
 977             p2m->change_entry_type_global(p2m, ot, nt);
 978             gfn = end;
 979         }
 980         end = p2m->max_mapped_pfn + 1;

end is being clipped here ...

 981     }
 982     if ( gfn < end )
 983         rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);

... and the if() above is not an else if(), so if ( unlikely(end >
p2m->max_mapped_pfn) ) we always clip end. What this new patch does in
that regard is just making sure it uses the hostp2m's max_mapped_pfn
instead of the altp2m's.

 984     if ( rc )
 985     {
 986         printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx]
from %d to %d\n",
 987                rc, d->domain_id, start, end - 1, ot, nt);
 988         domain_crash(d);
 989     }
 990
 991     switch ( nt )
 992     {
 993     case p2m_ram_rw:
 994         if ( ot == p2m_ram_logdirty )
 995             rc = rangeset_remove_range(p2m->logdirty_ranges, start,
end - 1);
 996         break;
 997     case p2m_ram_logdirty:
 998         if ( ot == p2m_ram_rw )
 999             rc = rangeset_add_range(p2m->logdirty_ranges, start,
end - 1);
1000         break;
1001     default:
1002         break;
1003     }

Then above it calls rangeset_remove_range() or rangeset_add_range() with
the clipped end. rangeset_add_range() ASSERT()s that start <= end, so
we've established that if ( start > end ) return; is at least healthy
for that.

I could move the if ( start > end ) return; below the
p2m->change_entry_type_global(p2m, ot, nt); call so that the code uses
the same flow as it does now. But that would only matter for the case
when start == 0 and end < 0 (which is impossible, with end being an
unsigned long).

The current code already checks if ( gfn < end ) (where gfn is start in
the new patch) before calling p2m->change_entry_type_range() (again,
with the clipped end), so in that respect it's not different at all from
the current logic.

In light of all of that, I'm reading your comment to mean that you think
that the current logic is flawed because the actual work inside
p2m_change_type_range() is done on a clipped range - so you'd like to
either have the new patch refrain from clipping anything, or an
explanation as to why this is proper behaviour (and I was wrong to pay
special attention to the if() returning early you've mentioned in your
original review). Am I correct?


Thanks,
Razvan

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

Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Jan Beulich]

>>> On 30.11.18 at 22:59, <rcojocaru@bitdefender.com> wrote:
> On 11/29/18 3:58 PM, Jan Beulich wrote:
>> Altp2m-s don't matter here at all. My point is that the present,
>> unpatched p2m_change_type_range() updates the log-dirty
>> ranges with the unclipped [start,end), but calls
>> p2m->change_entry_type_range() with a possibly reduced
>> range. Any subsequent caller of p2m_is_logdirty_range() may
>> thus be mislead if the rangeset update now also used only the
>> clipped range.
> 
> I've been reading and re-reading the code and I'm still not sure I follow:
> 
>  973     if ( unlikely(end > p2m->max_mapped_pfn) )
>  974     {
>  975         if ( !gfn )
>  976         {
>  977             p2m->change_entry_type_global(p2m, ot, nt);
>  978             gfn = end;
>  979         }
>  980         end = p2m->max_mapped_pfn + 1;
> 
> end is being clipped here ...
> 
>  981     }
>  982     if ( gfn < end )
>  983         rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
> 
> ... and the if() above is not an else if(), so if ( unlikely(end >
> p2m->max_mapped_pfn) ) we always clip end. What this new patch does in
> that regard is just making sure it uses the hostp2m's max_mapped_pfn
> instead of the altp2m's.

Oh, good point. I was focussing too much on "start", the clipping
of which is prevented by having the "gfn" local variable. And I
think current code is wrong then too (and I further think your
change then just extends badness to certain cases of "start"). So
unless this can be explained as correct behavior, I'd hope for
the situation to at least not be made worse than it is. Ideally it
would be improved, but I realize the incentive may be low as it's
presumably just a theoretical consideration.

Jan



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

Re: [PATCH V10 0/5] Fix VGA logdirty related display freezes with altp2m

[Tamas K Lengyel]

On Wed, Nov 28, 2018 at 2:57 PM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> This series aims to prevent the display from freezing when
> enabling altp2m and switching to a new view (and assorted problems
> when resizing the display).
>
> The series allocates a new logdirty rangeset for each new altp2m,
> and propagates (under lock) changes to all p2ms.
>
> [PATCH V10 1/5] x86/p2m: allocate logdirty_ranges for altp2m
> [PATCH V10 2/5] x86/p2m: refactor p2m_reset_altp2m()
> [PATCH V10 3/5] x86/altp2m: fix display frozen when switching to a new view early
> [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
> [PATCH V10 5/5] p2m: change_type_range: Only invalidate mapped gfns

Series works great and it would be very nice to have this fix
available in the 4.12 release! Thanks.

Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

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

Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Razvan Cojocaru]

On 12/3/18 10:49 AM, Jan Beulich wrote:
>>>> On 30.11.18 at 22:59, <rcojocaru@bitdefender.com> wrote:
>> On 11/29/18 3:58 PM, Jan Beulich wrote:
>>> Altp2m-s don't matter here at all. My point is that the present,
>>> unpatched p2m_change_type_range() updates the log-dirty
>>> ranges with the unclipped [start,end), but calls
>>> p2m->change_entry_type_range() with a possibly reduced
>>> range. Any subsequent caller of p2m_is_logdirty_range() may
>>> thus be mislead if the rangeset update now also used only the
>>> clipped range.
>>
>> I've been reading and re-reading the code and I'm still not sure I follow:
>>
>>  973     if ( unlikely(end > p2m->max_mapped_pfn) )
>>  974     {
>>  975         if ( !gfn )
>>  976         {
>>  977             p2m->change_entry_type_global(p2m, ot, nt);
>>  978             gfn = end;
>>  979         }
>>  980         end = p2m->max_mapped_pfn + 1;
>>
>> end is being clipped here ...
>>
>>  981     }
>>  982     if ( gfn < end )
>>  983         rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
>>
>> ... and the if() above is not an else if(), so if ( unlikely(end >
>> p2m->max_mapped_pfn) ) we always clip end. What this new patch does in
>> that regard is just making sure it uses the hostp2m's max_mapped_pfn
>> instead of the altp2m's.
> 
> Oh, good point. I was focussing too much on "start", the clipping
> of which is prevented by having the "gfn" local variable. And I
> think current code is wrong then too (and I further think your
> change then just extends badness to certain cases of "start"). So
> unless this can be explained as correct behavior, I'd hope for
> the situation to at least not be made worse than it is. Ideally it
> would be improved, but I realize the incentive may be low as it's
> presumably just a theoretical consideration.

Right, so you're saying that the series would be able to go in provided
that the situation is not made worse than it currently is.

As far as I can tell, the patch changes nothing functionally from the
current state of affairs: when called for the hostp2m it behaves exactly
the same.

The one difference is the early return, which is certainly not making
things worse: rangeset_add_range() will crash the hypervisor in an
ASSERT(start <= end). It just happened that that never occured for the
hostp2m - so it will continue to not happen for the hostp2m.

Could you please provide more details on the case that is making things
worse? Which cases of "start" (if that is what you mean) make things worse?


Thanks,
Razvan

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

Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Jan Beulich]

>>> On 04.12.18 at 13:18, <rcojocaru@bitdefender.com> wrote:
> Right, so you're saying that the series would be able to go in provided
> that the situation is not made worse than it currently is.

Well, I'm not the maintainer of this code, so in principle the
series can go in despite my reservations.

> As far as I can tell, the patch changes nothing functionally from the
> current state of affairs: when called for the hostp2m it behaves exactly
> the same.
> 
> The one difference is the early return, which is certainly not making
> things worse: rangeset_add_range() will crash the hypervisor in an
> ASSERT(start <= end). It just happened that that never occured for the
> hostp2m - so it will continue to not happen for the hostp2m.
> 
> Could you please provide more details on the case that is making things
> worse? Which cases of "start" (if that is what you mean) make things worse?

Okay, I've looked again - start indeed doesn't get clipped. Yet the
added return path plus the comment next to it still make the
situation worse imo, as they further the impression that the clipping
of end is correct. Or in other words, with the change it'll be less
visible that there's a (perhaps just theoretical, as said) bug here.
Hence my desire to get the bug addressed while this code is
being basically re-done anyway.

Jan



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

Re: [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets

[Razvan Cojocaru]

On 12/4/18 2:54 PM, Jan Beulich wrote:
>>>> On 04.12.18 at 13:18, <rcojocaru@bitdefender.com> wrote:
>> Right, so you're saying that the series would be able to go in provided
>> that the situation is not made worse than it currently is.
> 
> Well, I'm not the maintainer of this code, so in principle the
> series can go in despite my reservations.
> 
>> As far as I can tell, the patch changes nothing functionally from the
>> current state of affairs: when called for the hostp2m it behaves exactly
>> the same.
>>
>> The one difference is the early return, which is certainly not making
>> things worse: rangeset_add_range() will crash the hypervisor in an
>> ASSERT(start <= end). It just happened that that never occured for the
>> hostp2m - so it will continue to not happen for the hostp2m.
>>
>> Could you please provide more details on the case that is making things
>> worse? Which cases of "start" (if that is what you mean) make things worse?
> 
> Okay, I've looked again - start indeed doesn't get clipped. Yet the
> added return path plus the comment next to it still make the
> situation worse imo, as they further the impression that the clipping
> of end is correct. Or in other words, with the change it'll be less
> visible that there's a (perhaps just theoretical, as said) bug here.
> Hence my desire to get the bug addressed while this code is
> being basically re-done anyway.

Understood.

The original Xen code seems to be in patch
437f54d3a33d3787a7cc485eb2b3451e8be49ca7 ("x86/EPT: don't walk page
tables when changing types on a range") which does clip always clip
"end" as pointed out previously, and
90ac32559bfbd08127638ba13f99b5ed565cfc2b ("x86/EPT: don't walk entire
page tables when globally changing types"), which adds code specifically
for logdirty_ranges.

The always-on clipping is done in the first of the two commits (the
previous code just did "for ( gfn = start; gfn < end; ) [...]
p2m_set_entry(p2m, gfn, mfn, order, nt, a);"). The only reference to
max_mapped_pfn is in p2m_change_type_range(). The change appears to be
related to the introduction of ept_change_entry_type_range(). I am not,
unfortunately, familiar with that code and can't tell for sure yet
whether the optimization is also a potential bug. It certainly doesn't
look like clipping end is necessary.

In the interest of moving forward, and since this patch is called
"Always use hostp2m when clipping rangesets" and doesn't appear to make
things worse, I suggest that I clean up the series and send out V11
(with the newly added Tested-by from Tamas), and - also depending on
input from George - we can revisit the max_mapped_pfn clipping
discussion after we at least end up fixing the stuck display with altp2m
that basically prevents Tamas and us from using altp2m.


Thanks,
Razvan

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