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

[PATCH V4 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 V4 1/3] x86/altp2m: propagate ept.ad changes to all active
[PATCH V4 2/3] x86/mm: allocate logdirty_ranges for altp2ms
[PATCH V4 3/3] x86/altp2m: fix display frozen when switching to a new


Thanks,
Razvan

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

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

The p2m_{en,dis}able_hardware_log_dirty() hostp2m locking has
been moved to the new ept_{en,dis}able_hardware_log_dirty()
functions as part of the code refactoring, while locks for the
individual altp2ms are taken in ept_set_ad_sync() (called by
ept_{en,dis}able_pml()).

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Suggested-by: George Dunlap <george.dunlap@citrix.com>
Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

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

---
Changes since V3:
 - Expanded the comment to bring attention to the altered locking.
---
 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 V4 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 V3:
- Removed all added p2m_free_logdirty() calls happening before
  p2m_free_one(), since p2m_free_one() already calls
  p2m_free_logdirty().
- Removed the p2m_free_logdirty() call from p2m_reset_altp2m().
  Jan has rightly asked about its placement, and on further review
  it was plain wrong: the rangeset would have been freed but not
  re-allocated in this case, so a p2m_reset_altp2m() would have
  just led to using a NULL logdirty_ranges pointer.
---
 xen/arch/x86/mm/p2m.c     | 76 ++++++++++++++++++++++++++++++++++++-----------
 xen/include/asm-x86/p2m.h |  2 +-
 2 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 42b9ef4..bc6e543 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,6 @@ static void p2m_teardown_hostp2m(struct domain *d)
 
     if ( p2m )
     {
-        rangeset_destroy(p2m->logdirty_ranges);
         p2m_free_one(p2m);
         d->arch.p2m = NULL;
     }
@@ -2279,6 +2303,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 +2326,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 +2347,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 +2382,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);
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 V4 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 V3:
 - RFC: We need George's opinion on Jan's suggestion to update
   p2m-pt.c as well.
 - Fixed mis-indented line in change_type_range().
 - Moved p2m_memory_type_changed() (and static helper) under
   #ifdef CONFIG_HVM.
---
 xen/arch/x86/mm/p2m-ept.c |  8 +++++
 xen/arch/x86/mm/p2m.c     | 83 +++++++++++++++++++++++++++++++++++++++--------
 xen/include/asm-x86/p2m.h |  6 ++--
 3 files changed, 81 insertions(+), 16 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 bc6e543..70e436d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -279,7 +279,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;
@@ -288,24 +287,49 @@ 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)
+#ifdef CONFIG_HVM
+/* 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);
@@ -314,6 +338,21 @@ void p2m_memory_type_changed(struct domain *d)
     }
 }
 
+void p2m_memory_type_changed(struct domain *d)
+{
+    _memory_type_changed(p2m_get_hostp2m(d));
+
+    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)
@@ -994,12 +1033,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);
@@ -1052,6 +1091,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
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index e905c65..27fd4f2 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -625,9 +625,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);
 
@@ -658,6 +655,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

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

[George Dunlap]

On 11/01/2018 02:45 PM, Razvan Cojocaru 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.
> 
> The p2m_{en,dis}able_hardware_log_dirty() hostp2m locking has
> been moved to the new ept_{en,dis}able_hardware_log_dirty()
> functions as part of the code refactoring, while locks for the
> individual altp2ms are taken in ept_set_ad_sync() (called by
> ept_{en,dis}able_pml()).
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> Tested-by: Tamas K Lengyel <tamas@tklengyel.com>

Sorry for the delay:

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


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

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

[Razvan Cojocaru]

On 11/8/18 6:56 PM, George Dunlap wrote:
> On 11/01/2018 02:45 PM, Razvan Cojocaru 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.
>>
>> The p2m_{en,dis}able_hardware_log_dirty() hostp2m locking has
>> been moved to the new ept_{en,dis}able_hardware_log_dirty()
>> functions as part of the code refactoring, while locks for the
>> individual altp2ms are taken in ept_set_ad_sync() (called by
>> ept_{en,dis}able_pml()).
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>> Tested-by: Tamas K Lengyel <tamas@tklengyel.com>
> 
> Sorry for the delay:
> 
> Acked-by: George Dunlap <george.dunlap@citrix.com>

Thanks!


Much appreciated,
Razvan

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

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

[George Dunlap]

On 11/01/2018 02:45 PM, Razvan Cojocaru wrote:
> When an new altp2m view is created very early on guest boot, the
> display will freeze (although the guest will run normally). This
> may also happen on resizing the display. The reason is the way
> Xen currently (mis)handles logdirty VGA: it intentionally
> misconfigures VGA pages so that they will fault.
> 
> The problem is that it only does this in the host p2m. Once we
> switch to a new altp2m, the misconfigured entries will no longer
> fault, so the display will not be updated.
> 
> This patch:
> * updates ept_handle_misconfig() to use the active altp2m instead
>   of the hostp2m;
> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>   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>

Hey Razvan,

Sorry for taking so long to get to this.  At a high level this looks
good.  One answer and one other comment...

> 
> ---
> 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 V3:
>  - RFC: We need George's opinion on Jan's suggestion to update
>    p2m-pt.c as well.

Hmm -- if the only change is to add the `p2m_get_altp2m()` clause from
ept_handle_misconfig(), it wouldn't be terrible; still, it's extra code
and an extra branch that will never be executed.

I think I'd rather put an ASSERT(!altp2m_active()) there instead, with a
comment to the effect that if altp2m is ever enabled for NPT / shadow,
we'll have to use the altp2m; and to see ept_handle_misconfig().

>  - Fixed mis-indented line in change_type_range().
>  - Moved p2m_memory_type_changed() (and static helper) under
>    #ifdef CONFIG_HVM.
> ---
>  xen/arch/x86/mm/p2m-ept.c |  8 +++++
>  xen/arch/x86/mm/p2m.c     | 83 +++++++++++++++++++++++++++++++++++++++--------
>  xen/include/asm-x86/p2m.h |  6 ++--
>  3 files changed, 81 insertions(+), 16 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 bc6e543..70e436d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -279,7 +279,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;
> @@ -288,24 +287,49 @@ 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);

So here you hold the host p2m lock while updating all the altp2m locks.
That sounds like it's probably necessary; but do you remember the
locking discipline?  Is that allowed, and/or do we ever grab the altp2m
lock and then the hostp2m lock?

But then...

>  }
>  
> -void p2m_memory_type_changed(struct domain *d)
> +#ifdef CONFIG_HVM
> +/* 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);
> @@ -314,6 +338,21 @@ void p2m_memory_type_changed(struct domain *d)
>      }
>  }
>  
> +void p2m_memory_type_changed(struct domain *d)
> +{
> +    _memory_type_changed(p2m_get_hostp2m(d));
> +
> +    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

...here and...

> +
>  int p2m_set_ioreq_server(struct domain *d,
>                           unsigned int flags,
>                           struct hvm_ioreq_server *s)
> @@ -994,12 +1033,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);
> @@ -1052,6 +1091,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
> +}
> +

...here you grab & release each lock separately, inside the update
function.  memory_type_changed is probably more or less idempotent, so
won't matter if two different calls race; but it seems likely that if
two p2m_change_type_range() calls happen concurrently, the various
altp2ms will get different results.  Is it worth refactoring both of
these so that, like change_entry_type_global, you hold the host p2m lock
while you change the individual altp2m locks?

 -George

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

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

[Razvan Cojocaru]

On 11/8/18 8:14 PM, George Dunlap wrote:
> On 11/01/2018 02:45 PM, Razvan Cojocaru wrote:
>> When an new altp2m view is created very early on guest boot, the
>> display will freeze (although the guest will run normally). This
>> may also happen on resizing the display. The reason is the way
>> Xen currently (mis)handles logdirty VGA: it intentionally
>> misconfigures VGA pages so that they will fault.
>>
>> The problem is that it only does this in the host p2m. Once we
>> switch to a new altp2m, the misconfigured entries will no longer
>> fault, so the display will not be updated.
>>
>> This patch:
>> * updates ept_handle_misconfig() to use the active altp2m instead
>>   of the hostp2m;
>> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>>   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>
> 
> Hey Razvan,
> 
> Sorry for taking so long to get to this.  At a high level this looks
> good.  One answer and one other comment...

No problem, thanks for the review!

>> ---
>> 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 V3:
>>  - RFC: We need George's opinion on Jan's suggestion to update
>>    p2m-pt.c as well.
> 
> Hmm -- if the only change is to add the `p2m_get_altp2m()` clause from
> ept_handle_misconfig(), it wouldn't be terrible; still, it's extra code
> and an extra branch that will never be executed.
> 
> I think I'd rather put an ASSERT(!altp2m_active()) there instead, with a
> comment to the effect that if altp2m is ever enabled for NPT / shadow,
> we'll have to use the altp2m; and to see ept_handle_misconfig().

Will do.

>>  - Fixed mis-indented line in change_type_range().
>>  - Moved p2m_memory_type_changed() (and static helper) under
>>    #ifdef CONFIG_HVM.
>> ---
>>  xen/arch/x86/mm/p2m-ept.c |  8 +++++
>>  xen/arch/x86/mm/p2m.c     | 83 +++++++++++++++++++++++++++++++++++++++--------
>>  xen/include/asm-x86/p2m.h |  6 ++--
>>  3 files changed, 81 insertions(+), 16 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 bc6e543..70e436d 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -279,7 +279,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;
>> @@ -288,24 +287,49 @@ 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);
> 
> So here you hold the host p2m lock while updating all the altp2m locks.
> That sounds like it's probably necessary; but do you remember the
> locking discipline?  Is that allowed, and/or do we ever grab the altp2m
> lock and then the hostp2m lock?

Locking the hostp2m first seems to be the pattern everywhere I've looked
(for example, in p2m_change_altp2m_gfn(), p2m_set_suppress_ve(),
p2m_get_suppress_ve(), or p2m_set_mem_access()).

> But then...
> 
>>  }
>>  
>> -void p2m_memory_type_changed(struct domain *d)
>> +#ifdef CONFIG_HVM
>> +/* 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);
>> @@ -314,6 +338,21 @@ void p2m_memory_type_changed(struct domain *d)
>>      }
>>  }
>>  
>> +void p2m_memory_type_changed(struct domain *d)
>> +{
>> +    _memory_type_changed(p2m_get_hostp2m(d));
>> +
>> +    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
> 
> ...here and...
> 
>> +
>>  int p2m_set_ioreq_server(struct domain *d,
>>                           unsigned int flags,
>>                           struct hvm_ioreq_server *s)
>> @@ -994,12 +1033,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);
>> @@ -1052,6 +1091,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
>> +}
>> +
> 
> ...here you grab & release each lock separately, inside the update
> function.  memory_type_changed is probably more or less idempotent, so
> won't matter if two different calls race; but it seems likely that if
> two p2m_change_type_range() calls happen concurrently, the various
> altp2ms will get different results.  Is it worth refactoring both of
> these so that, like change_entry_type_global, you hold the host p2m lock
> while you change the individual altp2m locks?

You're right. I do agree it is worth refactoring the code to hold the
hostp2m lock until the end. I'll do that.

On the first patch of the series: can it go in independently, since Jan
is OK with it and it just got your ack? Or should I just add the ack and
carry it over to the next version of the series?


Thanks again,
Razvan

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

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

[Jan Beulich]

>>> On 08.11.18 at 19:37, <rcojocaru@bitdefender.com> wrote:
> On the first patch of the series: can it go in independently, since Jan
> is OK with it and it just got your ack? Or should I just add the ack and
> carry it over to the next version of the series?

Well, I would have committed it, if only it had a VMX/EPT maintainer
ack.

Jan



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

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

[Razvan Cojocaru]

On 11/9/18 12:34 PM, Jan Beulich wrote:
>>>> On 08.11.18 at 19:37, <rcojocaru@bitdefender.com> wrote:
>> On the first patch of the series: can it go in independently, since Jan
>> is OK with it and it just got your ack? Or should I just add the ack and
>> carry it over to the next version of the series?
> 
> Well, I would have committed it, if only it had a VMX/EPT maintainer
> ack.

Fair enough - Jun, Kevin, would you mind taking a quick look at this
patch please?


Thanks,
Razvan

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

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

[George Dunlap]

On 11/1/18 2:45 PM, Razvan Cojocaru 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>

Algorithm looks good; one issue:

>  /* 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);
> +    }

This is wrong -- the rangeset is created even if !hap_enabled() or
!cpu_has_vmx, but only destroyed here if both.

Everything else looks OK.

 -George

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

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

[Razvan Cojocaru]

On 11/9/18 2:29 PM, George Dunlap wrote:
> On 11/1/18 2:45 PM, Razvan Cojocaru 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>
> 
> Algorithm looks good; one issue:
> 
>>  /* 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);
>> +    }
> 
> This is wrong -- the rangeset is created even if !hap_enabled() or
> !cpu_has_vmx, but only destroyed here if both.
> 
> Everything else looks OK.

Right, I'll get to fixing that. :)


Thanks,
Razvan

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

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

[Razvan Cojocaru]

[-- Attachment #1: Type: text/plain, Size: 6719 bytes --]

On 11/8/18 8:14 PM, George Dunlap wrote:
> On 11/01/2018 02:45 PM, Razvan Cojocaru wrote:
> ...here and...
> 
>> +
>>  int p2m_set_ioreq_server(struct domain *d,
>>                           unsigned int flags,
>>                           struct hvm_ioreq_server *s)
>> @@ -994,12 +1033,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);
>> @@ -1052,6 +1091,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
>> +}
>> +
> 
> ...here you grab & release each lock separately, inside the update
> function.  memory_type_changed is probably more or less idempotent, so
> won't matter if two different calls race; but it seems likely that if
> two p2m_change_type_range() calls happen concurrently, the various
> altp2ms will get different results.  Is it worth refactoring both of
> these so that, like change_entry_type_global, you hold the host p2m lock
> while you change the individual altp2m locks?

I have changed the code to do all the modifications under hostp2m lock,
and on changing the resolution on a host with three altp2ms I get a
"Watchdog timer detects that CPU3 is stuck!" hypervisor crash:

(XEN) stdvga.c:178:d1v0 leaving stdvga mode
(XEN) 1 p2m_lock(hostp2m)
(XEN) 1 change_type_range(hostp2m)
(XEN) 1 p2m_lock(altp2m)
(XEN) 1 change_type_range(altp2m)
(XEN) 1 p2m_unlock(altp2m)
(XEN) 1 p2m_lock(altp2m)
(XEN) 1 change_type_range(altp2m)
(XEN) 1 p2m_unlock(altp2m)
(XEN) 1 p2m_lock(altp2m)
(XEN) 1 change_type_range(altp2m)
(XEN) 1 p2m_unlock(altp2m)
(XEN) 1 p2m_unlock(hostp2m)
(XEN) Watchdog timer detects that CPU3 is stuck!
(XEN) ----[ Xen-4.12-unstable  x86_64  debug=y   Not tainted ]----
(XEN) CPU:    3
(XEN) RIP:    e008:[<ffff82d080239107>] vcpu_sleep_sync+0x40/0x71
(XEN) RFLAGS: 0000000000000202   CONTEXT: hypervisor (d0v0)
(XEN) rax: 0000000000000001   rbx: ffff8308dc1c3000   rcx: ffff8308dc1c3130
(XEN) rdx: 0000000000000000   rsi: 0000000000000292   rdi: ffff830c529f1010
(XEN) rbp: ffff830c52987c88   rsp: ffff830c52987c78   r8:  ffff830ae7a1dfd0
(XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 0000000000001c80
(XEN) r12: ffff82d0802390c7   r13: ffff8308d5222000   r14: ffff82c000225000
(XEN) r15: 00000000000f0000   cr0: 0000000080050033   cr4: 0000000000372660
(XEN) cr3: 0000000856c56000   cr2: 00007f8d36195000
(XEN) fsb: 00007f8d3fffe8c0   gsb: ffff880276c00000   gss: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d080239107> (vcpu_sleep_sync+0x40/0x71):
(XEN)  01 00 00 00 74 24 f3 90 <8b> 11 48 8b 43 10 8b 80 cc 01 00 00 09
d0 48 98
(XEN) Xen stack trace from rsp=ffff830c52987c78:
(XEN)    0000000000000240 ffff8308dc1c3000 ffff830c52987cb8 ffff82d0802070f1
(XEN)    ffff830c52987cc8 ffff8308d5222000 0000000000000240 0000000000000048
(XEN)    ffff830c52987cc8 ffff82d0802084bd ffff830c52987d38 ffff82d08036861e
(XEN)    8c00000000000001 ffff8308d5222648 ffff830c52987d28 00000000000f0000
(XEN)    00007f8d400a7010 0000000000000048 0000000000856c23 0000000000000000
(XEN)    ffff830c52987e00 ffffffffffffffea deadbeefdeadf00d ffff82d0802eebef
(XEN)    ffff830c52987de8 ffff82d0802ee217 01ff830c00000001 ffff82e011b4e6e0
(XEN)    ffff830c52987d98 0000000000000000 ffff830c52971000 ffff830c52959000
(XEN)    0000000000000001 ffff830c52971000 ffff830c52987d98 0000000000000007
(XEN)    0000000000000240 00000000000f0000 ffff830c52987fff ffff8308d5222000
(XEN)    ffff830c52987dc8 0000000000000002 0000000000000001 00007f8d400af010
(XEN)    deadbeefdeadf00d ffff82d0802eebef ffff830c52987e48 ffff82d0802eec73
(XEN)    ffff82d08037a3c4 0000000280370001 00007f8d400ae010 0000000000000020
(XEN)    00007f8d400a7010 0000000000000048 ffff82d08037a3c4 ffff830c52987ef8
(XEN)    ffff830c52959000 0000000000000029 ffff830c52987ee8 ffff82d080373722
(XEN)    03ff82d08037a3c4 0000000000000001 0000000000000002 00007f8d400af010
(XEN)    deadbeefdeadf00d deadbeefdeadf00d ffff82d08037a3c4 ffff82d08037a3b8
(XEN)    ffff82d08037a3c4 ffff82d08037a3b8 ffff82d08037a3c4 ffff82d08037a3b8
(XEN)    ffff82d08037a3c4 ffff830c52959000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 00007cf3ad6780e7 ffff82d08037a422
(XEN) Xen call trace:
(XEN)    [<ffff82d080239107>] vcpu_sleep_sync+0x40/0x71
(XEN)    [<ffff82d0802070f1>] domain.c#do_domain_pause+0x33/0x4f
(XEN)    [<ffff82d0802084bd>] domain_pause+0x25/0x27
(XEN)    [<ffff82d08036861e>] hap_track_dirty_vram+0x2b3/0x491
(XEN)    [<ffff82d0802ee217>] dm.c#dm_op+0x472/0xd46
(XEN)    [<ffff82d0802eec73>] do_dm_op+0x84/0xba
(XEN)    [<ffff82d080373722>] pv_hypercall+0x1af/0x4cd
(XEN)    [<ffff82d08037a422>] lstar_enter+0x112/0x120
(XEN)
(XEN) CPU1 @ e008:ffff82d0802a853f
(time.c#time_calibration_std_rendezvous+0x62/0x77)
(XEN) CPU2 @ e008:ffff82d0802a853f
(time.c#time_calibration_std_rendezvous+0x62/0x77)
(XEN) CPU0 @ e008:ffff82d0802a8514
(time.c#time_calibration_std_rendezvous+0x37/0x77)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 3:
(XEN) FATAL TRAP: vector = 2 (nmi)
(XEN) [error_code=0000]
(XEN) ****************************************

AFAICT, it just takes longer than the timeout to do the sync. The code
changes have been simple (patch attached), I don't think this is an
actual deadlock but of course I could be wrong (at least printing out
all of the lock / unlock calls in the new code looks like everything is
being properly unlocked at the end).

Any suggestion, as usual, appreciated. :)


Thanks,
Razvan

[-- Attachment #2: change_altp2ms_under_hostp2m_lock.patch --]
[-- Type: text/x-patch, Size: 8033 bytes --]

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-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 bc6e543..04c2c65 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -279,7 +279,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;
@@ -288,31 +287,85 @@ 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 )
+    printk("2 p2m_lock(hostp2m)\n");
+    p2m_lock(hostp2m);
+
+    printk("2 memory_type_changed(hostp2m)\n");
+    _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];
+
+                printk("2 p2m_lock(altp2m)\n");
+                p2m_lock(altp2m);
+                printk("2 memory_type_changed(altp2m)\n");
+                _memory_type_changed(altp2m);
+                printk("2 p2m_unlock(altp2m)\n");
+                p2m_unlock(altp2m);
+            }
     }
+
+    printk("2 p2m_unlock(hostp2m)\n");
+    p2m_unlock(hostp2m);
 }
+#endif
 
 int p2m_set_ioreq_server(struct domain *d,
                          unsigned int flags,
@@ -994,18 +1047,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) )
@@ -1049,7 +1098,47 @@ 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));
+
+    printk("1 p2m_lock(hostp2m)\n");
+    p2m_lock(hostp2m);
+
+    printk("1 change_type_range(hostp2m)\n");
+    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];
+
+                printk("1 p2m_lock(altp2m)\n");
+                p2m_lock(altp2m);
+
+                printk("1 change_type_range(altp2m)\n");
+                change_type_range(altp2m, start, end, ot, nt);
+
+                printk("1 p2m_unlock(altp2m)\n");
+                p2m_lock(altp2m);
+            }
+    }
+#endif
+
+    printk("1 p2m_unlock(hostp2m)\n");
+    p2m_unlock(hostp2m);
 }
 
 /*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index c7f5710..be5b7a2 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -630,9 +630,6 @@ int p2m_finish_type_change(struct domain *d,
                            gfn_t first_gfn,
                            unsigned long max_nr);
 
-/* Report a change affecting memory types. */
-void p2m_memory_type_changed(struct domain *d);
-
 int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
                           unsigned long end);
 
@@ -663,6 +660,9 @@ void p2m_pod_dump_data(struct domain *d);
 
 #ifdef CONFIG_HVM
 
+/* Report a change affecting memory types. */
+void p2m_memory_type_changed(struct domain *d);
+
 /* Called by p2m code when demand-populating a PoD page */
 bool
 p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn, unsigned int order);

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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

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

[Razvan Cojocaru]

On 11/9/18 4:19 PM, Razvan Cojocaru wrote:
> (XEN) Xen call trace:
> (XEN)    [<ffff82d080239107>] vcpu_sleep_sync+0x40/0x71
> (XEN)    [<ffff82d0802070f1>] domain.c#do_domain_pause+0x33/0x4f
> (XEN)    [<ffff82d0802084bd>] domain_pause+0x25/0x27
> (XEN)    [<ffff82d08036861e>] hap_track_dirty_vram+0x2b3/0x491
> (XEN)    [<ffff82d0802ee217>] dm.c#dm_op+0x472/0xd46
> (XEN)    [<ffff82d0802eec73>] do_dm_op+0x84/0xba
> (XEN)    [<ffff82d080373722>] pv_hypercall+0x1af/0x4cd
> (XEN)    [<ffff82d08037a422>] lstar_enter+0x112/0x120
> (XEN)
> (XEN) CPU1 @ e008:ffff82d0802a853f
> (time.c#time_calibration_std_rendezvous+0x62/0x77)
> (XEN) CPU2 @ e008:ffff82d0802a853f
> (time.c#time_calibration_std_rendezvous+0x62/0x77)
> (XEN) CPU0 @ e008:ffff82d0802a8514
> (time.c#time_calibration_std_rendezvous+0x37/0x77)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 3:
> (XEN) FATAL TRAP: vector = 2 (nmi)
> (XEN) [error_code=0000]
> (XEN) ****************************************

I also have a:

(XEN) Xen call trace:
(XEN)    [<ffff82d0802a7ab8>] time.c#timer_interrupt+0xda/0x11e
(XEN)    [<ffff82d080285755>] do_IRQ+0x5b5/0x65d
(XEN)    [<ffff82d08037a8aa>] common_interrupt+0x10a/0x120
(XEN)    [<ffff82d08022ad80>] queue_write_lock_slowpath+0x50/0x80
(XEN)    [<ffff82d08022adf7>] _percpu_write_lock+0x47/0x13a
(XEN)    [<ffff82d08033de26>] ept_handle_misconfig+0xcb/0x270
(XEN)    [<ffff82d0803226a7>] vmx_vmexit_handler+0x17e1/0x1b91
(XEN)    [<ffff82d0803277fa>] vmx_asm_vmexit_handler+0xfa/0x270
(XEN)
(XEN) CPU1 @ e008:ffff82d080239113 (vcpu_sleep_sync+0x4c/0x71)
(XEN) CPU3 @ e008:ffff82d0802dcb10 (mwait_idle_with_hints+0xed/0x133)
(XEN) CPU2 @ e008:ffff82d0802dcb10 (mwait_idle_with_hints+0xed/0x133)

(ept_handle_misconfig() is waiting to take a lock on the active p2m).


Thanks,
Razvan

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

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

[Razvan Cojocaru]

On 11/9/18 4:19 PM, Razvan Cojocaru wrote:
> On 11/8/18 8:14 PM, George Dunlap wrote:
>> On 11/01/2018 02:45 PM, Razvan Cojocaru wrote:
>> ...here and...
>>
>>> +
>>>  int p2m_set_ioreq_server(struct domain *d,
>>>                           unsigned int flags,
>>>                           struct hvm_ioreq_server *s)
>>> @@ -994,12 +1033,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);
>>> @@ -1052,6 +1091,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
>>> +}
>>> +
>>
>> ...here you grab & release each lock separately, inside the update
>> function.  memory_type_changed is probably more or less idempotent, so
>> won't matter if two different calls race; but it seems likely that if
>> two p2m_change_type_range() calls happen concurrently, the various
>> altp2ms will get different results.  Is it worth refactoring both of
>> these so that, like change_entry_type_global, you hold the host p2m lock
>> while you change the individual altp2m locks?
> 
> I have changed the code to do all the modifications under hostp2m lock,
> and on changing the resolution on a host with three altp2ms I get a
> "Watchdog timer detects that CPU3 is stuck!" hypervisor crash:

Apologies, this is clearly my fault. I've missed a copy-paste problem
that went wrong - I have a p2m_lock() where there should have been a
p2m_unlock().

I'm sorry for the noise.


Thanks,
Razvan

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