(no subject)

(no subject)

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

Since the last version of the series, what has previously been
the first patch is already upstream, and two patches kindly
authored by George Dunlap have been appended. The patches
optimize the way rangeset changes are propagated in
change_type_range().

[PATCH V7 1/5] x86/mm: introduce p2m_{init,free}_logdirty()
[PATCH V7 2/5] x86/mm: allocate logdirty_ranges for altp2ms
[PATCH V7 3/5] x86/altp2m: fix display frozen when switching to a new view early
[PATCH V7 4/5] p2m: Always use hostp2m when clipping rangesets
[PATCH V7 5/5] p2m: change_range_type: Only invalidate mapped gfns


Thanks,
Razvan

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

[PATCH V7 1/5] x86/mm: introduce p2m_{init, free}_logdirty()

[Razvan Cojocaru]

Add logdirty_ranges allocator / deallocator helpers.
p2m_init_logdirty() will not re-allocate if
p2m->logdirty ranges has already been allocated.

Move the rangeset deallocation call from p2m_teardown_hostp2m()
to p2m_free_one() - we will want this to apply to altp2ms
as well.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.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>

---
Changes since V6:
 - Added George's Reviewed-by.
---
 xen/arch/x86/mm/p2m.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6a1abb6..418ff85 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -59,6 +59,28 @@ static void p2m_nestedp2m_init(struct p2m_domain *p2m)
 #endif
 }
 
+static int p2m_init_logdirty(struct p2m_domain *p2m)
+{
+    if ( p2m->logdirty_ranges )
+        return 0;
+
+    p2m->logdirty_ranges = rangeset_new(p2m->domain, "log-dirty",
+                                        RANGESETF_prettyprint_hex);
+    if ( !p2m->logdirty_ranges )
+        return -ENOMEM;
+
+    return 0;
+}
+
+static void p2m_free_logdirty(struct p2m_domain *p2m)
+{
+    if ( !p2m->logdirty_ranges )
+        return;
+
+    rangeset_destroy(p2m->logdirty_ranges);
+    p2m->logdirty_ranges = NULL;
+}
+
 /* Init the datastructures for later use by the p2m code */
 static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
 {
@@ -107,6 +129,7 @@ free_p2m:
 
 static void p2m_free_one(struct p2m_domain *p2m)
 {
+    p2m_free_logdirty(p2m);
     if ( hap_enabled(p2m->domain) && cpu_has_vmx )
         ept_p2m_uninit(p2m);
     free_cpumask_var(p2m->dirty_cpumask);
@@ -116,19 +139,19 @@ static void p2m_free_one(struct p2m_domain *p2m)
 static int p2m_init_hostp2m(struct domain *d)
 {
     struct p2m_domain *p2m = p2m_init_one(d);
+    int rc;
 
-    if ( p2m )
-    {
-        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
-                                            RANGESETF_prettyprint_hex);
-        if ( p2m->logdirty_ranges )
-        {
-            d->arch.p2m = p2m;
-            return 0;
-        }
+    if ( !p2m )
+        return -ENOMEM;
+
+    rc = p2m_init_logdirty(p2m);
+
+    if ( !rc )
+        d->arch.p2m = p2m;
+    else
         p2m_free_one(p2m);
-    }
-    return -ENOMEM;
+
+    return rc;
 }
 
 static void p2m_teardown_hostp2m(struct domain *d)
@@ -138,7 +161,6 @@ static void p2m_teardown_hostp2m(struct domain *d)
 
     if ( p2m )
     {
-        rangeset_destroy(p2m->logdirty_ranges);
         p2m_free_one(p2m);
         d->arch.p2m = NULL;
     }
-- 
2.7.4


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

[PATCH V7 2/5] x86/mm: 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.

Write a helper function to do altp2m activiation (appropriately
handling failures). Also, refactor p2m_reset_altp2m() so that it
can be used to remove redundant codepaths, fixing the locking
while we’re at it.

While we're here, switch global_logdirty from bool_t to bool.

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>

---
Changes since V6:
 - Replaced the description of the patch with the one suggested
   by George.
 - p2m_reset_altp2m() now takes an enum altp2m_reset_type param
   to make it clearer what it does at callsites.
 - The "Uninit and reinit ept to force TLB shootdown" comment
   has been moved above the ept_p2m_uninit() call in
   p2m_reset_altp2m().
 - p2m_init_altp2m_logdirty() has been merged into
   p2m_activate_altp2m().
 - p2m_activate_altp2m() now takes the p2m lock.
---
 xen/arch/x86/mm/p2m.c     | 103 ++++++++++++++++++++++++++++++++--------------
 xen/include/asm-x86/p2m.h |   2 +-
 2 files changed, 74 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 418ff85..9773495 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2282,6 +2282,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;
@@ -2290,16 +2320,47 @@ 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);
     }
 
     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;
@@ -2310,10 +2371,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;
@@ -2331,9 +2389,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;
     }
@@ -2360,10 +2419,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;
         }
@@ -2488,16 +2544,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)
@@ -2531,7 +2577,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
@@ -2545,10 +2591,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;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index ac33f50..c7f5710 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -222,7 +222,7 @@ struct p2m_domain {
     struct rangeset   *logdirty_ranges;
 
     /* Host p2m: Global log-dirty mode enabled for the domain. */
-    bool_t             global_logdirty;
+    bool               global_logdirty;
 
     /* Host p2m: when this flag is set, don't flush all the nested-p2m 
      * tables on every host-p2m change.  The setter of this flag 
-- 
2.7.4


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

[PATCH V7 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.

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

---
Changes since V6:
 - Removed Kevin's Reviewed-by since I've done non-trivial changes
   (not sure if that was wrong).
 - Now setting p2m->max_mapped_pfn to 0.
 - Added finish_type_change().
---
 xen/arch/x86/mm/p2m-ept.c |   9 ++-
 xen/arch/x86/mm/p2m-pt.c  |   8 +++
 xen/arch/x86/mm/p2m.c     | 165 ++++++++++++++++++++++++++++++++++++++--------
 xen/include/asm-x86/p2m.h |   6 +-
 4 files changed, 157 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index fabcd06..36e648c 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;
@@ -1440,9 +1443,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 55df185..3828088 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 9773495..823feb4 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,
@@ -991,18 +1038,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) )
@@ -1046,23 +1089,54 @@ void p2m_change_type_range(struct domain *d,
     p2m->defer_nested_flush = 0;
     if ( nestedhvm_enabled(d) )
         p2m_flush_nestedp2m(d);
-    p2m_unlock(p2m);
+}
+
+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);
+
+    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
+
+    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 )
     {
@@ -1077,14 +1151,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 c7f5710..be5b7a2 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -630,9 +630,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);
 
@@ -663,6 +660,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 V7 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>

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

---
RFC: Wasn't sure what the best thing was to do if start >=
host_max_pfn.  We silently clip the logdirty rangeset to
max_mapped_pfn, and the chosen behavior seems consistent with that.
But it seems like such a request would almost certainly be a bug
somewhere that people might like to find out about.
---
 xen/arch/x86/mm/p2m.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 823feb4..80d3331 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1037,32 +1037,44 @@ 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;
+    unsigned long rangeset_start, rangeset_end;
     struct domain *d = p2m->domain;
+    unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
     int rc = 0;
 
+    rangeset_start = start;
+    rangeset_end   = end - 1;
+
+    /* Always clip the rangeset down to the host p2m */
+    if ( unlikely(rangeset_end > host_max_pfn) )
+        rangeset_end = host_max_pfn;
+
+    /* If the requested range is out of scope, return doing nothing */
+    if ( rangeset_start > rangeset_end )
+        return;
+
     p2m->defer_nested_flush = 1;
 
-    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);
+    /*
+     * If all valid gfns are in the invalidation range, just do a
+     * global type change.  Otherwise, invalidate only the range we
+     * need.
+     */
+    if ( !rangeset_start && rangeset_end >= p2m->max_mapped_pfn)
+        p2m->change_entry_type_global(p2m, ot, nt);
+    else
+        rc = p2m->change_entry_type_range(p2m, ot, nt,
+                                          rangeset_start, rangeset_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);
+               rc, d->domain_id, rangeset_start, rangeset_end, ot, nt);
         domain_crash(d);
     }
 
@@ -1070,11 +1082,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, rangeset_start, rangeset_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, rangeset_start, rangeset_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 V7 5/5] p2m: change_range_type: 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>

---
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>
---
 xen/arch/x86/mm/p2m.c | 57 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 80d3331..f3f3764 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1043,39 +1043,62 @@ static void change_type_range(struct p2m_domain *p2m,
                               p2m_type_t ot, p2m_type_t nt)
 {
     unsigned long rangeset_start, rangeset_end;
+    unsigned long invalidate_start, invalidate_end;
     struct domain *d = p2m->domain;
     unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
+    unsigned long max_pfn = p2m->max_mapped_pfn;
     int rc = 0;
 
-    rangeset_start = start;
-    rangeset_end   = end - 1;
+    /*
+     * 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.
+     */
+    rangeset_start = invalidate_start = start;
+    rangeset_end   = invalidate_end   = end - 1;
 
-    /* Always clip the rangeset down to the host p2m */
+    /* Clip down to the host p2m */
     if ( unlikely(rangeset_end > host_max_pfn) )
-        rangeset_end = host_max_pfn;
+        rangeset_end = invalidate_end = host_max_pfn;
 
     /* If the requested range is out of scope, return doing nothing */
     if ( rangeset_start > rangeset_end )
         return;
 
+    if ( p2m_is_altp2m(p2m) )
+        invalidate_end = min(invalidate_end, max_pfn);
+
     p2m->defer_nested_flush = 1;
 
     /*
-     * 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 ( !rangeset_start && rangeset_end >= p2m->max_mapped_pfn)
-        p2m->change_entry_type_global(p2m, ot, nt);
-    else
-        rc = p2m->change_entry_type_range(p2m, ot, nt,
-                                          rangeset_start, rangeset_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, rangeset_start, rangeset_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: (no subject)

[Razvan Cojocaru]

Apologies, the subject should have been, of course, "[PATCH V7 0/5] Fix
VGA logdirty related display freezes with altp2m", which I did paste in,
but ommited to uncomment.


Sorry,
Razvan

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

Re: [PATCH V7 2/5] x86/mm: allocate logdirty_ranges for altp2ms

[Jan Beulich]

>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> wrote:
> 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.
> 
> Write a helper function to do altp2m activiation (appropriately
> handling failures). Also, refactor p2m_reset_altp2m() so that it
> can be used to remove redundant codepaths, fixing the locking
> while we’re at it.

Perhaps this should have been a separate patch again, such
that e.g. ...

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

... the addition of these can be properly associated with either
part of the change. Looking at the code you remove from e.g.
p2m_flush_altp2m() it's not part of the refactoring, but of what
this patch's actual purpose is. But this is guesswork of mine
without the split and without the addition getting explained,
not the least because this getting moved here from the original
instance of the function might also mean that it's part of the
refactoring, but would then need to be done only in the
ALTP2M_RESET case.

Jan


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

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

[Jan Beulich]

>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> 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.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>

Judging from George's earlier analysis I wonder whether the patch
ordering is correct: I've got the impression that the patch here should
be last in the series, for it to be correct and efficient in all cases.

Furthermore (minor, but this appears to recur), may I ask that you
switch around tags like the above ones in the future? As previously
expressed (in other contexts) I'm of the opinion that the sequence
of tags should represent the flow of events, and a bug report as
much as a suggestion of a change can't have happened before the
writing of a patch.

Jan



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

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

[Razvan Cojocaru]

On 11/20/18 11:12 AM, Jan Beulich wrote:
>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> 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.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
> Judging from George's earlier analysis I wonder whether the patch
> ordering is correct: I've got the impression that the patch here should
> be last in the series, for it to be correct and efficient in all cases.
> 
> Furthermore (minor, but this appears to recur), may I ask that you
> switch around tags like the above ones in the future? As previously
> expressed (in other contexts) I'm of the opinion that the sequence
> of tags should represent the flow of events, and a bug report as
> much as a suggestion of a change can't have happened before the
> writing of a patch.

It's quite true, hence my question here:

https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg02044.html

It has also occured to me that applying this patch before George's (and
_not_ applying his afterwards) will produce code that will pass all the
compilation tests but will still crash the hypervisor when altp2m gets
activated on a guest.

If George doesn't mind, I can try to figure out a way to have them
either applied before the final patch.

As for the tag order, sorry about that - I simply didn't know that was
the convention. It's of course no problem to switch them.


Thanks,
Razvan

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

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

[George Dunlap]

> On Nov 20, 2018, at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> 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.
>> 
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
> Judging from George's earlier analysis I wonder whether the patch
> ordering is correct: I've got the impression that the patch here should
> be last in the series, for it to be correct and efficient in all cases.

My patches back would require significant rework — both of my patches to rebase on an earlier tree, and of Razvan’s patches to be rebased later.  I don’t think this kind of thing should be required unless there is a compelling benefit to doing so.

Normally the reason for such an ordering is “no regressions in the middle of the series”, primarily in order to avoid breaking bisection; and of course there’s also something  much more aesthetically satisfying about doing a bunch of prep work behind the scenes and then flipping one switch to enable it at the end of the series.

In this case, altp2m + logdirty was already broken; so I didn’t think this patch could be considered to introduce a regression.  Thus the only reason to have this patch be the final patch would be for aesthetic purposes, which I didn’t consider enough value to justify requesting a patch re-ordering.

Did you have a compelling reason in mind for doing the reordering?

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

Re: [PATCH V7 2/5] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 11/20/18 11:05 AM, Jan Beulich wrote:
>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> wrote:
>> 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.
>>
>> Write a helper function to do altp2m activiation (appropriately
>> handling failures). Also, refactor p2m_reset_altp2m() so that it
>> can be used to remove redundant codepaths, fixing the locking
>> while we’re at it.
> 
> Perhaps this should have been a separate patch again, such
> that e.g. ...
> 
>> +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;
> 
> ... the addition of these can be properly associated with either
> part of the change. Looking at the code you remove from e.g.
> p2m_flush_altp2m() it's not part of the refactoring, but of what
> this patch's actual purpose is. But this is guesswork of mine
> without the split and without the addition getting explained,
> not the least because this getting moved here from the original
> instance of the function might also mean that it's part of the
> refactoring, but would then need to be done only in the
> ALTP2M_RESET case.

If you mean that p2m->min_remapped_gfn = gfn_x(INVALID_GFN); and
p2m->max_remapped_gfn = 0; should only happen on the ALTP2M_RESET case,
while that is technically true (and should follow from a verbatim
refactoring), George has pointed out that the assignments are in that
case unnecessary but harmless, and so the conditional is not worth it.

I can add the if and treat the RESET case explicitly if that's required.


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 20.11.18 at 10:43, <George.Dunlap@citrix.com> wrote:

> 
>> On Nov 20, 2018, at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> 
>>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> 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.
>>> 
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>> 
>> Judging from George's earlier analysis I wonder whether the patch
>> ordering is correct: I've got the impression that the patch here should
>> be last in the series, for it to be correct and efficient in all cases.
> 
> My patches back would require significant rework — both of my patches to 
> rebase on an earlier tree, and of Razvan’s patches to be rebased later.  I 
> don’t think this kind of thing should be required unless there is a 
> compelling benefit to doing so.
> 
> Normally the reason for such an ordering is “no regressions in the middle of 
> the series”, primarily in order to avoid breaking bisection; and of course 
> there’s also something  much more aesthetically satisfying about doing a 
> bunch of prep work behind the scenes and then flipping one switch to enable 
> it at the end of the series.
> 
> In this case, altp2m + logdirty was already broken; so I didn’t think this 
> patch could be considered to introduce a regression.  Thus the only reason to 
> have this patch be the final patch would be for aesthetic purposes, which I 
> didn’t consider enough value to justify requesting a patch re-ordering.
> 
> Did you have a compelling reason in mind for doing the reordering?

No, it merely looked wrong to me from earlier discussion. If staying
with the current order is fine with you, it'll be fine with me as well.
(It wasn't clear to me that re-ordering would be significant effort.)

Jan


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

Re: [PATCH V7 2/5] x86/mm: allocate logdirty_ranges for altp2ms

[Jan Beulich]

>>> On 20.11.18 at 11:02, <rcojocaru@bitdefender.com> wrote:
> On 11/20/18 11:05 AM, Jan Beulich wrote:
>>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> wrote:
>>> 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.
>>>
>>> Write a helper function to do altp2m activiation (appropriately
>>> handling failures). Also, refactor p2m_reset_altp2m() so that it
>>> can be used to remove redundant codepaths, fixing the locking
>>> while we’re at it.
>> 
>> Perhaps this should have been a separate patch again, such
>> that e.g. ...
>> 
>>> +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;
>> 
>> ... the addition of these can be properly associated with either
>> part of the change. Looking at the code you remove from e.g.
>> p2m_flush_altp2m() it's not part of the refactoring, but of what
>> this patch's actual purpose is. But this is guesswork of mine
>> without the split and without the addition getting explained,
>> not the least because this getting moved here from the original
>> instance of the function might also mean that it's part of the
>> refactoring, but would then need to be done only in the
>> ALTP2M_RESET case.
> 
> If you mean that p2m->min_remapped_gfn = gfn_x(INVALID_GFN); and
> p2m->max_remapped_gfn = 0; should only happen on the ALTP2M_RESET case,
> while that is technically true (and should follow from a verbatim
> refactoring), George has pointed out that the assignments are in that
> case unnecessary but harmless, and so the conditional is not worth it.
> 
> I can add the if and treat the RESET case explicitly if that's required.

No, I'm specifically not requiring this. What I'm requiring is that the
description match the changes. Which in turn would be easier if
the refactoring was a separate patch.

Jan


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

Re: [PATCH V7 2/5] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 11/20/18 12:27 PM, Jan Beulich wrote:
>>>> On 20.11.18 at 11:02, <rcojocaru@bitdefender.com> wrote:
>> On 11/20/18 11:05 AM, Jan Beulich wrote:
>>>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> wrote:
>>>> 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.
>>>>
>>>> Write a helper function to do altp2m activiation (appropriately
>>>> handling failures). Also, refactor p2m_reset_altp2m() so that it
>>>> can be used to remove redundant codepaths, fixing the locking
>>>> while we’re at it.
>>>
>>> Perhaps this should have been a separate patch again, such
>>> that e.g. ...
>>>
>>>> +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;
>>>
>>> ... the addition of these can be properly associated with either
>>> part of the change. Looking at the code you remove from e.g.
>>> p2m_flush_altp2m() it's not part of the refactoring, but of what
>>> this patch's actual purpose is. But this is guesswork of mine
>>> without the split and without the addition getting explained,
>>> not the least because this getting moved here from the original
>>> instance of the function might also mean that it's part of the
>>> refactoring, but would then need to be done only in the
>>> ALTP2M_RESET case.
>>
>> If you mean that p2m->min_remapped_gfn = gfn_x(INVALID_GFN); and
>> p2m->max_remapped_gfn = 0; should only happen on the ALTP2M_RESET case,
>> while that is technically true (and should follow from a verbatim
>> refactoring), George has pointed out that the assignments are in that
>> case unnecessary but harmless, and so the conditional is not worth it.
>>
>> I can add the if and treat the RESET case explicitly if that's required.
> 
> No, I'm specifically not requiring this. What I'm requiring is that the
> description match the changes. Which in turn would be easier if
> the refactoring was a separate patch.

I'll split this patch.


Thanks,
Razvan

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