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

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

The first patch is the same as:
[PATCH V4] x86/altp2m: propagate ept.ad changes to all active altp2ms
but as it is now required for the rest of the series to apply cleanly,
it has been resent as part of this series.

[PATCH V2 1/3] x86/altp2m: propagate ept.ad changes to all active altp2ms
[PATCH V2 2/3] x86/mm: allocate logdirty_ranges for altp2ms
[PATCH V2 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 V2 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>
---
 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 V2 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>
---
 xen/arch/x86/mm/p2m-ept.c         | 15 ++++++++++-
 xen/arch/x86/mm/p2m.c             | 55 ++++++++++++++++++++++++++-------------
 xen/include/asm-x86/hvm/vmx/vmx.h |  2 +-
 xen/include/asm-x86/p2m.h         |  5 +++-
 4 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index fabcd06..989917c 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1348,6 +1348,8 @@ int ept_p2m_init(struct p2m_domain *p2m)
 void ept_p2m_uninit(struct p2m_domain *p2m)
 {
     struct ept_data *ept = &p2m->ept;
+
+    p2m_free_logdirty(p2m);
     free_cpumask_var(ept->invalidate);
 }
 
@@ -1434,11 +1436,20 @@ void setup_ept_dump(void)
     register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
 }
 
-void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
+int 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;
+    int rc = p2m_init_logdirty(p2m);
+
+    if ( rc )
+        return rc;
+
+    rc = rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
+
+    if ( rc )
+        return rc;
 
     p2m->ept.ad = hostp2m->ept.ad;
     p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
@@ -1446,6 +1457,8 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     ept = &p2m->ept;
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
     d->arch.altp2m_eptp[i] = ept->eptp;
+
+    return 0;
 }
 
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 42b9ef4..b37db06 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -113,22 +113,44 @@ static void p2m_free_one(struct p2m_domain *p2m)
     xfree(p2m);
 }
 
+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;
+}
+
+void p2m_free_logdirty(struct p2m_domain *p2m)
+{
+    if ( !p2m->logdirty_ranges )
+        return;
+
+    rangeset_destroy(p2m->logdirty_ranges);
+    p2m->logdirty_ranges = NULL;
+}
+
 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 +160,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 +216,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);
     }
 }
@@ -2289,10 +2312,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_init_altp2m_ept(d, idx);
 
     altp2m_list_unlock(d);
     return rc;
@@ -2310,9 +2330,8 @@ 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);
+        rc = p2m_init_altp2m_ept(d, i);
         *idx = i;
-        rc = 0;
 
         break;
     }
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index b110e16..19730c7 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -598,7 +598,7 @@ void ept_p2m_uninit(struct p2m_domain *p2m);
 void ept_walk_table(struct domain *d, unsigned long gfn);
 bool_t ept_handle_misconfig(uint64_t gpa);
 void setup_ept_dump(void);
-void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
+int p2m_init_altp2m_ept(struct domain *d, unsigned int i);
 /* Locate an alternate p2m by its EPTP */
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d08c595..aa52212 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 
@@ -581,6 +581,9 @@ int p2m_alloc_table(struct p2m_domain *p2m);
 void p2m_teardown(struct p2m_domain *p2m);
 void p2m_final_teardown(struct domain *d);
 
+int p2m_init_logdirty(struct p2m_domain *p2m);
+void p2m_free_logdirty(struct p2m_domain *p2m);
+
 /* Add a page to a domain's p2m table */
 int guest_physmap_add_entry(struct domain *d, gfn_t gfn,
                             mfn_t mfn, unsigned int page_order,
-- 
2.7.4


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

[PATCH V2 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>
---
 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 989917c..5cac304 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;
@@ -1451,6 +1454,11 @@ int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     if ( rc )
         return rc;
 
+    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 b37db06..4de1372 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -278,7 +278,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;
@@ -287,24 +286,47 @@ int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
     return 0;
 }
 
+static void _p2m_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);
+
+    _p2m_change_entry_type_global(p2m_get_hostp2m(d), 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);
+                _p2m_change_entry_type_global(p2m, ot, nt);
+                p2m_unlock(p2m);
+            }
+    }
+#endif
+
+    p2m_unlock(hostp2m);
 }
 
-void p2m_memory_type_changed(struct domain *d)
+static void _p2m_memory_type_changed(struct p2m_domain *p2m)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
-
     if ( p2m->memory_type_changed )
     {
         p2m_lock(p2m);
@@ -313,6 +335,22 @@ void p2m_memory_type_changed(struct domain *d)
     }
 }
 
+void p2m_memory_type_changed(struct domain *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) )
+                _p2m_memory_type_changed(d->arch.altp2m_p2m[i]);
+    }
+#endif
+
+    _p2m_memory_type_changed(p2m_get_hostp2m(d));
+}
+
 int p2m_set_ioreq_server(struct domain *d,
                          unsigned int flags,
                          struct hvm_ioreq_server *s)
@@ -993,12 +1031,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 _p2m_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);
@@ -1051,6 +1089,25 @@ 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)
+{
+#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) )
+                _p2m_change_type_range(d->arch.altp2m_p2m[i], start, end, ot,
+                                       nt);
+    }
+#endif
+
+    _p2m_change_type_range(p2m_get_hostp2m(d), start, end, ot, nt);
+}
+
 /*
  * 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 V2 3/3] x86/altp2m: fix display frozen when switching to a new view early

[Jan Beulich]

>>> On 23.10.18 at 15:19, <rcojocaru@bitdefender.com> wrote:
>  xen/arch/x86/mm/p2m-ept.c |  8 +++++
>  xen/arch/x86/mm/p2m.c     | 83 +++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 78 insertions(+), 13 deletions(-)

What about p2m-pt.c?

> @@ -287,24 +286,47 @@ int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
>      return 0;
>  }
>  
> +static void _p2m_change_entry_type_global(struct p2m_domain *p2m,

Instead of prefixing another underscore, why don't you
drop p2m_ from this static helper?

> +                                          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);
> +
> +    _p2m_change_entry_type_global(p2m_get_hostp2m(d), ot, nt);

Use hostp2m here too?

> +
> +#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);
> +                _p2m_change_entry_type_global(p2m, ot, nt);
> +                p2m_unlock(p2m);
> +            }
> +    }
> +#endif
> +
> +    p2m_unlock(hostp2m);
>  }
>  
> -void p2m_memory_type_changed(struct domain *d)
> +static void _p2m_memory_type_changed(struct p2m_domain *p2m)

Same as above.

> @@ -313,6 +335,22 @@ void p2m_memory_type_changed(struct domain *d)
>      }
>  }
>  
> +void p2m_memory_type_changed(struct domain *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) )
> +                _p2m_memory_type_changed(d->arch.altp2m_p2m[i]);
> +    }
> +#endif
> +
> +    _p2m_memory_type_changed(p2m_get_hostp2m(d));
> +}

Is there a particular reason this one doesn't follow the host-then-
alt pattern used above and elsewhere? If so, a comment should
be attached. If not, using that same model would seem better.
(Same then below for the range function.)

Jan



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

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

[Jan Beulich]

>>> On 23.10.18 at 15:19, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1348,6 +1348,8 @@ int ept_p2m_init(struct p2m_domain *p2m)
>  void ept_p2m_uninit(struct p2m_domain *p2m)
>  {
>      struct ept_data *ept = &p2m->ept;
> +
> +    p2m_free_logdirty(p2m);

This and ...

>      free_cpumask_var(ept->invalidate);
>  }
>  
> @@ -1434,11 +1436,20 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>  
> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> +int 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;
> +    int rc = p2m_init_logdirty(p2m);
> +
> +    if ( rc )
> +        return rc;
> +
> +    rc = rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
> +
> +    if ( rc )
> +        return rc;

... all of this logically belong into p2m.c, as it's not code which is
EPT-specific. This would also eliminate the need to make the two
functions non-static.

Also I think the rangeset_merge() invocation would deserve a
brief comment noting that this "merge" is really just a "copy".

Jan



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

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

[Razvan Cojocaru]

On 10/26/18 5:47 PM, Jan Beulich wrote:
>>>> On 23.10.18 at 15:19, <rcojocaru@bitdefender.com> wrote:
>>  xen/arch/x86/mm/p2m-ept.c |  8 +++++
>>  xen/arch/x86/mm/p2m.c     | 83 +++++++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 78 insertions(+), 13 deletions(-)
> 
> What about p2m-pt.c?

Thank you for the review!

I thought altp2m was an EPT-only tool, do you mean that I should also
iterate over possibly active altp2ms in p2m_pt_handle_deferred_changes()
there as well?

>> @@ -287,24 +286,47 @@ int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
>>      return 0;
>>  }
>>  
>> +static void _p2m_change_entry_type_global(struct p2m_domain *p2m,
> 
> Instead of prefixing another underscore, why don't you
> drop p2m_ from this static helper?

Of course, I'll do that.

>> +                                          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);
>> +
>> +    _p2m_change_entry_type_global(p2m_get_hostp2m(d), ot, nt);
> 
> Use hostp2m here too?

Right, I should have. I'll change it.

>> +
>> +#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);
>> +                _p2m_change_entry_type_global(p2m, ot, nt);
>> +                p2m_unlock(p2m);
>> +            }
>> +    }
>> +#endif
>> +
>> +    p2m_unlock(hostp2m);
>>  }
>>  
>> -void p2m_memory_type_changed(struct domain *d)
>> +static void _p2m_memory_type_changed(struct p2m_domain *p2m)
> 
> Same as above.

Will change it.

>> @@ -313,6 +335,22 @@ void p2m_memory_type_changed(struct domain *d)
>>      }
>>  }
>>  
>> +void p2m_memory_type_changed(struct domain *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) )
>> +                _p2m_memory_type_changed(d->arch.altp2m_p2m[i]);
>> +    }
>> +#endif
>> +
>> +    _p2m_memory_type_changed(p2m_get_hostp2m(d));
>> +}
> 
> Is there a particular reason this one doesn't follow the host-then-
> alt pattern used above and elsewhere? If so, a comment should
> be attached. If not, using that same model would seem better.
> (Same then below for the range function.)

No reason, no. I'll make them consistent.


Thanks,
Razvan

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

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

[Razvan Cojocaru]

On 10/26/18 5:54 PM, Jan Beulich wrote:
>>>> On 23.10.18 at 15:19, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1348,6 +1348,8 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>  void ept_p2m_uninit(struct p2m_domain *p2m)
>>  {
>>      struct ept_data *ept = &p2m->ept;
>> +
>> +    p2m_free_logdirty(p2m);
> 
> This and ...
> 
>>      free_cpumask_var(ept->invalidate);
>>  }
>>  
>> @@ -1434,11 +1436,20 @@ void setup_ept_dump(void)
>>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>>  }
>>  
>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>> +int 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;
>> +    int rc = p2m_init_logdirty(p2m);
>> +
>> +    if ( rc )
>> +        return rc;
>> +
>> +    rc = rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
>> +
>> +    if ( rc )
>> +        return rc;
> 
> ... all of this logically belong into p2m.c, as it's not code which is
> EPT-specific. This would also eliminate the need to make the two
> functions non-static.
> 
> Also I think the rangeset_merge() invocation would deserve a
> brief comment noting that this "merge" is really just a "copy".

I'll move them to p2m.c.


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 26.10.18 at 17:00, <rcojocaru@bitdefender.com> wrote:
> On 10/26/18 5:47 PM, Jan Beulich wrote:
>>>>> On 23.10.18 at 15:19, <rcojocaru@bitdefender.com> wrote:
>>>  xen/arch/x86/mm/p2m-ept.c |  8 +++++
>>>  xen/arch/x86/mm/p2m.c     | 83 
> +++++++++++++++++++++++++++++++++++++++--------
>>>  2 files changed, 78 insertions(+), 13 deletions(-)
>> 
>> What about p2m-pt.c?
> 
> I thought altp2m was an EPT-only tool,

Right, I had thought of this after sending, yet I still think ...

> do you mean that I should also
> iterate over possibly active altp2ms in p2m_pt_handle_deferred_changes()
> there as well?

... having that code in sync _might_ be a good idea. Wait for
George's opinion though before you invest time there.

Jan



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