[PATCH V8 0/7] Fix VGA logdirty related display freezes with altp2m

[PATCH V8 0/7] 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.

Since the last version of the series, the second patch has been
split into three smaller patches for easier review.

[PATCH V8 1/7] x86/mm: introduce p2m_{init,free}_logdirty()
[PATCH V8 2/7] x86/p2m: switch global_logdirty from bool_t to bool
[PATCH V8 3/7] x86/p2m: allocate logdirty_ranges for altp2ms
[PATCH V8 4/7] x86/p2m: refactor p2m_reset_altp2m()
[PATCH V8 5/7] x86/altp2m: fix display frozen when switching to a new view early
[PATCH V8 6/7] p2m: Always use hostp2m when clipping rangesets
[PATCH V8 7/7] 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 V8 1/7] 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>
CC: "Roger Pau Monné" <roger.pau@citrix.com>

---
Changes since V7:
 - None.
---
 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 V8 2/7] x86/p2m: switch global_logdirty from bool_t to bool

[Razvan Cojocaru]

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.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/include/asm-x86/p2m.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 6d849a5..b52fefd 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 V8 3/7] x86/p2m: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

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

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

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

---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
---
 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 418ff85..955c3c7 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2300,6 +2300,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;
@@ -2310,10 +2344,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 +2362,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 V8 4/7] 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.

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

---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
---
 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 955c3c7..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,10 +2320,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);
     }
 
@@ -2392,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;
         }
@@ -2520,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)
@@ -2563,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
@@ -2577,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;
-- 
2.7.4


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

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

[Razvan Cojocaru]

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

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

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

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

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

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

---
Changes since V7:
 - Reversed tag order (suggested by Jan Beulich).
---
 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 b52fefd..f353dcd 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -627,9 +627,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);
 
@@ -660,6 +657,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 V8 6/7] 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.

---
Changes since V7:
 - None.
---
 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 V8 7/7] 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>

---
Changes since V7:
 - None.
---
 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: [PATCH V8 3/7] x86/p2m: allocate logdirty_ranges for altp2ms

[Jan Beulich]

>>> On 21.11.18 at 11:19, <rcojocaru@bitdefender.com> wrote:
> +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);

Is p2m != hostp2m guaranteed here (recalling the discussion about
array slot 0)? The function may happen to work with both rangesets
the same, but I don't think this is guaranteed.

> +    if ( rc )
> +    {
> +        p2m_free_logdirty(p2m);
> +        goto out;
> +    }
> +
> +    p2m_init_altp2m_ept(d, idx);
> +
> +out:

Labels indented by at least one blank please.

Jan



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

Re: [PATCH V8 4/7] x86/p2m: refactor p2m_reset_altp2m()

[Jan Beulich]

>>> On 21.11.18 at 11:19, <rcojocaru@bitdefender.com> wrote:
> Refactor p2m_reset_altp2m() so that it can be used to remove
> redundant codepaths, fixing the locking while we're at it.

Still no word about ...

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

... these now done even when previously they weren't? In fact,
having looked again, it seems as if their omission from e.g.
p2m_flush_altp2m() was a mistake before, which you now fix.

Jan



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

Re: [PATCH V8 3/7] x86/p2m: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 11/21/18 12:45 PM, Jan Beulich wrote:
>>>> On 21.11.18 at 11:19, <rcojocaru@bitdefender.com> wrote:
>> +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);
> 
> Is p2m != hostp2m guaranteed here (recalling the discussion about
> array slot 0)? The function may happen to work with both rangesets
> the same, but I don't think this is guaranteed.

Yes, it is. We're "wasting" altp2m[0], as previously discussed.

>> +    if ( rc )
>> +    {
>> +        p2m_free_logdirty(p2m);
>> +        goto out;
>> +    }
>> +
>> +    p2m_init_altp2m_ept(d, idx);
>> +
>> +out:
> 
> Labels indented by at least one blank please.

Right, will change it in the next version.


Thanks,
Razvan

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

Re: [PATCH V8 4/7] x86/p2m: refactor p2m_reset_altp2m()

[Razvan Cojocaru]

On 11/21/18 12:49 PM, Jan Beulich wrote:
>>>> On 21.11.18 at 11:19, <rcojocaru@bitdefender.com> wrote:
>> Refactor p2m_reset_altp2m() so that it can be used to remove
>> redundant codepaths, fixing the locking while we're at it.
> 
> Still no word about ...
> 
>> +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;
> 
> ... these now done even when previously they weren't? In fact,
> having looked again, it seems as if their omission from e.g.
> p2m_flush_altp2m() was a mistake before, which you now fix.

Right, that's an omission I'll rectify in the next version.

According to a previous review from George, I'm not now fixing a problem
since the assignment was not required, however it is harmless (and
perhaps preferrable). AFAICT it does not fix a correctness issue.


Thanks,
Razvan

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

Re: [PATCH V8 4/7] x86/p2m: refactor p2m_reset_altp2m()

[Jan Beulich]

>>> On 21.11.18 at 11:57, <rcojocaru@bitdefender.com> wrote:
> On 11/21/18 12:49 PM, Jan Beulich wrote:
>>>>> On 21.11.18 at 11:19, <rcojocaru@bitdefender.com> wrote:
>>> Refactor p2m_reset_altp2m() so that it can be used to remove
>>> redundant codepaths, fixing the locking while we're at it.
>> 
>> Still no word about ...
>> 
>>> +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;
>> 
>> ... these now done even when previously they weren't? In fact,
>> having looked again, it seems as if their omission from e.g.
>> p2m_flush_altp2m() was a mistake before, which you now fix.
> 
> Right, that's an omission I'll rectify in the next version.
> 
> According to a previous review from George, I'm not now fixing a problem
> since the assignment was not required, however it is harmless (and
> perhaps preferrable). AFAICT it does not fix a correctness issue.

Not a correctness one, agreed, but an efficiency one?

Jan



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

Re: [PATCH V8 4/7] x86/p2m: refactor p2m_reset_altp2m()

[George Dunlap]

On 11/21/18 10:49 AM, Jan Beulich wrote:
>>>> On 21.11.18 at 11:19, <rcojocaru@bitdefender.com> wrote:
>> Refactor p2m_reset_altp2m() so that it can be used to remove
>> redundant codepaths, fixing the locking while we're at it.
> 
> Still no word about ...
> 
>> +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;
> 
> ... these now done even when previously they weren't? In fact,
> having looked again, it seems as if their omission from e.g.
> p2m_flush_altp2m() was a mistake before, which you now fix.

As Razvan says, it wasn't a correctness issue; in the other two
"reset-like" bits of code, the altp2m idx was disabled; which guaranteed
that before being used again it would go through p2m_init_altp2m_ept(),
which resets them.

His first version of this patch actually had these set conditionally, so
they'd only be reset in the case where they were reset originally. I
asked him to take it out: saving two redundant memory writes isn't worth
the extra conditional and the extra argument.

So the code is the way it should be; but it should be mentioned in the
commit message, so that reviewers / archaeologists can simply verify it
rather than re-discovering it.

 -George



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

Re: [PATCH V8 4/7] x86/p2m: refactor p2m_reset_altp2m()

[Jan Beulich]

>>> On 21.11.18 at 12:48, <george.dunlap@citrix.com> wrote:
> On 11/21/18 10:49 AM, Jan Beulich wrote:
>>>>> On 21.11.18 at 11:19, <rcojocaru@bitdefender.com> wrote:
>>> Refactor p2m_reset_altp2m() so that it can be used to remove
>>> redundant codepaths, fixing the locking while we're at it.
>> 
>> Still no word about ...
>> 
>>> +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;
>> 
>> ... these now done even when previously they weren't? In fact,
>> having looked again, it seems as if their omission from e.g.
>> p2m_flush_altp2m() was a mistake before, which you now fix.
> 
> As Razvan says, it wasn't a correctness issue; in the other two
> "reset-like" bits of code, the altp2m idx was disabled; which guaranteed
> that before being used again it would go through p2m_init_altp2m_ept(),
> which resets them.

Oh. That looks like a layering violation. I certainly didn't expect
these fields to be reset outside of p2m.c + altp2m.c, and the
resets missing from p2m_flush_altp2m() therefore looked like
a mistake.

> So the code is the way it should be; but it should be mentioned in the
> commit message, so that reviewers / archaeologists can simply verify it
> rather than re-discovering it.

Right, that's what I was trying to get at with the first sentence
(question).

Jan



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