[PATCH V3 0/3] Fix VGA logdirty related display freezes with altp2m

[PATCH V3 0/3] Fix VGA logdirty related display freezes with altp2m

[Razvan Cojocaru]

Hello,

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). Since the last version of the series,
what was previously the second (and last) patch has been split in
two patches, the first of which only concerns itself with rangeset
allocation / deallocation / initialization.

The first patch propagates ept.ad changes to all active altp2ms,
the second one allocates and initializes a new logdirty rangeset for
each new altp2m, and the third propagates (under lock) changes to all
p2ms.

[PATCH V3 1/3] x86/altp2m: propagate ept.ad changes to all active altp2ms
[PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms
[PATCH V3 3/3] x86/altp2m: fix display frozen when switching to a new view early


Thanks,
Razvan

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

[PATCH V3 1/3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Razvan Cojocaru]

This patch is a pre-requisite for fixing the logdirty VGA issue
(display freezes when switching to a new altp2m view early in a
domain's lifetime), but sent separately for easier review.
The new ept_set_ad_sync() function has been added to update all
active altp2ms' ept.ad. New altp2ms will inherit the hostp2m's
ept.ad value.

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>
---
 xen/arch/x86/mm/p2m-ept.c | 57 +++++++++++++++++++++++++++++++++++++++++++----
 xen/arch/x86/mm/p2m.c     |  8 -------
 2 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 407e299..fabcd06 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -17,6 +17,7 @@
 
 #include <xen/domain_page.h>
 #include <xen/sched.h>
+#include <asm/altp2m.h>
 #include <asm/current.h>
 #include <asm/paging.h>
 #include <asm/types.h>
@@ -1222,6 +1223,34 @@ static void ept_tlb_flush(struct p2m_domain *p2m)
     ept_sync_domain_mask(p2m, p2m->domain->dirty_cpumask);
 }
 
+static void ept_set_ad_sync(struct domain *d, bool value)
+{
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
+
+    ASSERT(p2m_locked_by_me(hostp2m));
+
+    hostp2m->ept.ad = value;
+
+    if ( unlikely(altp2m_active(d)) )
+    {
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+        {
+            struct p2m_domain *p2m;
+
+            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+                continue;
+
+            p2m = d->arch.altp2m_p2m[i];
+
+            p2m_lock(p2m);
+            p2m->ept.ad = value;
+            p2m_unlock(p2m);
+        }
+    }
+}
+
 static void ept_enable_pml(struct p2m_domain *p2m)
 {
     /* Domain must have been paused */
@@ -1236,7 +1265,7 @@ static void ept_enable_pml(struct p2m_domain *p2m)
         return;
 
     /* Enable EPT A/D bit for PML */
-    p2m->ept.ad = 1;
+    ept_set_ad_sync(p2m->domain, true);
     vmx_domain_update_eptp(p2m->domain);
 }
 
@@ -1248,10 +1277,28 @@ static void ept_disable_pml(struct p2m_domain *p2m)
     vmx_domain_disable_pml(p2m->domain);
 
     /* Disable EPT A/D bit */
-    p2m->ept.ad = 0;
+    ept_set_ad_sync(p2m->domain, false);
     vmx_domain_update_eptp(p2m->domain);
 }
 
+static void ept_enable_hardware_log_dirty(struct p2m_domain *p2m)
+{
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
+
+    p2m_lock(hostp2m);
+    ept_enable_pml(hostp2m);
+    p2m_unlock(hostp2m);
+}
+
+static void ept_disable_hardware_log_dirty(struct p2m_domain *p2m)
+{
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
+
+    p2m_lock(hostp2m);
+    ept_disable_pml(hostp2m);
+    p2m_unlock(hostp2m);
+}
+
 static void ept_flush_pml_buffers(struct p2m_domain *p2m)
 {
     /* Domain must have been paused */
@@ -1281,8 +1328,8 @@ int ept_p2m_init(struct p2m_domain *p2m)
 
     if ( cpu_has_vmx_pml )
     {
-        p2m->enable_hardware_log_dirty = ept_enable_pml;
-        p2m->disable_hardware_log_dirty = ept_disable_pml;
+        p2m->enable_hardware_log_dirty = ept_enable_hardware_log_dirty;
+        p2m->disable_hardware_log_dirty = ept_disable_hardware_log_dirty;
         p2m->flush_hardware_cached_dirty = ept_flush_pml_buffers;
     }
 
@@ -1390,8 +1437,10 @@ void setup_ept_dump(void)
 void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
 {
     struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
+    p2m->ept.ad = hostp2m->ept.ad;
     p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
     p2m->max_remapped_gfn = 0;
     ept = &p2m->ept;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a00a3c1..42b9ef4 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -360,11 +360,7 @@ void p2m_enable_hardware_log_dirty(struct domain *d)
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     if ( p2m->enable_hardware_log_dirty )
-    {
-        p2m_lock(p2m);
         p2m->enable_hardware_log_dirty(p2m);
-        p2m_unlock(p2m);
-    }
 }
 
 void p2m_disable_hardware_log_dirty(struct domain *d)
@@ -372,11 +368,7 @@ void p2m_disable_hardware_log_dirty(struct domain *d)
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     if ( p2m->disable_hardware_log_dirty )
-    {
-        p2m_lock(p2m);
         p2m->disable_hardware_log_dirty(p2m);
-        p2m_unlock(p2m);
-    }
 }
 
 void p2m_flush_hardware_cached_dirty(struct domain *d)
-- 
2.7.4


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

[PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

This patch is a pre-requisite for the one fixing VGA logdirty
freezes when using altp2m. It only concerns itself with the
ranges allocation / deallocation / initialization part. While
touching the code, I've switched 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 V2:
 - Moved the logdirty_ranges allocation / deallocation logic from
   p2m-ept.c to p2m.c on Jan's suggestion.
 - Added a comment clarifyin that the rangeset_merge() call is
   really just a copy in our context.
---
 xen/arch/x86/mm/p2m.c     | 80 +++++++++++++++++++++++++++++++++++++----------
 xen/include/asm-x86/p2m.h |  2 +-
 2 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 42b9ef4..c6b17e6 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)
 {
@@ -108,7 +130,10 @@ free_p2m:
 static void p2m_free_one(struct p2m_domain *p2m)
 {
     if ( hap_enabled(p2m->domain) && cpu_has_vmx )
+    {
+        p2m_free_logdirty(p2m);
         ept_p2m_uninit(p2m);
+    }
     free_cpumask_var(p2m->dirty_cpumask);
     xfree(p2m);
 }
@@ -116,19 +141,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 +163,7 @@ static void p2m_teardown_hostp2m(struct domain *d)
 
     if ( p2m )
     {
-        rangeset_destroy(p2m->logdirty_ranges);
+        p2m_free_logdirty(p2m);
         p2m_free_one(p2m);
         d->arch.p2m = NULL;
     }
@@ -194,6 +219,7 @@ static void p2m_teardown_altp2m(struct domain *d)
             continue;
         p2m = d->arch.altp2m_p2m[i];
         d->arch.altp2m_p2m[i] = NULL;
+        p2m_free_logdirty(p2m);
         p2m_free_one(p2m);
     }
 }
@@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
     {
         p2m_flush_table(d->arch.altp2m_p2m[i]);
         /* Uninit and reinit ept to force TLB shootdown */
+        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
         ept_p2m_uninit(d->arch.altp2m_p2m[i]);
         ept_p2m_init(d->arch.altp2m_p2m[i]);
         d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
@@ -2279,6 +2306,18 @@ void p2m_flush_altp2m(struct domain *d)
     altp2m_list_unlock(d);
 }
 
+static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
+{
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
+    int rc = p2m_init_logdirty(p2m);
+
+    if ( rc )
+        return rc;
+
+    /* The following is really just a rangeset copy. */
+    return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
+}
+
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
     int rc = -EINVAL;
@@ -2290,8 +2329,9 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
     {
-        p2m_init_altp2m_ept(d, idx);
-        rc = 0;
+        rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
+        if ( !rc )
+            p2m_init_altp2m_ept(d, idx);
     }
 
     altp2m_list_unlock(d);
@@ -2310,9 +2350,13 @@ 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_init_altp2m_logdirty(d->arch.altp2m_p2m[i]);
+
+        if ( !rc )
+        {
+            p2m_init_altp2m_ept(d, i);
+            *idx = i;
+        }
 
         break;
     }
@@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
         {
             p2m_flush_table(d->arch.altp2m_p2m[idx]);
             /* Uninit and reinit ept to force TLB shootdown */
+            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
             ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
             ept_p2m_init(d->arch.altp2m_p2m[idx]);
             d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
@@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
 {
     p2m_flush_table(p2m);
     /* Uninit and reinit ept to force TLB shootdown */
+    p2m_free_logdirty(p2m);
     ept_p2m_uninit(p2m);
     ept_p2m_init(p2m);
     p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d08c595..e905c65 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -223,7 +223,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 V3 3/3] 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
  and p2m_change_type_range() to propagate their changes to all
  valid altp2ms.

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 V2:
 - RFC: We need George's opinion on Jan's suggestion to update
   p2m-pt.c as well.
 - Dropped the p2m_ prefix from the static helpers instead of
   adding a leading underscore (except for _memory_type_changed(),
   as there's a memory_type_changed() in asm/mtrr.h, which would
   break the build.
 - Used hostp2m in p2m_change_entry_type_global() where previously
   I had missed doing so for
   _p2m_change_entry_type_global(p2m_get_hostp2m(d), ot, nt);
   (although it had been already latched).
 - Now consistenly using the "update the hostp2m first, then the
   altp2ms" pattern.
---
 xen/arch/x86/mm/p2m-ept.c |  8 +++++
 xen/arch/x86/mm/p2m.c     | 83 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index fabcd06..e6fa85f 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,6 +1443,11 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
+    p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
+    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;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c6b17e6..5fbee82 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -281,7 +281,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;
@@ -290,24 +289,48 @@ 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 *p2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(p2m);
+                change_entry_type_global(p2m, ot, nt);
+                p2m_unlock(p2m);
+            }
+    }
+#endif
+
+    p2m_unlock(hostp2m);
 }
 
-void p2m_memory_type_changed(struct domain *d)
+/* There's already a memory_type_changed() in asm/mtrr.h. */
+static void _memory_type_changed(struct p2m_domain *p2m)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
-
     if ( p2m->memory_type_changed )
     {
         p2m_lock(p2m);
@@ -316,6 +339,22 @@ void p2m_memory_type_changed(struct domain *d)
     }
 }
 
+void p2m_memory_type_changed(struct domain *d)
+{
+    _memory_type_changed(p2m_get_hostp2m(d));
+
+#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) )
+                _memory_type_changed(d->arch.altp2m_p2m[i]);
+    }
+#endif
+}
+
 int p2m_set_ioreq_server(struct domain *d,
                          unsigned int flags,
                          struct hvm_ioreq_server *s)
@@ -996,12 +1035,12 @@ 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);
@@ -1054,6 +1093,24 @@ void p2m_change_type_range(struct domain *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)
+{
+    change_type_range(p2m_get_hostp2m(d), 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) )
+                change_type_range(d->arch.altp2m_p2m[i], start, end, ot, nt);
+    }
+#endif
+}
+
 /*
  * Finish p2m type change for gfns which are marked as need_recalc in a range.
  * Returns: 0/1 for success, negative for failure
-- 
2.7.4


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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Tamas K Lengyel]

On Mon, Oct 29, 2018 at 6:41 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> This patch is a pre-requisite for the one fixing VGA logdirty
> freezes when using altp2m. It only concerns itself with the
> ranges allocation / deallocation / initialization part. While
> touching the code, I've switched global_logdirty from bool_t
> to bool.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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

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

Re: [PATCH V3 1/3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Tamas K Lengyel]

On Mon, Oct 29, 2018 at 6:42 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> This patch is a pre-requisite for fixing the logdirty VGA issue
> (display freezes when switching to a new altp2m view early in a
> domain's lifetime), but sent separately for easier review.
> The new ept_set_ad_sync() function has been added to update all
> active altp2ms' ept.ad. New altp2ms will inherit the hostp2m's
> ept.ad value.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>

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

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

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

[Tamas K Lengyel]

On Mon, Oct 29, 2018 at 6:41 AM Razvan Cojocaru
<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
>   and p2m_change_type_range() to propagate their changes to all
>   valid altp2ms.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>

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

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

Re: [PATCH V3 1/3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Jan Beulich]

>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
> This patch is a pre-requisite for fixing the logdirty VGA issue
> (display freezes when switching to a new altp2m view early in a
> domain's lifetime), but sent separately for easier review.
> The new ept_set_ad_sync() function has been added to update all
> active altp2ms' ept.ad. New altp2ms will inherit the hostp2m's
> ept.ad value.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>

I'm okay with the code changes now, but I think the altered locking
would deserve a sentence in the description.

Jan



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

Re: [PATCH V3 1/3] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Razvan Cojocaru]

On 10/30/18 6:14 PM, Jan Beulich wrote:
>>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
>> This patch is a pre-requisite for fixing the logdirty VGA issue
>> (display freezes when switching to a new altp2m view early in a
>> domain's lifetime), but sent separately for easier review.
>> The new ept_set_ad_sync() function has been added to update all
>> active altp2ms' ept.ad. New altp2ms will inherit the hostp2m's
>> ept.ad value.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
> I'm okay with the code changes now, but I think the altered locking
> would deserve a sentence in the description.

Of course, I'll change the description in the next iteration of the series.


Thanks,
Razvan

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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Jan Beulich]

>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
> This patch is a pre-requisite for the one fixing VGA logdirty
> freezes when using altp2m. It only concerns itself with the
> ranges allocation / deallocation / initialization part.

But while looking (briefly only for now) over patch 3 I couldn't
see any sync-ing of the log-dirty ranges there either. Doesn't
this need doing either there or here, if you go the copy route?

> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>      {
>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>          /* Uninit and reinit ept to force TLB shootdown */
> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>          {
>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>              /* Uninit and reinit ept to force TLB shootdown */
> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>  {
>      p2m_flush_table(p2m);
>      /* Uninit and reinit ept to force TLB shootdown */
> +    p2m_free_logdirty(p2m);
>      ept_p2m_uninit(p2m);
>      ept_p2m_init(p2m);
>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);

For one these look all pretty similar, so I wonder why there's
no helper function. But that's not something you need to change.
Yet why are you freeing the log-dirty ranges here? These aren't
full cleanup paths afaict.

Jan



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

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

[Jan Beulich]

>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
> @@ -316,6 +339,22 @@ void p2m_memory_type_changed(struct domain *d)
>      }
>  }
>  
> +void p2m_memory_type_changed(struct domain *d)
> +{
> +    _memory_type_changed(p2m_get_hostp2m(d));
> +
> +#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) )
> +                _memory_type_changed(d->arch.altp2m_p2m[i]);
> +    }
> +#endif
> +}

Hmm, I'm puzzled by the #ifdef placement. Wei, didn't we settle that
this code is HVM only altogether?

Jan



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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Andrew Cooper]

On 30/10/18 16:22, Jan Beulich wrote:
>>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
>> This patch is a pre-requisite for the one fixing VGA logdirty
>> freezes when using altp2m. It only concerns itself with the
>> ranges allocation / deallocation / initialization part.
> But while looking (briefly only for now) over patch 3 I couldn't
> see any sync-ing of the log-dirty ranges there either. Doesn't
> this need doing either there or here, if you go the copy route?
>
>> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>>      {
>>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>>          /* Uninit and reinit ept to force TLB shootdown */
>> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>          {
>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>              /* Uninit and reinit ept to force TLB shootdown */
>> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>>  {
>>      p2m_flush_table(p2m);
>>      /* Uninit and reinit ept to force TLB shootdown */
>> +    p2m_free_logdirty(p2m);
>>      ept_p2m_uninit(p2m);
>>      ept_p2m_init(p2m);
>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
> For one these look all pretty similar, so I wonder why there's
> no helper function. But that's not something you need to change.
> Yet why are you freeing the log-dirty ranges here? These aren't
> full cleanup paths afaict.

Rangesets get added to the domain rangeset list, and we clean them all
up rangeset_domain_destroy()

TBH, I'm not sure why we do it like this, and I'm not 100% convinced it
is a clever deallocation scheme.

~Andrew

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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 10/30/18 6:22 PM, Jan Beulich wrote:
>>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
>> This patch is a pre-requisite for the one fixing VGA logdirty
>> freezes when using altp2m. It only concerns itself with the
>> ranges allocation / deallocation / initialization part.
> 
> But while looking (briefly only for now) over patch 3 I couldn't
> see any sync-ing of the log-dirty ranges there either. Doesn't
> this need doing either there or here, if you go the copy route?

The syncing is done in patch 3, in p2m_change_entry_type_global(),
p2m_memory_type_changed(), p2m_change_type_range(), where now all active
p2ms (hostp2m + altp2ms) receive the same changes.

On allocating the logdirty_ranges for a new altp2m, the hostp2m (which
is sync with all others) is copied over, and then whatever logdirty
ranges the hypervisor is using for reading should be fine.

That was the thinking at least, am I missing something?

>> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>>      {
>>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>>          /* Uninit and reinit ept to force TLB shootdown */
>> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>          {
>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>              /* Uninit and reinit ept to force TLB shootdown */
>> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>>  {
>>      p2m_flush_table(p2m);
>>      /* Uninit and reinit ept to force TLB shootdown */
>> +    p2m_free_logdirty(p2m);
>>      ept_p2m_uninit(p2m);
>>      ept_p2m_init(p2m);
>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
> 
> For one these look all pretty similar, so I wonder why there's
> no helper function. But that's not something you need to change.
> Yet why are you freeing the log-dirty ranges here? These aren't
> full cleanup paths afaict.

After the first iteration Andrew has suggested that I should free in
ept_p2m_uninit(p2m):

https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01430.html

at which point I did so for V2, where you've suggested that that
function should be static and live in p2m.c. Which led to V3 where
basically I've plopped this function right before ept_p2m_uninit(p2m)
wherever it was called to match the V2 behaviour.


Thanks,
Razvan

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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Jan Beulich]

>>> On 30.10.18 at 17:32, <rcojocaru@bitdefender.com> wrote:
> On 10/30/18 6:22 PM, Jan Beulich wrote:
>>>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
>>> This patch is a pre-requisite for the one fixing VGA logdirty
>>> freezes when using altp2m. It only concerns itself with the
>>> ranges allocation / deallocation / initialization part.
>> 
>> But while looking (briefly only for now) over patch 3 I couldn't
>> see any sync-ing of the log-dirty ranges there either. Doesn't
>> this need doing either there or here, if you go the copy route?
> 
> The syncing is done in patch 3, in p2m_change_entry_type_global(),
> p2m_memory_type_changed(), p2m_change_type_range(), where now all active
> p2ms (hostp2m + altp2ms) receive the same changes.
> 
> On allocating the logdirty_ranges for a new altp2m, the hostp2m (which
> is sync with all others) is copied over, and then whatever logdirty
> ranges the hypervisor is using for reading should be fine.
> 
> That was the thinking at least, am I missing something?

Ah, yes, I was misremembering how the log-dirty rangesets
get maintained. Sorry.

>>> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>>>      {
>>>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>>>          /* Uninit and reinit ept to force TLB shootdown */
>>> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>>>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>>>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>>>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>>> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>>          {
>>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>>              /* Uninit and reinit ept to force TLB shootdown */
>>> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>>> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>>>  {
>>>      p2m_flush_table(p2m);
>>>      /* Uninit and reinit ept to force TLB shootdown */
>>> +    p2m_free_logdirty(p2m);
>>>      ept_p2m_uninit(p2m);
>>>      ept_p2m_init(p2m);
>>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>> 
>> For one these look all pretty similar, so I wonder why there's
>> no helper function. But that's not something you need to change.
>> Yet why are you freeing the log-dirty ranges here? These aren't
>> full cleanup paths afaict.
> 
> After the first iteration Andrew has suggested that I should free in
> ept_p2m_uninit(p2m):
> 
> https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01430.html 
> 
> at which point I did so for V2, where you've suggested that that
> function should be static and live in p2m.c. Which led to V3 where
> basically I've plopped this function right before ept_p2m_uninit(p2m)
> wherever it was called to match the V2 behaviour.

I don't care much about the history of from where exactly to
do the cleanup you do here; I'd like to understand _why_ this
is needed, when this not a p2m destruction path. IOW why is
the original rangeset structure no longer needed? Wouldn't it
be re-allocated anyway later on, in which case you could just
empty it (if that's indeed needed at these points)?

Jan



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

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

[Wei Liu]

On Tue, Oct 30, 2018 at 10:25:20AM -0600, Jan Beulich wrote:
> >>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
> > @@ -316,6 +339,22 @@ void p2m_memory_type_changed(struct domain *d)
> >      }
> >  }
> >  
> > +void p2m_memory_type_changed(struct domain *d)
> > +{
> > +    _memory_type_changed(p2m_get_hostp2m(d));
> > +
> > +#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) )
> > +                _memory_type_changed(d->arch.altp2m_p2m[i]);
> > +    }
> > +#endif
> > +}
> 
> Hmm, I'm puzzled by the #ifdef placement. Wei, didn't we settle that
> this code is HVM only altogether?

Its callers live in hvm/mtrr.c which don't get built when HVM is
disabled. We rely on DCE to purge this function when linking.

With the introduction of altp2m fields in this function it may now be
better to put the whole function under CONFIG_HVM.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 10/30/18 6:28 PM, Andrew Cooper wrote:
> On 30/10/18 16:22, Jan Beulich wrote:
>>>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
>>> This patch is a pre-requisite for the one fixing VGA logdirty
>>> freezes when using altp2m. It only concerns itself with the
>>> ranges allocation / deallocation / initialization part.
>> But while looking (briefly only for now) over patch 3 I couldn't
>> see any sync-ing of the log-dirty ranges there either. Doesn't
>> this need doing either there or here, if you go the copy route?
>>
>>> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>>>      {
>>>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>>>          /* Uninit and reinit ept to force TLB shootdown */
>>> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>>>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>>>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>>>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>>> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>>          {
>>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>>              /* Uninit and reinit ept to force TLB shootdown */
>>> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>>> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>>>  {
>>>      p2m_flush_table(p2m);
>>>      /* Uninit and reinit ept to force TLB shootdown */
>>> +    p2m_free_logdirty(p2m);
>>>      ept_p2m_uninit(p2m);
>>>      ept_p2m_init(p2m);
>>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>> For one these look all pretty similar, so I wonder why there's
>> no helper function. But that's not something you need to change.
>> Yet why are you freeing the log-dirty ranges here? These aren't
>> full cleanup paths afaict.
> 
> Rangesets get added to the domain rangeset list, and we clean them all
> up rangeset_domain_destroy()
> 
> TBH, I'm not sure why we do it like this, and I'm not 100% convinced it
> is a clever deallocation scheme.

To eliminate any confusion: are you saying that rangesets should only be
allocated, and never explicitly deallocated (since
rangeset_domain_destroy() takes care of that)? If that is correct, then
there's a problem in the code now with the way we're handling the
logdirty_ranges for the hostp2m (where we clean it up in p2m_free_one()
and p2m_teardown_hostp2m()).


Thanks,
Razvan

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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Andrew Cooper]

On 30/10/18 16:51, Razvan Cojocaru wrote:
> On 10/30/18 6:28 PM, Andrew Cooper wrote:
>> On 30/10/18 16:22, Jan Beulich wrote:
>>>>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
>>>> This patch is a pre-requisite for the one fixing VGA logdirty
>>>> freezes when using altp2m. It only concerns itself with the
>>>> ranges allocation / deallocation / initialization part.
>>> But while looking (briefly only for now) over patch 3 I couldn't
>>> see any sync-ing of the log-dirty ranges there either. Doesn't
>>> this need doing either there or here, if you go the copy route?
>>>
>>>> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>>>>      {
>>>>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>>>>          /* Uninit and reinit ept to force TLB shootdown */
>>>> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>>>>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>>>>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>>>>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>>>> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>>>          {
>>>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>>>              /* Uninit and reinit ept to force TLB shootdown */
>>>> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>>>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>>>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>>>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>>>> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>>>>  {
>>>>      p2m_flush_table(p2m);
>>>>      /* Uninit and reinit ept to force TLB shootdown */
>>>> +    p2m_free_logdirty(p2m);
>>>>      ept_p2m_uninit(p2m);
>>>>      ept_p2m_init(p2m);
>>>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>>> For one these look all pretty similar, so I wonder why there's
>>> no helper function. But that's not something you need to change.
>>> Yet why are you freeing the log-dirty ranges here? These aren't
>>> full cleanup paths afaict.
>> Rangesets get added to the domain rangeset list, and we clean them all
>> up rangeset_domain_destroy()
>>
>> TBH, I'm not sure why we do it like this, and I'm not 100% convinced it
>> is a clever deallocation scheme.
> To eliminate any confusion: are you saying that rangesets should only be
> allocated, and never explicitly deallocated (since
> rangeset_domain_destroy() takes care of that)?

No, because that becomes (effectively) a memory leak each time we create
a new view.

>  If that is correct, then
> there's a problem in the code now with the way we're handling the
> logdirty_ranges for the hostp2m (where we clean it up in p2m_free_one()
> and p2m_teardown_hostp2m()).

To answer Jan's question, the reason you are destroying/recreating the
rangeset is because we've got no clear API.  Perhaps fixing that is the
better course of action.

~Andrew

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

Re: [PATCH V3 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 10/30/18 6:54 PM, Andrew Cooper wrote:
> On 30/10/18 16:51, Razvan Cojocaru wrote:
>> On 10/30/18 6:28 PM, Andrew Cooper wrote:
>>> On 30/10/18 16:22, Jan Beulich wrote:
>>>>>>> On 29.10.18 at 13:40, <rcojocaru@bitdefender.com> wrote:
>>>>> This patch is a pre-requisite for the one fixing VGA logdirty
>>>>> freezes when using altp2m. It only concerns itself with the
>>>>> ranges allocation / deallocation / initialization part.
>>>> But while looking (briefly only for now) over patch 3 I couldn't
>>>> see any sync-ing of the log-dirty ranges there either. Doesn't
>>>> this need doing either there or here, if you go the copy route?
>>>>
>>>>> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>>>>>      {
>>>>>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>>>>>          /* Uninit and reinit ept to force TLB shootdown */
>>>>> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>>>>>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>>>>>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>>>>>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>>>>> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>>>>          {
>>>>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>>>>              /* Uninit and reinit ept to force TLB shootdown */
>>>>> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>>>>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>>>>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>>>>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>>>>> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>>>>>  {
>>>>>      p2m_flush_table(p2m);
>>>>>      /* Uninit and reinit ept to force TLB shootdown */
>>>>> +    p2m_free_logdirty(p2m);
>>>>>      ept_p2m_uninit(p2m);
>>>>>      ept_p2m_init(p2m);
>>>>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>>>> For one these look all pretty similar, so I wonder why there's
>>>> no helper function. But that's not something you need to change.
>>>> Yet why are you freeing the log-dirty ranges here? These aren't
>>>> full cleanup paths afaict.
>>> Rangesets get added to the domain rangeset list, and we clean them all
>>> up rangeset_domain_destroy()
>>>
>>> TBH, I'm not sure why we do it like this, and I'm not 100% convinced it
>>> is a clever deallocation scheme.
>> To eliminate any confusion: are you saying that rangesets should only be
>> allocated, and never explicitly deallocated (since
>> rangeset_domain_destroy() takes care of that)?
> 
> No, because that becomes (effectively) a memory leak each time we create
> a new view.

Not really, since the patch checks if ( p2m->logdirty_ranges ) in the
new p2m_init_logdirty() function, and does not allocate in that case.
However, that might be wrong with regard to rangeset_merge() if
de-allocations don't happen when they do now.

>>  If that is correct, then
>> there's a problem in the code now with the way we're handling the
>> logdirty_ranges for the hostp2m (where we clean it up in p2m_free_one()
>> and p2m_teardown_hostp2m()).
> 
> To answer Jan's question, the reason you are destroying/recreating the
> rangeset is because we've got no clear API.  Perhaps fixing that is the
> better course of action.

Fair enough, but if possible it would be great if we could get this
working upstream as soon as possible - both Tamas and us think of this
as a high-priority problem, since it's basically a dealbreaker for using
altp2m.

I'm happy to follow Jan's suggestion of keeping the altp2m rangeset
around until p2m_teardown_altp2m() and simply empty it before merging,
and we can discuss refactoring the API after the fix.

Is this acceptable to everyone? If so, is p2m_teardown_altp2m() the
ideal place to clean up?


Thanks,
Razvan

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