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

[PATCH V11 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 introduces p2m_{init,free}_logdirty(), allocates a new
logdirty rangeset for each new altp2m, and  propagates (under lock)
changes to all p2ms.

[PATCH V11 1/5] x86/p2m: allocate logdirty_ranges for altp2ms
[PATCH V11 2/5] x86/p2m: refactor p2m_reset_altp2m()
[PATCH V11 3/5] x86/altp2m: fix display frozen when switching to a new view early
[PATCH V11 4/5] p2m: Always use hostp2m when clipping rangesets
[PATCH V11 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 V11 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>
Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
 - Added Tamas' Tested-by.
---
 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 V11 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>
Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
 - Added Tamas' Tested-by.
---
 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 V11 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>
Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
 - Added Tamas' Tested-by.
---
 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 V11 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>
Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
 - Fixed a double-space in the patch description.
 - Fixed a coding style issue for
   "if ( !start && end >= p2m->max_mapped_pfn)" (no space before
   closing ')').
 - Switched the early return comment back to "/* If the requested
   range is out of scope, return doing nothing. */.
 - Added Tamas' Tested-by.
---
 xen/arch/x86/mm/p2m.c | 47 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d145850..539ea16 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1002,30 +1002,43 @@ 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. */
+    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 +1046,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 V11 5/5] p2m: change_type_range: Only invalidate mapped gfns

[Razvan Cojocaru]

change_type_range() 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>
Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
 - Corrected function name in patch description.
 - Fixed indentation issues, removed some double spaces from
   comments.
 - Added Tamas' Tested-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 539ea16..7fbb8ed 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,29 +1019,54 @@ 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. */
     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 V11 4/5] p2m: Always use hostp2m when clipping rangesets

[Jan Beulich]

>>> On 05.12.18 at 10:18, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1002,30 +1002,43 @@ 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. */
> +    if ( start > end )
> +        return;

My prior comment remains: Even if there's no change in behavior
(and you avoid the assertion), especially due to the comment the
impression results (at least to me) that all is well here, when it
really is a (latent) bug to "do nothing" in this case. George, so far
this was a discussion between Razvan and me - do you have an
opinion either way here?

Jan



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

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

[Razvan Cojocaru]

On 12/5/18 6:30 PM, Jan Beulich wrote:
>>>> On 05.12.18 at 10:18, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1002,30 +1002,43 @@ 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. */
>> +    if ( start > end )
>> +        return;
> 
> My prior comment remains: Even if there's no change in behavior
> (and you avoid the assertion), especially due to the comment the
> impression results (at least to me) that all is well here, when it
> really is a (latent) bug to "do nothing" in this case. George, so far
> this was a discussion between Razvan and me - do you have an
> opinion either way here?

Obviously I can't speak for George, but to reiterate my previous
analysis, it looks like this patch has added the clipping:

http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=437f54d3a33d3787a7cc485eb2b3451e8be49ca7

and this patch has added the global_logdirty ranges code:

http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=90ac32559bfbd08127638ba13f99b5ed565cfc2b

but left the clipping code in (possibly accidentally). You may have some
insight into that, being their author, although it's been a few years
since then.

For my own part, I see no reason why not clipping end should not work
when updating the ranges only (as long as start continues to be <=
unclipped_end).

Would that modification + testing of it help this series continue?


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 13.12.18 at 11:22, <rcojocaru@bitdefender.com> wrote:
> For my own part, I see no reason why not clipping end should not work
> when updating the ranges only (as long as start continues to be <=
> unclipped_end).
> 
> Would that modification + testing of it help this series continue?

I think so, at least as far as I'm concerned. But I think we really need
George's opinion as well.

Jan



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

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

[George Dunlap]

On 12/13/18 10:43 AM, Jan Beulich wrote:
>>>> On 13.12.18 at 11:22, <rcojocaru@bitdefender.com> wrote:
>> For my own part, I see no reason why not clipping end should not work
>> when updating the ranges only (as long as start continues to be <=
>> unclipped_end).
>>
>> Would that modification + testing of it help this series continue?
> 
> I think so, at least as far as I'm concerned. But I think we really need
> George's opinion as well.

We are going off into the weeds a little bit here I think.

If I understand Jan's concern properly, he's concerned about a situation
like this:

[start] p2m->max_mapped_pfn == 0xfff
1. change_type_range ram => logdirty, [0x900, 0x1200)

Obviously the actual p2m entries can only be changed from 0x900 to
0xfff; but what about the logdirty ranges?  At the moment, the result
will be a rangeset with [0x900, 0xfff].

Jan is asking whether the rangeset should instead be [0x900, 0x11ff].

So the time when it would matter would be a situation like the following:

2. p2m_set_entry(0x1100, M)

3. change_entry_type_global(ram => logdirty)

4. change_entry_type_global(logdirty => ram)

Under the current regime gfn 0x1100 would be have type ram_rw both after
step 2, and after step 4.

If we used Jan's suggestion, then it would be marked as ram_rw after
step 2, and logdirty after step 4.

But of course that's no different than what would happen if
max_mapped_pfn were 0x2000, but gfns 0x1000-11ff just happened to be empty.

Under normal circumstances, neither of these situations should happen;
and in neither case will catastrophic consequences happen (unless you
were relying on hap_track_dirty_vram for something other than tracking
dirty vram).

I'm inclined to say that ideally, change_type_range should pass an error
up if end > max_mapped_pfn.

But of course, it doesn't return an error at the moment, so that's out
of scope for this series.

I take it, Jan, that in the absence of changing the behavior, you'd like
the comment to look something like this?

"Always clip the rangeset down to the host p2m.  NB that this means the
logdirty_range will also be clipped, so in the future gfns in
(host_max_pfn, end) range won't be affected by change_entry_type_global.
 We should probably return an error in this case instead, as it's almost
certainly a mistake; but that's left as a clean-up for another time."

 -George

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

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

[Jan Beulich]

>>> On 19.12.18 at 18:26, <george.dunlap@citrix.com> wrote:
> On 12/13/18 10:43 AM, Jan Beulich wrote:
>>>>> On 13.12.18 at 11:22, <rcojocaru@bitdefender.com> wrote:
>>> For my own part, I see no reason why not clipping end should not work
>>> when updating the ranges only (as long as start continues to be <=
>>> unclipped_end).
>>>
>>> Would that modification + testing of it help this series continue?
>> 
>> I think so, at least as far as I'm concerned. But I think we really need
>> George's opinion as well.
> 
> We are going off into the weeds a little bit here I think.
> 
> If I understand Jan's concern properly, he's concerned about a situation
> like this:
> 
> [start] p2m->max_mapped_pfn == 0xfff
> 1. change_type_range ram => logdirty, [0x900, 0x1200)
> 
> Obviously the actual p2m entries can only be changed from 0x900 to
> 0xfff; but what about the logdirty ranges?  At the moment, the result
> will be a rangeset with [0x900, 0xfff].
> 
> Jan is asking whether the rangeset should instead be [0x900, 0x11ff].
> 
> So the time when it would matter would be a situation like the following:
> 
> 2. p2m_set_entry(0x1100, M)
> 
> 3. change_entry_type_global(ram => logdirty)
> 
> 4. change_entry_type_global(logdirty => ram)
> 
> Under the current regime gfn 0x1100 would be have type ram_rw both after
> step 2, and after step 4.
> 
> If we used Jan's suggestion, then it would be marked as ram_rw after
> step 2, and logdirty after step 4.

Afaict it would be marked logdirty also after step 2, at least
effectively (to the outside world), due to ept_get_entry()'s call
to p2m_recalc_type(). It may well be that there are more bugs
here (like ept_set_entry() not honoring this, but then again
this is perhaps something the callers should already take care
of), but that's the behavior I'd expect, and why I think the
range should not be clipped for the purpose of insertion into
the rangeset.

> But of course that's no different than what would happen if
> max_mapped_pfn were 0x2000, but gfns 0x1000-11ff just happened to be empty.
> 
> Under normal circumstances, neither of these situations should happen;
> and in neither case will catastrophic consequences happen (unless you
> were relying on hap_track_dirty_vram for something other than tracking
> dirty vram).
> 
> I'm inclined to say that ideally, change_type_range should pass an error
> up if end > max_mapped_pfn.
> 
> But of course, it doesn't return an error at the moment, so that's out
> of scope for this series.
> 
> I take it, Jan, that in the absence of changing the behavior, you'd like
> the comment to look something like this?
> 
> "Always clip the rangeset down to the host p2m.  NB that this means the
> logdirty_range will also be clipped, so in the future gfns in
> (host_max_pfn, end) range won't be affected by change_entry_type_global.
>  We should probably return an error in this case instead, as it's almost
> certainly a mistake; but that's left as a clean-up for another time."

Well, not exactly. IMO at least p2m_change_type_range(...,
0, ULONG_MAX) should match p2m_change_entry_type_global(),
with the exception of the rangeset modification (which in the
p2m_change_entry_type_global() global case is replaced by
modifying p2m->global_logdirty).

Jan



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

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

[George Dunlap]

On 12/20/18 8:20 AM, Jan Beulich wrote:
>>>> On 19.12.18 at 18:26, <george.dunlap@citrix.com> wrote:
>> On 12/13/18 10:43 AM, Jan Beulich wrote:
>>>>>> On 13.12.18 at 11:22, <rcojocaru@bitdefender.com> wrote:
>>>> For my own part, I see no reason why not clipping end should not work
>>>> when updating the ranges only (as long as start continues to be <=
>>>> unclipped_end).
>>>>
>>>> Would that modification + testing of it help this series continue?
>>>
>>> I think so, at least as far as I'm concerned. But I think we really need
>>> George's opinion as well.
>>
>> We are going off into the weeds a little bit here I think.
>>
>> If I understand Jan's concern properly, he's concerned about a situation
>> like this:
>>
>> [start] p2m->max_mapped_pfn == 0xfff
>> 1. change_type_range ram => logdirty, [0x900, 0x1200)
>>
>> Obviously the actual p2m entries can only be changed from 0x900 to
>> 0xfff; but what about the logdirty ranges?  At the moment, the result
>> will be a rangeset with [0x900, 0xfff].
>>
>> Jan is asking whether the rangeset should instead be [0x900, 0x11ff].
>>
>> So the time when it would matter would be a situation like the following:
>>
>> 2. p2m_set_entry(0x1100, M)
>>
>> 3. change_entry_type_global(ram => logdirty)
>>
>> 4. change_entry_type_global(logdirty => ram)
>>
>> Under the current regime gfn 0x1100 would be have type ram_rw both after
>> step 2, and after step 4.
>>
>> If we used Jan's suggestion, then it would be marked as ram_rw after
>> step 2, and logdirty after step 4.
> 
> Afaict it would be marked logdirty also after step 2, at least
> effectively (to the outside world), due to ept_get_entry()'s call
> to p2m_recalc_type().

That's not what I'm seeing.  Let's consider the ept entry for gfn 0x1100
at/after the various stages:

[start]: empty (valid bit clear)
1. change_type_range doesn't touch this, so still empty.
2. ept_set_entry(M)
 - Calls recalc_type(). This will walk the ept table down to the
particular ept entry, resolving the `recalc` bit at each level.
 - Finally it will set the entry to point to M, with the recalc bit
clear, and the entry *not* misconfigured.

Guest writes will not trigger an EPT fault at this point, so the most
important part of the "outside world" will not effectively see a logdirty.

What about ept_get_entry() after point 2?  Well, it calls
p2m_recalc_type() with "recalc || ept->recalc".  The first is
accumulated by walking down the ept tables; but in this case those bits
will already have been cleared by the recalc_type() at the top of
set_entry.  And of course, the gfn's own ept entry will have the recalc
bit clear.

So p2m_recalc_type() will be called with `recalc` set to zero.  When
that's the case, it always returns the type passed to it, without
checking logdirty.

Did I miss anything?

> It may well be that there are more bugs
> here (like ept_set_entry() not honoring this, but then again
> this is perhaps something the callers should already take care
> of), but that's the behavior I'd expect, and why I think the
> range should not be clipped for the purpose of insertion into
> the rangeset.
> 
>> But of course that's no different than what would happen if
>> max_mapped_pfn were 0x2000, but gfns 0x1000-11ff just happened to be empty.
>>
>> Under normal circumstances, neither of these situations should happen;
>> and in neither case will catastrophic consequences happen (unless you
>> were relying on hap_track_dirty_vram for something other than tracking
>> dirty vram).
>>
>> I'm inclined to say that ideally, change_type_range should pass an error
>> up if end > max_mapped_pfn.
>>
>> But of course, it doesn't return an error at the moment, so that's out
>> of scope for this series.
>>
>> I take it, Jan, that in the absence of changing the behavior, you'd like
>> the comment to look something like this?
>>
>> "Always clip the rangeset down to the host p2m.  NB that this means the
>> logdirty_range will also be clipped, so in the future gfns in
>> (host_max_pfn, end) range won't be affected by change_entry_type_global.
>>  We should probably return an error in this case instead, as it's almost
>> certainly a mistake; but that's left as a clean-up for another time."
> 
> Well, not exactly. IMO at least p2m_change_type_range(...,
> 0, ULONG_MAX) should match p2m_change_entry_type_global(),
> with the exception of the rangeset modification (which in the
> p2m_change_entry_type_global() global case is replaced by
> modifying p2m->global_logdirty).

That's one sensible interface I considered; but I don't think it's the
best one.  It has the advantage that from an interface perspective it's
clean and satisfying.  But I'm having difficulty imagining a situation
where that behavior would lead to better outcomes.  On the contrary, the
only time I can imagine this situation happening at the moment is if
there were a bug in the device model -- in which case, returning an
error would be a much more helpful thing to do.

 -George

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

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

[Jan Beulich]

>>> On 20.12.18 at 13:59, <george.dunlap@citrix.com> wrote:
> On 12/20/18 8:20 AM, Jan Beulich wrote:
>>>>> On 19.12.18 at 18:26, <george.dunlap@citrix.com> wrote:
>>> On 12/13/18 10:43 AM, Jan Beulich wrote:
>>>>>>> On 13.12.18 at 11:22, <rcojocaru@bitdefender.com> wrote:
>>>>> For my own part, I see no reason why not clipping end should not work
>>>>> when updating the ranges only (as long as start continues to be <=
>>>>> unclipped_end).
>>>>>
>>>>> Would that modification + testing of it help this series continue?
>>>>
>>>> I think so, at least as far as I'm concerned. But I think we really need
>>>> George's opinion as well.
>>>
>>> We are going off into the weeds a little bit here I think.
>>>
>>> If I understand Jan's concern properly, he's concerned about a situation
>>> like this:
>>>
>>> [start] p2m->max_mapped_pfn == 0xfff
>>> 1. change_type_range ram => logdirty, [0x900, 0x1200)
>>>
>>> Obviously the actual p2m entries can only be changed from 0x900 to
>>> 0xfff; but what about the logdirty ranges?  At the moment, the result
>>> will be a rangeset with [0x900, 0xfff].
>>>
>>> Jan is asking whether the rangeset should instead be [0x900, 0x11ff].
>>>
>>> So the time when it would matter would be a situation like the following:
>>>
>>> 2. p2m_set_entry(0x1100, M)
>>>
>>> 3. change_entry_type_global(ram => logdirty)
>>>
>>> 4. change_entry_type_global(logdirty => ram)
>>>
>>> Under the current regime gfn 0x1100 would be have type ram_rw both after
>>> step 2, and after step 4.
>>>
>>> If we used Jan's suggestion, then it would be marked as ram_rw after
>>> step 2, and logdirty after step 4.
>> 
>> Afaict it would be marked logdirty also after step 2, at least
>> effectively (to the outside world), due to ept_get_entry()'s call
>> to p2m_recalc_type().
> 
> That's not what I'm seeing.  Let's consider the ept entry for gfn 0x1100
> at/after the various stages:
> 
> [start]: empty (valid bit clear)
> 1. change_type_range doesn't touch this, so still empty.
> 2. ept_set_entry(M)
>  - Calls recalc_type(). This will walk the ept table down to the
> particular ept entry, resolving the `recalc` bit at each level.
>  - Finally it will set the entry to point to M, with the recalc bit
> clear, and the entry *not* misconfigured.

Well of course - if the caller specified p2m_ram_rw. But this is
wrong for the caller to do for any page inside the logdirty
range. ept_set_entry() is a function which is not itself
implementing policy; it depends on higher levels getting things
right together with the page tables being in proper state.

> Guest writes will not trigger an EPT fault at this point, so the most
> important part of the "outside world" will not effectively see a logdirty.
> 
> What about ept_get_entry() after point 2?  Well, it calls
> p2m_recalc_type() with "recalc || ept->recalc".  The first is
> accumulated by walking down the ept tables; but in this case those bits
> will already have been cleared by the recalc_type() at the top of
> set_entry.  And of course, the gfn's own ept entry will have the recalc
> bit clear.
> 
> So p2m_recalc_type() will be called with `recalc` set to zero.  When
> that's the case, it always returns the type passed to it, without
> checking logdirty.
> 
> Did I miss anything?

I don't think so, except perhaps me having said ...

>> It may well be that there are more bugs
>> here (like ept_set_entry() not honoring this, but then again

... this. IOW what you describe might match the current
situation, but I'm unconvinced it's intended/wanted behavior.

>> this is perhaps something the callers should already take care
>> of), but that's the behavior I'd expect, and why I think the
>> range should not be clipped for the purpose of insertion into
>> the rangeset.
>> 
>>> But of course that's no different than what would happen if
>>> max_mapped_pfn were 0x2000, but gfns 0x1000-11ff just happened to be empty.
>>>
>>> Under normal circumstances, neither of these situations should happen;
>>> and in neither case will catastrophic consequences happen (unless you
>>> were relying on hap_track_dirty_vram for something other than tracking
>>> dirty vram).
>>>
>>> I'm inclined to say that ideally, change_type_range should pass an error
>>> up if end > max_mapped_pfn.
>>>
>>> But of course, it doesn't return an error at the moment, so that's out
>>> of scope for this series.
>>>
>>> I take it, Jan, that in the absence of changing the behavior, you'd like
>>> the comment to look something like this?
>>>
>>> "Always clip the rangeset down to the host p2m.  NB that this means the
>>> logdirty_range will also be clipped, so in the future gfns in
>>> (host_max_pfn, end) range won't be affected by change_entry_type_global.
>>>  We should probably return an error in this case instead, as it's almost
>>> certainly a mistake; but that's left as a clean-up for another time."
>> 
>> Well, not exactly. IMO at least p2m_change_type_range(...,
>> 0, ULONG_MAX) should match p2m_change_entry_type_global(),
>> with the exception of the rangeset modification (which in the
>> p2m_change_entry_type_global() global case is replaced by
>> modifying p2m->global_logdirty).
> 
> That's one sensible interface I considered; but I don't think it's the
> best one.  It has the advantage that from an interface perspective it's
> clean and satisfying.  But I'm having difficulty imagining a situation
> where that behavior would lead to better outcomes.  On the contrary, the
> only time I can imagine this situation happening at the moment is if
> there were a bug in the device model -- in which case, returning an
> error would be a much more helpful thing to do.

I would agree if ->max_mapped_gfn was under qemu's direct
control. But the value depends on guest behavior. It just so
happens that for dirty vram tracking it's perhaps quite unlikely
for the range to live above ->max_mapped_gfn, but I wouldn't
be surprised if things broke when moving the frame buffer
pretty high up in (guest) physical memory, far beyond RAM
(and above 4Gb).

Jan


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

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

[George Dunlap]

On 12/20/18 1:52 PM, Jan Beulich wrote:
>>>> On 20.12.18 at 13:59, <george.dunlap@citrix.com> wrote:
>> On 12/20/18 8:20 AM, Jan Beulich wrote:
>>>>>> On 19.12.18 at 18:26, <george.dunlap@citrix.com> wrote:
>>>> On 12/13/18 10:43 AM, Jan Beulich wrote:
>>>>>>>> On 13.12.18 at 11:22, <rcojocaru@bitdefender.com> wrote:
>>>>>> For my own part, I see no reason why not clipping end should not work
>>>>>> when updating the ranges only (as long as start continues to be <=
>>>>>> unclipped_end).
>>>>>>
>>>>>> Would that modification + testing of it help this series continue?
>>>>>
>>>>> I think so, at least as far as I'm concerned. But I think we really need
>>>>> George's opinion as well.
>>>>
>>>> We are going off into the weeds a little bit here I think.
>>>>
>>>> If I understand Jan's concern properly, he's concerned about a situation
>>>> like this:
>>>>
>>>> [start] p2m->max_mapped_pfn == 0xfff
>>>> 1. change_type_range ram => logdirty, [0x900, 0x1200)
>>>>
>>>> Obviously the actual p2m entries can only be changed from 0x900 to
>>>> 0xfff; but what about the logdirty ranges?  At the moment, the result
>>>> will be a rangeset with [0x900, 0xfff].
>>>>
>>>> Jan is asking whether the rangeset should instead be [0x900, 0x11ff].
>>>>
>>>> So the time when it would matter would be a situation like the following:
>>>>
>>>> 2. p2m_set_entry(0x1100, M)
>>>>
>>>> 3. change_entry_type_global(ram => logdirty)
>>>>
>>>> 4. change_entry_type_global(logdirty => ram)
>>>>
>>>> Under the current regime gfn 0x1100 would be have type ram_rw both after
>>>> step 2, and after step 4.
>>>>
>>>> If we used Jan's suggestion, then it would be marked as ram_rw after
>>>> step 2, and logdirty after step 4.
>>>
>>> Afaict it would be marked logdirty also after step 2, at least
>>> effectively (to the outside world), due to ept_get_entry()'s call
>>> to p2m_recalc_type().
>>
>> That's not what I'm seeing.  Let's consider the ept entry for gfn 0x1100
>> at/after the various stages:
>>
>> [start]: empty (valid bit clear)
>> 1. change_type_range doesn't touch this, so still empty.
>> 2. ept_set_entry(M)
>>  - Calls recalc_type(). This will walk the ept table down to the
>> particular ept entry, resolving the `recalc` bit at each level.
>>  - Finally it will set the entry to point to M, with the recalc bit
>> clear, and the entry *not* misconfigured.
> 
> Well of course - if the caller specified p2m_ram_rw. But this is
> wrong for the caller to do for any page inside the logdirty
> range. ept_set_entry() is a function which is not itself
> implementing policy; it depends on higher levels getting things
> right together with the page tables being in proper state.

But nobody is doing any checking right now.  Under what circumstances
*would* p2m_set_entry() with p2m_ram_logdirty be called?  On the other
hand, p2m_set_entry() with p2m_ram_rw could easily happen if the guest
calls decrease_reservation and then populate_physmap on a page in the
logdirty range.

I also disagree about the layering argument.  Everything else about
managing logdirty setup in the p2m tables is handled by p2m-[e]pt.c;
swizzling p2m_ram_rw into p2m_ram_logdirty in [e]pt_set_entry() would be
the most obvious and consistent thing to do.

> IOW what you describe might match the current
> situation, but I'm unconvinced it's intended/wanted behavior.

Well no, I wouldn't say it's wanted behavior.  The point was to look at
the actual current behavior, and see if there was a simple change we
could make to make things consistent.  It turns out, there's not.

At the moment I'm only working 4-ish more days between now and the code
freeze, and we're arguing over whether the comment should say, "We
should probably do X instead" or "We should probably do Y instead."

Can we just for now take the text as I proposed it?  You can argue about
the right thing to do when we do the alleged clean-up.

 -George

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

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

[Jan Beulich]

>>> On 20.12.18 at 15:38, <george.dunlap@citrix.com> wrote:
> Can we just for now take the text as I proposed it?  You can argue about
> the right thing to do when we do the alleged clean-up.

With the "We should probably return an error ..." part dropped
or replaced by e.g. "We should revisit this" this would be okay
with me. But you're the maintainer of the code anyway...

Jan



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

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

[Jan Beulich]

>>> On 20.12.18 at 15:38, <george.dunlap@citrix.com> wrote:
> At the moment I'm only working 4-ish more days between now and the code
> freeze, and we're arguing over whether the comment should say, "We
> should probably do X instead" or "We should probably do Y instead."
> 
> Can we just for now take the text as I proposed it?  You can argue about
> the right thing to do when we do the alleged clean-up.

Oh, and btw - while I see where you're coming from, I don't like
taking an approaching code freeze as an excuse to short circuit
anything, neither reviews nor discussions. In the end it's a
comment only here, but the overall situation is (apparently)
unclear enough that a new misleading comment is not going to
help at all, the more that we all know how intended cleanups
often work out once the main roadblock is out of the way.

Jan



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

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

[Razvan Cojocaru]

On 12/20/18 4:49 PM, Jan Beulich wrote:
>>>> On 20.12.18 at 15:38, <george.dunlap@citrix.com> wrote:
>> Can we just for now take the text as I proposed it?  You can argue about
>> the right thing to do when we do the alleged clean-up.
> 
> With the "We should probably return an error ..." part dropped
> or replaced by e.g. "We should revisit this" this would be okay
> with me. But you're the maintainer of the code anyway...

Is the change something that can be done on commit (if there are no
other objections of course), or should I re-submit the series with the
comment changed?


Thanks,
Razvan

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

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

[George Dunlap]

On 12/20/18 3:14 PM, Razvan Cojocaru wrote:
> On 12/20/18 4:49 PM, Jan Beulich wrote:
>>>>> On 20.12.18 at 15:38, <george.dunlap@citrix.com> wrote:
>>> Can we just for now take the text as I proposed it?  You can argue about
>>> the right thing to do when we do the alleged clean-up.
>>
>> With the "We should probably return an error ..." part dropped
>> or replaced by e.g. "We should revisit this" this would be okay
>> with me. But you're the maintainer of the code anyway...
> 
> Is the change something that can be done on commit (if there are no
> other objections of course), or should I re-submit the series with the
> comment changed?

Well I haven't given a review of your changes to v4 yet; this was just
trying to answer Jan's question about what ideal interface would be.

If it's just comments I can fix things up on check-in.

 -George

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

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

[George Dunlap]

On 12/5/18 9:18 AM, Razvan Cojocaru wrote:
> 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>
> Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

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

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

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

[George Dunlap]

On 12/5/18 9:18 AM, Razvan Cojocaru wrote:
> 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>
> Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
>  - Fixed a double-space in the patch description.
>  - Fixed a coding style issue for
>    "if ( !start && end >= p2m->max_mapped_pfn)" (no space before
>    closing ')').
>  - Switched the early return comment back to "/* If the requested
>    range is out of scope, return doing nothing. */.
>  - Added Tamas' Tested-by.
> ---
>  xen/arch/x86/mm/p2m.c | 47 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index d145850..539ea16 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1002,30 +1002,43 @@ 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;

I don't like the idea of modifying arguments, which is why I duplicated
them in the first place.

What about calling the argument end_exclusive, and having a local
variable, 'end', which we set to be end_exclusive-1?

> +    if ( start >= host_max_pfn )
> +        printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n",
> +               d->domain_id);

This doesn't seem to be the right condition for this warning..  Surely
this warning would go better under the next conditional, where we
actually do the clipping?

> +    /* Always clip the rangeset down to the host p2m. */
> +    if ( unlikely(end > host_max_pfn) )
> +        end = host_max_pfn;

So what about modifying this comment thus:

"Always clip the rangeset down to the host p2m.  This is probably not
the right behavior.  This should be revisited later, but for now post a
warning."

Thanks,
 -George

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

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

[George Dunlap]

On 12/5/18 9:18 AM, Razvan Cojocaru wrote:
> change_type_range() 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>
> Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

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

So it looks like as soon as we get patch 4 into shape we can check this in.

 -George

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

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

[Razvan Cojocaru]

On 12/20/18 6:09 PM, George Dunlap wrote:
> On 12/5/18 9:18 AM, Razvan Cojocaru wrote:
>> 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>
>> Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
>>  - Fixed a double-space in the patch description.
>>  - Fixed a coding style issue for
>>    "if ( !start && end >= p2m->max_mapped_pfn)" (no space before
>>    closing ')').
>>  - Switched the early return comment back to "/* If the requested
>>    range is out of scope, return doing nothing. */.
>>  - Added Tamas' Tested-by.
>> ---
>>  xen/arch/x86/mm/p2m.c | 47 ++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index d145850..539ea16 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1002,30 +1002,43 @@ 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;
> 
> I don't like the idea of modifying arguments, which is why I duplicated
> them in the first place.
> 
> What about calling the argument end_exclusive, and having a local
> variable, 'end', which we set to be end_exclusive-1?

Of course, I'll do that.

>> +    if ( start >= host_max_pfn )
>> +        printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n",
>> +               d->domain_id);
> 
> This doesn't seem to be the right condition for this warning..  Surely
> this warning would go better under the next conditional, where we
> actually do the clipping?
> 
>> +    /* Always clip the rangeset down to the host p2m. */
>> +    if ( unlikely(end > host_max_pfn) )
>> +        end = host_max_pfn;
> 
> So what about modifying this comment thus:
> 
> "Always clip the rangeset down to the host p2m.  This is probably not
> the right behavior.  This should be revisited later, but for now post a
> warning."

You're right, the warning is a bit out of place above. I'll move it and
update the comment as indicated.


Thank you,
Razvan

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

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

[George Dunlap]

On 12/5/18 9:18 AM, Razvan Cojocaru wrote:
> 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>
> Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
>  - Fixed a double-space in the patch description.
>  - Fixed a coding style issue for
>    "if ( !start && end >= p2m->max_mapped_pfn)" (no space before
>    closing ')').
>  - Switched the early return comment back to "/* If the requested
>    range is out of scope, return doing nothing. */.
>  - Added Tamas' Tested-by.
> ---
>  xen/arch/x86/mm/p2m.c | 47 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index d145850..539ea16 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1002,30 +1002,43 @@ 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. */
> +    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);

Nitpick: This is printk is also wrong ATM: It uses [..), which would
indicate that the last value was exclusive.  And it was when we weren't
modifying end; but with the `--end` at the top, the range is now inclusive.

Whatever we end up doing with `end`, this should match.  If we name the
argument as end_exclusive, I'd say leave the string and use
end_exclusive here.

-George


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

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

[Razvan Cojocaru]

On 12/20/18 6:31 PM, George Dunlap wrote:
> On 12/5/18 9:18 AM, Razvan Cojocaru wrote:
>> 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>
>> Tested-by: Tamas K Lengyel <tamas@tklengyel.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 V10:
>>  - Fixed a double-space in the patch description.
>>  - Fixed a coding style issue for
>>    "if ( !start && end >= p2m->max_mapped_pfn)" (no space before
>>    closing ')').
>>  - Switched the early return comment back to "/* If the requested
>>    range is out of scope, return doing nothing. */.
>>  - Added Tamas' Tested-by.
>> ---
>>  xen/arch/x86/mm/p2m.c | 47 ++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index d145850..539ea16 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1002,30 +1002,43 @@ 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. */
>> +    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);
> 
> Nitpick: This is printk is also wrong ATM: It uses [..), which would
> indicate that the last value was exclusive.  And it was when we weren't
> modifying end; but with the `--end` at the top, the range is now inclusive.
> 
> Whatever we end up doing with `end`, this should match.  If we name the
> argument as end_exclusive, I'd say leave the string and use
> end_exclusive here.

Right, sorry about that. I'll use end_exclusive.


Thanks,
Razvan

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