[PATCH V6 0/4] Fix VGA logdirty related display freezes with altp2m

[PATCH V6 0/4] Fix VGA logdirty related display freezes with altp2m

[Razvan Cojocaru]

This series aims to prevent the display from freezing when
enabling altp2m and switching to a new view (and assorted problems
when resizing the display). Since the last version of the series,
what was previously the second  patch has been split in
two patches.

The first patch propagates ept.ad changes to all active altp2ms,
the second one introduces p2m_{init,free}_logdirty(), the third
allocates  a new logdirty rangeset for each new altp2m, and the
fourth propagates (under lock) changes to all p2ms.

[PATCH V6 1/4] x86/altp2m: propagate ept.ad changes to all active altp2ms
[PATCH V6 2/4] x86/mm: introduce p2m_{init,free}_logdirty()
[PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms
[PATCH V6 4/4] x86/altp2m: fix display frozen when switching to a new view early

In the previous series the possibility of allocating altp2ms on
demand has been discussed. If that's a requirement for the series
we can certainly continue the conversation. Otherwise, since
the problem fixed here is completely blocking altp2m usage,
I hope we can fix this urgent matter, after which we'll happily
contribute code to switch to on-demand altp2m allocation.


Thanks,
Razvan

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

[PATCH V6 1/4] 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).

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>
Acked-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.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 V5:
 - Corrected patch description (this patch is part of the series,
   not sent separately).
 - Added Kevin's Reviewed-by.
---
 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 4bdc5e3..6a1abb6 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 V6 2/4] x86/mm: introduce p2m_{init, free}_logdirty()

[Razvan Cojocaru]

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

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

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

---
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 V5:
 - Added this new patch by splitting the former second patch for
   easier review.
---
 xen/arch/x86/mm/p2m.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

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


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

Re: [PATCH V6 2/4] x86/mm: introduce p2m_{init, free}_logdirty()

[George Dunlap]

> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
> 
> Add logdirty_ranges allocator / deallocator helpers.
> p2m_init_logdirty() will not re-allocate if
> p2m->logdirty ranges has already been allocated.
> 
> Move the rangeset deallocation call from p2m_teardown_hostp2m()
> to p2m_free_one() - we will want this to apply to altp2ms
> as well.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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


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

[PATCH V6 3/4] 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.

P2m_reset_altp2m() has been refactored to reduce code
repetition, and it now takes the p2m lock. Similar
refactoring has been done with p2m_activate_altp2m().

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 V5:
 - This is the second part of the former second patch split
   requested by George.
 - p2m_activate_altp2m() now puts the "if p2m_init_altp2m_logdirty()
   succeeds, then call p2m_init_altp2m_ept()" in one places.
 - p2m_flush_altp2m() now frees the logdirty ranges.
 - Put the p2m_flush_table() -> ept_p2m_uninit() ->
   ept_p2m_init() exclusively in p2m_reset_altp2m() and refactored
   it to also conditionally free the logdirty ranges and reset
   {min,max}_remapped_gfn.
---
 xen/arch/x86/mm/p2m.c     | 94 ++++++++++++++++++++++++++++++++---------------
 xen/include/asm-x86/p2m.h |  2 +-
 2 files changed, 65 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 418ff85..abdf443 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
     return 1;
 }
 
+static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
+                             bool reset_remapped, bool free_logdirty_ranges)
+{
+    struct p2m_domain *p2m;
+
+    ASSERT(idx < MAX_ALTP2M);
+    p2m = d->arch.altp2m_p2m[idx];
+
+    p2m_lock(p2m);
+
+    p2m_flush_table_locked(p2m);
+    /* Uninit and reinit ept to force TLB shootdown */
+
+    if ( free_logdirty_ranges )
+        p2m_free_logdirty(p2m);
+
+    ept_p2m_uninit(p2m);
+    ept_p2m_init(p2m);
+
+    if ( reset_remapped )
+    {
+        p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
+        p2m->max_remapped_gfn = 0;
+    }
+
+    p2m_unlock(p2m);
+}
+
 void p2m_flush_altp2m(struct domain *d)
 {
     unsigned int i;
@@ -2290,16 +2318,40 @@ void p2m_flush_altp2m(struct domain *d)
 
     for ( i = 0; i < MAX_ALTP2M; i++ )
     {
-        p2m_flush_table(d->arch.altp2m_p2m[i]);
-        /* Uninit and reinit ept to force TLB shootdown */
-        ept_p2m_uninit(d->arch.altp2m_p2m[i]);
-        ept_p2m_init(d->arch.altp2m_p2m[i]);
+        p2m_reset_altp2m(d, i, false, true);
         d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
     }
 
     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);
+}
+
+static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
+{
+    int rc;
+
+    ASSERT(idx < MAX_ALTP2M);
+    rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
+
+    if ( rc )
+        return rc;
+
+    p2m_init_altp2m_ept(d, idx);
+
+    return 0;
+}
+
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
     int rc = -EINVAL;
@@ -2310,10 +2362,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
-    {
-        p2m_init_altp2m_ept(d, idx);
-        rc = 0;
-    }
+        rc = p2m_activate_altp2m(d, idx);
 
     altp2m_list_unlock(d);
     return rc;
@@ -2331,9 +2380,10 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
         if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
             continue;
 
-        p2m_init_altp2m_ept(d, i);
-        *idx = i;
-        rc = 0;
+        rc = p2m_activate_altp2m(d, i);
+
+        if ( !rc )
+            *idx = i;
 
         break;
     }
@@ -2360,10 +2410,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
 
         if ( !_atomic_read(p2m->active_vcpus) )
         {
-            p2m_flush_table(d->arch.altp2m_p2m[idx]);
-            /* Uninit and reinit ept to force TLB shootdown */
-            ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
-            ept_p2m_init(d->arch.altp2m_p2m[idx]);
+            p2m_reset_altp2m(d, idx, false, true);
             d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
             rc = 0;
         }
@@ -2488,16 +2535,6 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
     return rc;
 }
 
-static void p2m_reset_altp2m(struct p2m_domain *p2m)
-{
-    p2m_flush_table(p2m);
-    /* Uninit and reinit ept to force TLB shootdown */
-    ept_p2m_uninit(p2m);
-    ept_p2m_init(p2m);
-    p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
-    p2m->max_remapped_gfn = 0;
-}
-
 int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                                 mfn_t mfn, unsigned int page_order,
                                 p2m_type_t p2mt, p2m_access_t p2ma)
@@ -2531,7 +2568,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
         {
             if ( !reset_count++ )
             {
-                p2m_reset_altp2m(p2m);
+                p2m_reset_altp2m(d, i, true, false);
                 last_reset_idx = i;
             }
             else
@@ -2545,10 +2582,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                          d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
                         continue;
 
-                    p2m = d->arch.altp2m_p2m[i];
-                    p2m_lock(p2m);
-                    p2m_reset_altp2m(p2m);
-                    p2m_unlock(p2m);
+                    p2m_reset_altp2m(d, i, true, false);
                 }
 
                 ret = 0;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index ac33f50..c7f5710 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -222,7 +222,7 @@ struct p2m_domain {
     struct rangeset   *logdirty_ranges;
 
     /* Host p2m: Global log-dirty mode enabled for the domain. */
-    bool_t             global_logdirty;
+    bool               global_logdirty;
 
     /* Host p2m: when this flag is set, don't flush all the nested-p2m 
      * tables on every host-p2m change.  The setter of this flag 
-- 
2.7.4


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

Re: [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms

[Jan Beulich]

>>> On 14.11.18 at 21:40, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
>      return 1;
>  }
>  
> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
> +                             bool reset_remapped, bool free_logdirty_ranges)

Especially the call sites are quite ugly to read with two booleans like
these. Did you consider introducing an unsigned int flags parameter
instead, with suitable symbols constants #define-d to make visible
at the call sites what behavior is wanted?

Jan



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

Re: [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 11/15/18 12:56 PM, Jan Beulich wrote:
>>>> On 14.11.18 at 21:40, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
>>      return 1;
>>  }
>>  
>> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
>> +                             bool reset_remapped, bool free_logdirty_ranges)
> 
> Especially the call sites are quite ugly to read with two booleans like
> these. Did you consider introducing an unsigned int flags parameter
> instead, with suitable symbols constants #define-d to make visible
> at the call sites what behavior is wanted?

That's a very good idea, I'll do that.


Thanks,
Razvan

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

Re: [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms

[George Dunlap]

> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
> 
> This patch is a pre-requisite for the one fixing VGA logdirty
> freezes when using altp2m. It only concerns itself with the
> ranges allocation / deallocation / initialization part. While
> touching the code, I've switched global_logdirty from bool_t
> to bool.
> 
> P2m_reset_altp2m() has been refactored to reduce code
> repetition, and it now takes the p2m lock. Similar
> refactoring has been done with p2m_activate_altp2m().

Thanks!  I think this is almost there.  I have a couple of comments about the code below; so since we have to do another round, let me comment on the changelog.

It doesn’t make much sense to say you’re “refactoring” a function that you are just now introducing in this patch.


I think I’d say something  more like this:

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

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

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

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

> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 418ff85..abdf443 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
>     return 1;
> }
> 
> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
> +                             bool reset_remapped, bool free_logdirty_ranges)

As Jan says, passing in (…true, false) makes it more difficult to follow the code and see what’s going on.  You could use a ‘flags’ structure, as he mentions; or alternately, you could pass in an argument that was either DEACTIVATE or RESET, and then use that to decide which things to do.

> +{
> +    struct p2m_domain *p2m;
> +
> +    ASSERT(idx < MAX_ALTP2M);
> +    p2m = d->arch.altp2m_p2m[idx];
> +
> +    p2m_lock(p2m);
> +
> +    p2m_flush_table_locked(p2m);
> +    /* Uninit and reinit ept to force TLB shootdown */
> +
> +    if ( free_logdirty_ranges )
> +        p2m_free_logdirty(p2m);
> +
> +    ept_p2m_uninit(p2m);
> +    ept_p2m_init(p2m);

Nit: The comment about uninit/reinit should be just before the uninit/reinit. :-)

> +
> +    if ( reset_remapped )
> +    {
> +        p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
> +        p2m->max_remapped_gfn = 0;
> +    }

I don’t think there’s a need to do this conditionally.  In fact, the only reason it’s correct *not* to do this for the other two cases is because in those cases the p2m will go through p2m_init_altp2m_ept() before being used again.  Resetting these is redundant, but harmless, and not worth an extra function argument and conditional to avoid.

> +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);
> +}
> +
> +static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
> +{
> +    int rc;
> +
> +    ASSERT(idx < MAX_ALTP2M);
> +    rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
> +
> +    if ( rc )
> +        return rc;
> +
> +    p2m_init_altp2m_ept(d, idx);

Is there any reason to make these separate functions?  I had in mind a single p2m_activate_altp2m() function which would do all three things (p2m_init_logdirty, rangeset_merge, and init_altp2m_ept).

Also, I realize it’s _probably_ not necessary to grab the lock here for the p2m in question (since it shouldn’t be in use, because altp2m_eptp[] is INVALID_MFN); but since it’s not on the hot path, it seems like we might as well grab it just to be safe.

Everything else looks good, thanks.

 -George

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

Re: [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru]

On 11/15/18 5:52 PM, George Dunlap wrote:
> 
> 
>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
>>
>> This patch is a pre-requisite for the one fixing VGA logdirty
>> freezes when using altp2m. It only concerns itself with the
>> ranges allocation / deallocation / initialization part. While
>> touching the code, I've switched global_logdirty from bool_t
>> to bool.
>>
>> P2m_reset_altp2m() has been refactored to reduce code
>> repetition, and it now takes the p2m lock. Similar
>> refactoring has been done with p2m_activate_altp2m().
> 
> Thanks!  I think this is almost there.  I have a couple of comments about the code below; so since we have to do another round, let me comment on the changelog.
> 
> It doesn’t make much sense to say you’re “refactoring” a function that you are just now introducing in this patch.

Right, the term doesn't apply to p2m_activate_altp2m() - I got carried
away with p2m_reset_altp2m(). :)

> I think I’d say something  more like this:
> 
> ---
> For now, only do allocation/deallocation; keeping them in sync will be done in subsequent patches.
> 
> Logdirty synchronization will only be done for active altp2ms; so allocate logdirty rangesets (copying the host logdirty rangeset) when an altp2m is activated, and free it when deactivated.
> 
> Write a helper function to do altp2m activiation (appropriately handling failures).  Also, refactor p2m_reset_altp2m() so that it can be used to remove redundant codepaths, fixing the locking while we’re at it.
> 
> While we’re here, switch global_logdirty from bool_t to bool.
> ---

Thanks, I'll use the new description.

>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 418ff85..abdf443 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
>>     return 1;
>> }
>>
>> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
>> +                             bool reset_remapped, bool free_logdirty_ranges)
> 
> As Jan says, passing in (…true, false) makes it more difficult to follow the code and see what’s going on.  You could use a ‘flags’ structure, as he mentions; or alternately, you could pass in an argument that was either DEACTIVATE or RESET, and then use that to decide which things to do.

Will do.

>> +{
>> +    struct p2m_domain *p2m;
>> +
>> +    ASSERT(idx < MAX_ALTP2M);
>> +    p2m = d->arch.altp2m_p2m[idx];
>> +
>> +    p2m_lock(p2m);
>> +
>> +    p2m_flush_table_locked(p2m);
>> +    /* Uninit and reinit ept to force TLB shootdown */
>> +
>> +    if ( free_logdirty_ranges )
>> +        p2m_free_logdirty(p2m);
>> +
>> +    ept_p2m_uninit(p2m);
>> +    ept_p2m_init(p2m);
> 
> Nit: The comment about uninit/reinit should be just before the uninit/reinit. :-)

Will move it.

>> +
>> +    if ( reset_remapped )
>> +    {
>> +        p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>> +        p2m->max_remapped_gfn = 0;
>> +    }
> 
> I don’t think there’s a need to do this conditionally.  In fact, the only reason it’s correct *not* to do this for the other two cases is because in those cases the p2m will go through p2m_init_altp2m_ept() before being used again.  Resetting these is redundant, but harmless, and not worth an extra function argument and conditional to avoid.

I'll remove the if.

>> +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);
>> +}
>> +
>> +static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
>> +{
>> +    int rc;
>> +
>> +    ASSERT(idx < MAX_ALTP2M);
>> +    rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
>> +
>> +    if ( rc )
>> +        return rc;
>> +
>> +    p2m_init_altp2m_ept(d, idx);
> 
> Is there any reason to make these separate functions?  I had in mind a single p2m_activate_altp2m() function which would do all three things (p2m_init_logdirty, rangeset_merge, and init_altp2m_ept).

Nope, no reason, in fact I did think about just that but wasn't sure it
wasn't deviating from what I thought was requested. I'll make that code
a single function.

> Also, I realize it’s _probably_ not necessary to grab the lock here for the p2m in question (since it shouldn’t be in use, because altp2m_eptp[] is INVALID_MFN); but since it’s not on the hot path, it seems like we might as well grab it just to be safe.
> 
> Everything else looks good, thanks.

Thanks for the review!


Razvan

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

[PATCH V6 4/4] 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.

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

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Suggested-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.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 V5:
 - Added Kevin's Reviewed-by.
 - Added a note on p2m_memory_type_changed() being under CONFIG_HVM.
---
 xen/arch/x86/mm/p2m-ept.c |   8 ++++
 xen/arch/x86/mm/p2m-pt.c  |   8 ++++
 xen/arch/x86/mm/p2m.c     | 115 ++++++++++++++++++++++++++++++++++++++--------
 xen/include/asm-x86/p2m.h |   6 +--
 4 files changed, 114 insertions(+), 23 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-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 abdf443..c401806 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -277,7 +277,6 @@ int p2m_init(struct domain *d)
 int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
                           unsigned long end)
 {
-    ASSERT(p2m_is_hostp2m(p2m));
     if ( p2m->global_logdirty ||
          rangeset_contains_range(p2m->logdirty_ranges, start, end) )
         return 1;
@@ -286,31 +285,79 @@ int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
     return 0;
 }
 
+static void change_entry_type_global(struct p2m_domain *p2m,
+                                     p2m_type_t ot, p2m_type_t nt)
+{
+    p2m->change_entry_type_global(p2m, ot, nt);
+    p2m->global_logdirty = (nt == p2m_ram_logdirty);
+}
+
 void p2m_change_entry_type_global(struct domain *d,
                                   p2m_type_t ot, p2m_type_t nt)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
 
     ASSERT(ot != nt);
     ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
 
-    p2m_lock(p2m);
-    p2m->change_entry_type_global(p2m, ot, nt);
-    p2m->global_logdirty = (nt == p2m_ram_logdirty);
-    p2m_unlock(p2m);
+    p2m_lock(hostp2m);
+
+    change_entry_type_global(hostp2m, ot, nt);
+
+#ifdef CONFIG_HVM
+    if ( unlikely(altp2m_active(d)) )
+    {
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            {
+                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(altp2m);
+                change_entry_type_global(altp2m, ot, nt);
+                p2m_unlock(altp2m);
+            }
+    }
+#endif
+
+    p2m_unlock(hostp2m);
+}
+
+#ifdef CONFIG_HVM
+/* There's already a memory_type_changed() in asm/mtrr.h. */
+static void _memory_type_changed(struct p2m_domain *p2m)
+{
+    if ( p2m->memory_type_changed )
+        p2m->memory_type_changed(p2m);
 }
 
 void p2m_memory_type_changed(struct domain *d)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
 
-    if ( p2m->memory_type_changed )
+    p2m_lock(hostp2m);
+
+    _memory_type_changed(hostp2m);
+
+    if ( unlikely(altp2m_active(d)) )
     {
-        p2m_lock(p2m);
-        p2m->memory_type_changed(p2m);
-        p2m_unlock(p2m);
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            {
+                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(altp2m);
+                _memory_type_changed(altp2m);
+                p2m_unlock(altp2m);
+            }
     }
+
+    p2m_unlock(hostp2m);
 }
+#endif
 
 int p2m_set_ioreq_server(struct domain *d,
                          unsigned int flags,
@@ -991,18 +1038,14 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
 }
 
 /* Modify the p2m type of a range of gfns from ot to nt. */
-void p2m_change_type_range(struct domain *d, 
-                           unsigned long start, unsigned long end,
-                           p2m_type_t ot, p2m_type_t nt)
+static void change_type_range(struct p2m_domain *p2m,
+                              unsigned long start, unsigned long end,
+                              p2m_type_t ot, p2m_type_t nt)
 {
     unsigned long gfn = start;
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct domain *d = p2m->domain;
     int rc = 0;
 
-    ASSERT(ot != nt);
-    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
-
-    p2m_lock(p2m);
     p2m->defer_nested_flush = 1;
 
     if ( unlikely(end > p2m->max_mapped_pfn) )
@@ -1046,7 +1089,39 @@ void p2m_change_type_range(struct domain *d,
     p2m->defer_nested_flush = 0;
     if ( nestedhvm_enabled(d) )
         p2m_flush_nestedp2m(d);
-    p2m_unlock(p2m);
+}
+
+void p2m_change_type_range(struct domain *d,
+                           unsigned long start, unsigned long end,
+                           p2m_type_t ot, p2m_type_t nt)
+{
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
+
+    ASSERT(ot != nt);
+    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
+
+    p2m_lock(hostp2m);
+
+    change_type_range(hostp2m, start, end, ot, nt);
+
+#ifdef CONFIG_HVM
+    if ( unlikely(altp2m_active(d)) )
+    {
+        unsigned int i;
+
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            {
+                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
+
+                p2m_lock(altp2m);
+                change_type_range(altp2m, start, end, ot, nt);
+                p2m_unlock(altp2m);
+            }
+    }
+#endif
+
+    p2m_unlock(hostp2m);
 }
 
 /*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index c7f5710..be5b7a2 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -630,9 +630,6 @@ int p2m_finish_type_change(struct domain *d,
                            gfn_t first_gfn,
                            unsigned long max_nr);
 
-/* Report a change affecting memory types. */
-void p2m_memory_type_changed(struct domain *d);
-
 int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
                           unsigned long end);
 
@@ -663,6 +660,9 @@ void p2m_pod_dump_data(struct domain *d);
 
 #ifdef CONFIG_HVM
 
+/* Report a change affecting memory types. */
+void p2m_memory_type_changed(struct domain *d);
+
 /* Called by p2m code when demand-populating a PoD page */
 bool
 p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn, unsigned int order);
-- 
2.7.4


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

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

[George Dunlap]

> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
> 
> When an new altp2m view is created very early on guest boot, the
> display will freeze (although the guest will run normally). This
> may also happen on resizing the display. The reason is the way
> Xen currently (mis)handles logdirty VGA: it intentionally
> misconfigures VGA pages so that they will fault.
> 
> The problem is that it only does this in the host p2m. Once we
> switch to a new altp2m, the misconfigured entries will no longer
> fault, so the display will not be updated.
> 
> This patch:
> * updates ept_handle_misconfig() to use the active altp2m instead
>  of the hostp2m;
> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>  and p2m_change_type_range() to propagate their changes to all
>  valid altp2ms.
> 
> With the introduction of altp2m fields in p2m_memory_type_changed()
> the whole function has been put under CONFIG_HVM.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Just one more question...

> 
> ---
> 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 V5:
> - Added Kevin's Reviewed-by.
> - Added a note on p2m_memory_type_changed() being under CONFIG_HVM.
> ---
> xen/arch/x86/mm/p2m-ept.c |   8 ++++
> xen/arch/x86/mm/p2m-pt.c  |   8 ++++
> xen/arch/x86/mm/p2m.c     | 115 ++++++++++++++++++++++++++++++++++++++--------
> xen/include/asm-x86/p2m.h |   6 +--
> 4 files changed, 114 insertions(+), 23 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;

Why we need to copy this value?

I’ve just spent the afternoon tracing around the source code, and while I’m pretty sure it won’t hurt, I’m also not sure why it’s necessary.

Everything else looks good!

 -George

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

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

[Razvan Cojocaru]

On 11/15/18 7:34 PM, George Dunlap wrote:
> 
> 
>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
>>
>> When an new altp2m view is created very early on guest boot, the
>> display will freeze (although the guest will run normally). This
>> may also happen on resizing the display. The reason is the way
>> Xen currently (mis)handles logdirty VGA: it intentionally
>> misconfigures VGA pages so that they will fault.
>>
>> The problem is that it only does this in the host p2m. Once we
>> switch to a new altp2m, the misconfigured entries will no longer
>> fault, so the display will not be updated.
>>
>> This patch:
>> * updates ept_handle_misconfig() to use the active altp2m instead
>>  of the hostp2m;
>> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>>  and p2m_change_type_range() to propagate their changes to all
>>  valid altp2ms.
>>
>> With the introduction of altp2m fields in p2m_memory_type_changed()
>> the whole function has been put under CONFIG_HVM.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> Just one more question...
> 
>>
>> ---
>> 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 V5:
>> - Added Kevin's Reviewed-by.
>> - Added a note on p2m_memory_type_changed() being under CONFIG_HVM.
>> ---
>> xen/arch/x86/mm/p2m-ept.c |   8 ++++
>> xen/arch/x86/mm/p2m-pt.c  |   8 ++++
>> xen/arch/x86/mm/p2m.c     | 115 ++++++++++++++++++++++++++++++++++++++--------
>> xen/include/asm-x86/p2m.h |   6 +--
>> 4 files changed, 114 insertions(+), 23 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;
> 
> Why we need to copy this value?
> 
> I’ve just spent the afternoon tracing around the source code, and while I’m pretty sure it won’t hurt, I’m also not sure why it’s necessary.

I think I did that because I assumed that it would matter for
ept_get_entry() and misconfigurations, when:

 954     /* This pfn is higher than the highest the p2m map currently
holds */
 955     if ( gfn > p2m->max_mapped_pfn )
 956     {
 957         for ( i = ept->wl; i > 0; --i )
 958             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
 959                  p2m->max_mapped_pfn )
 960                 break;
 961         goto out;
 962     }

but I am not certain it is required either. I can certainly remove this
assignment and see if anything breaks.

> Everything else looks good!


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 15.11.18 at 18:54, <rcojocaru@bitdefender.com> wrote:
> On 11/15/18 7:34 PM, George Dunlap wrote:
>>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
>>> @@ -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;
>> 
>> Why we need to copy this value?
>> 
>> I’ve just spent the afternoon tracing around the source code, and while I’m 
> pretty sure it won’t hurt, I’m also not sure why it’s necessary.
> 
> I think I did that because I assumed that it would matter for
> ept_get_entry() and misconfigurations, when:
> 
>  954     /* This pfn is higher than the highest the p2m map currently
> holds */
>  955     if ( gfn > p2m->max_mapped_pfn )

The question is whether under any condition such checks require that
the accumulated value be in sync between the host and the various
alt p2m-s.

>  956     {
>  957         for ( i = ept->wl; i > 0; --i )
>  958             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
>  959                  p2m->max_mapped_pfn )
>  960                 break;
>  961         goto out;
>  962     }
> 
> but I am not certain it is required either. I can certainly remove this
> assignment and see if anything breaks.

I've seen you mention such or alike a couple of times now while
discussing this series, and I have to admit that I'm a little frightened:
Making a change just based on the observation that nothing breaks
is setting us up for breakage in some corner case. That is - seeing
something break is a good indication that a change was wrong, but
not seeing any breakage doesn't alone justify a change.

In the particular case here I think whether the copying of the field
is needed depends on what else is being copied (I'm sorry, I'm not
that familiar with altp2m): If mappings get cloned from the host p2m,
then this value ought to be cloned too. For any mappings that
might be kept in sync between p2m-s, I think this field also would
need to be updated in sync.

Jan


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

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

[Razvan Cojocaru]

On 11/16/18 12:08 PM, Jan Beulich wrote:
> I've seen you mention such or alike a couple of times now while
> discussing this series, and I have to admit that I'm a little frightened:
> Making a change just based on the observation that nothing breaks
> is setting us up for breakage in some corner case. That is - seeing
> something break is a good indication that a change was wrong, but
> not seeing any breakage doesn't alone justify a change.
> 
> In the particular case here I think whether the copying of the field
> is needed depends on what else is being copied (I'm sorry, I'm not
> that familiar with altp2m): If mappings get cloned from the host p2m,
> then this value ought to be cloned too. For any mappings that
> might be kept in sync between p2m-s, I think this field also would
> need to be updated in sync.

I agree, I was merely offering to test whether any breakage occurs if
not syncing the field, not suggesting that that's a comprehensive
analysis or a justification for change, in case that offers us more insight.

AFAIK, EPT restrictions are always propagated from the hostp2m to all
active altp2ms. I've tested this at some point in the past and it has
proven true - it's supposedly an altp2m feature although it can also be
a bit of a headache: if we would have been able to use the hostp2m
independently then we wouldn't have had this display freeze problem,
since we could have used the hostp2m as the default (restricted) EPT
view. As things stand now, we need to create a new view when starting
introspection and switch to that immediately, so changes to it will not
propagate into other views (in other words, so we can use it
independently of all other altp2ms).


Thanks,
Razvan

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

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

[George Dunlap]

On 11/16/18 10:08 AM, Jan Beulich wrote:
>>>> On 15.11.18 at 18:54, <rcojocaru@bitdefender.com> wrote:
>> On 11/15/18 7:34 PM, George Dunlap wrote:
>>>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
>>>> @@ -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;
>>>
>>> Why we need to copy this value?
>>>
>>> I’ve just spent the afternoon tracing around the source code, and while I’m 
>> pretty sure it won’t hurt, I’m also not sure why it’s necessary.
>>
>> I think I did that because I assumed that it would matter for
>> ept_get_entry() and misconfigurations, when:
>>
>>  954     /* This pfn is higher than the highest the p2m map currently
>> holds */
>>  955     if ( gfn > p2m->max_mapped_pfn )
> 
> The question is whether under any condition such checks require that
> the accumulated value be in sync between the host and the various
> alt p2m-s.
> 
>>  956     {
>>  957         for ( i = ept->wl; i > 0; --i )
>>  958             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
>>  959                  p2m->max_mapped_pfn )
>>  960                 break;
>>  961         goto out;
>>  962     }
>>
>> but I am not certain it is required either. I can certainly remove this
>> assignment and see if anything breaks.
> 
> I've seen you mention such or alike a couple of times now while
> discussing this series, and I have to admit that I'm a little frightened:
> Making a change just based on the observation that nothing breaks
> is setting us up for breakage in some corner case. That is - seeing
> something break is a good indication that a change was wrong, but
> not seeing any breakage doesn't alone justify a change.
> 
> In the particular case here I think whether the copying of the field
> is needed depends on what else is being copied (I'm sorry, I'm not
> that familiar with altp2m): If mappings get cloned from the host p2m,
> then this value ought to be cloned too. For any mappings that
> might be kept in sync between p2m-s, I think this field also would
> need to be updated in sync.

So the basic question is, does "max_mapped_pfn" mean, "Maximum pfn _for
the domain_", or "Maximum pfn _for this p2m_".  When the element was
added to the p2m struct those were the same thing.  Which do the various
use cases expect it to be, and which would be the most robust going forward?

I spent a bunch of time going through the code yesterday, and I'm pretty
sure that as long as the value in the p2m is one or the other, there
will be at the moment no _correctness_ issues.  It looked to me like in
the case where altp2m->max_mapped_pfn > altp2m->max_remapped_gfn, then
the p2m machinery would do a certain amount of unnecessary work, but
that's all.

It also looked to me like before this patch, the value mostly ends up
being  "maximum pfn ever mapped in this p2m (even across altp2m
desroys)".  That's because when the altp2m is allocated, it starts as 0;
every time an entry is propagated from the hostp2m to the altp2m,
ept_set_entry() updates max_mapped_pfn; but nothing sets it back to zero.

Also, hostp2m->max_mapped_pfn is never decreased, only increased.

So either before or after this patch, altp2m->max_remapped_gfn <=
altp2m->max_mapped_pfn <= hostp2m->max_mapped_pfn.

In the vast majority of cases, max_mapped_pfn is explicitly being read
from the hostp2m.

Below are the cases where it may be read from an altp2m:

 - ept_get_entry(), ept_walk_tables(): If ==max_remapped_gfn, it will
return INVALID_MFN early.  If higher than max_remapped_gfn, it falls
back to walking the altp2m's ept tables, which will give you the same
answer, just a bit more slowly.

 - ept_dump_p2m_tables(): If ==max_remapped_gfn, it will dump only up to
the current map; if >max_remapped_gfn, it will dump a number of
unnecessary INVALID_MFN entries, but no more entries than the hostp2m.

 - p2m.c:change_type_range(): If ==max_remapped_gfn, it will only
invalidate entries in the altp2m as high have been currently remapped.
If >max_remapped_gfn, it will write invalid ept entries that *haven't*
yet been copied over.  But I don't think either one should cause a
correctness issue: either way, accessing a gfn > max_remapped_gfn will
cause a fault, at which point either the correct value will be copied
from the hostp2m (perhaps going through resolve_misconfig() on the
hostp2m in the process) or the correct value will be calculated via
resolve_misconfig().

So, it seemed from my investigation like it would be more useful if we
got rid of altp2m->max_remapped_gfn, and used atlp2m->max_mapped_pfn
instead.  If people want to know the maximum gfn mapped by the domain as
a whole, they should just use hostp2m->max_mapped_pfn.

The code is definitely complicated enough, though, that I may have
missed something, which is why I asked Razvan if there was a reason he
changed it.

For the purposes of this patch, I propose having p2m_altp2m_init_ept()
set max_mapped_pfn to 0 (if that works), and leaving "get rid of
max_remapped_pfn" for a future clean-up series.

That said -- Razvan, I realize the code is quite complex, but it's still
important that when you modify things you work to have a clear
understanding of why you're making changes.  The analysis I did above
with how max_mapped_pfn is used and what the effects would be one way or
the other should have been something you did before modifying it.
Otherwise things will only get more incomprehensible and buggy.

 -George

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

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

[Razvan Cojocaru]

On 11/16/18 2:03 PM, George Dunlap wrote:
> On 11/16/18 10:08 AM, Jan Beulich wrote:
>>>>> On 15.11.18 at 18:54, <rcojocaru@bitdefender.com> wrote:
>>> On 11/15/18 7:34 PM, George Dunlap wrote:
>>>>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
>>>>> @@ -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;
>>>>
>>>> Why we need to copy this value?
>>>>
>>>> I’ve just spent the afternoon tracing around the source code, and while I’m 
>>> pretty sure it won’t hurt, I’m also not sure why it’s necessary.
>>>
>>> I think I did that because I assumed that it would matter for
>>> ept_get_entry() and misconfigurations, when:
>>>
>>>  954     /* This pfn is higher than the highest the p2m map currently
>>> holds */
>>>  955     if ( gfn > p2m->max_mapped_pfn )
>>
>> The question is whether under any condition such checks require that
>> the accumulated value be in sync between the host and the various
>> alt p2m-s.
>>
>>>  956     {
>>>  957         for ( i = ept->wl; i > 0; --i )
>>>  958             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
>>>  959                  p2m->max_mapped_pfn )
>>>  960                 break;
>>>  961         goto out;
>>>  962     }
>>>
>>> but I am not certain it is required either. I can certainly remove this
>>> assignment and see if anything breaks.
>>
>> I've seen you mention such or alike a couple of times now while
>> discussing this series, and I have to admit that I'm a little frightened:
>> Making a change just based on the observation that nothing breaks
>> is setting us up for breakage in some corner case. That is - seeing
>> something break is a good indication that a change was wrong, but
>> not seeing any breakage doesn't alone justify a change.
>>
>> In the particular case here I think whether the copying of the field
>> is needed depends on what else is being copied (I'm sorry, I'm not
>> that familiar with altp2m): If mappings get cloned from the host p2m,
>> then this value ought to be cloned too. For any mappings that
>> might be kept in sync between p2m-s, I think this field also would
>> need to be updated in sync.
> 
> So the basic question is, does "max_mapped_pfn" mean, "Maximum pfn _for
> the domain_", or "Maximum pfn _for this p2m_".  When the element was
> added to the p2m struct those were the same thing.  Which do the various
> use cases expect it to be, and which would be the most robust going forward?
> 
> I spent a bunch of time going through the code yesterday, and I'm pretty
> sure that as long as the value in the p2m is one or the other, there
> will be at the moment no _correctness_ issues.  It looked to me like in
> the case where altp2m->max_mapped_pfn > altp2m->max_remapped_gfn, then
> the p2m machinery would do a certain amount of unnecessary work, but
> that's all.
> 
> It also looked to me like before this patch, the value mostly ends up
> being  "maximum pfn ever mapped in this p2m (even across altp2m
> desroys)".  That's because when the altp2m is allocated, it starts as 0;
> every time an entry is propagated from the hostp2m to the altp2m,
> ept_set_entry() updates max_mapped_pfn; but nothing sets it back to zero.
> 
> Also, hostp2m->max_mapped_pfn is never decreased, only increased.
> 
> So either before or after this patch, altp2m->max_remapped_gfn <=
> altp2m->max_mapped_pfn <= hostp2m->max_mapped_pfn.
> 
> In the vast majority of cases, max_mapped_pfn is explicitly being read
> from the hostp2m.
> 
> Below are the cases where it may be read from an altp2m:
> 
>  - ept_get_entry(), ept_walk_tables(): If ==max_remapped_gfn, it will
> return INVALID_MFN early.  If higher than max_remapped_gfn, it falls
> back to walking the altp2m's ept tables, which will give you the same
> answer, just a bit more slowly.
> 
>  - ept_dump_p2m_tables(): If ==max_remapped_gfn, it will dump only up to
> the current map; if >max_remapped_gfn, it will dump a number of
> unnecessary INVALID_MFN entries, but no more entries than the hostp2m.
> 
>  - p2m.c:change_type_range(): If ==max_remapped_gfn, it will only
> invalidate entries in the altp2m as high have been currently remapped.
> If >max_remapped_gfn, it will write invalid ept entries that *haven't*
> yet been copied over.  But I don't think either one should cause a
> correctness issue: either way, accessing a gfn > max_remapped_gfn will
> cause a fault, at which point either the correct value will be copied
> from the hostp2m (perhaps going through resolve_misconfig() on the
> hostp2m in the process) or the correct value will be calculated via
> resolve_misconfig().
> 
> So, it seemed from my investigation like it would be more useful if we
> got rid of altp2m->max_remapped_gfn, and used atlp2m->max_mapped_pfn
> instead.  If people want to know the maximum gfn mapped by the domain as
> a whole, they should just use hostp2m->max_mapped_pfn.
> 
> The code is definitely complicated enough, though, that I may have
> missed something, which is why I asked Razvan if there was a reason he
> changed it.
> 
> For the purposes of this patch, I propose having p2m_altp2m_init_ept()
> set max_mapped_pfn to 0 (if that works), and leaving "get rid of
> max_remapped_pfn" for a future clean-up series.

Thank you for the detailed analysis and the suggestions.

> That said -- Razvan, I realize the code is quite complex, but it's still
> important that when you modify things you work to have a clear
> understanding of why you're making changes.  The analysis I did above
> with how max_mapped_pfn is used and what the effects would be one way or
> the other should have been something you did before modifying it.
> Otherwise things will only get more incomprehensible and buggy.

Point taken, and I agree.

I have in fact done an analysis and reached some of the same
conclusions, but the reasons I have deferred to your call are:

1. the code being as complex as it is I thought the patch needed the
approval of someone with a much better overview of p2m,

2. most of the detailed analysis I've done happened months ago when the
series first got started, which means I'd have to redo at least part of
it to reach certainty - so couldn't reply on the spot more than I did,

and 3. I had misinterpreted the question to mean that the code is
probably incorrect (which is apparent from my initial reply), which of
course makes even the initial analysis seem wrong and ends up being a
hunt for judgment errors rather than just an analysis of the logic.

Thanks again for your help, and sorry for any accidental inconvenience.


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 16.11.18 at 13:03, <george.dunlap@citrix.com> wrote:
> So the basic question is, does "max_mapped_pfn" mean, "Maximum pfn _for
> the domain_", or "Maximum pfn _for this p2m_".  When the element was
> added to the p2m struct those were the same thing.  Which do the various
> use cases expect it to be, and which would be the most robust going forward?

So with the field getting updated right in ept_set_entry(), as long as
no copying of entries exists which does not go through that function,
I then agree that it shouldn't really matter whether the field gets
copied when setting up a new altp2m.

However, fair parts of your further response are confusing to me,
rather than clarifying. That's for one because you talk about the
max_remapped_gfn field, but you never mention its
min_remapped_gfn sibling. The only place I could find where
current code consumes these two is p2m_altp2m_propagate_change().
This suggests to me that both fields really only exist for optimization
purposes.

Furthermore I in particular ...

> I spent a bunch of time going through the code yesterday, and I'm pretty
> sure that as long as the value in the p2m is one or the other, there
> will be at the moment no _correctness_ issues.  It looked to me like in
> the case where altp2m->max_mapped_pfn > altp2m->max_remapped_gfn, then
> the p2m machinery would do a certain amount of unnecessary work, but
> that's all.
> 
> It also looked to me like before this patch, the value mostly ends up
> being  "maximum pfn ever mapped in this p2m (even across altp2m
> desroys)".  That's because when the altp2m is allocated, it starts as 0;
> every time an entry is propagated from the hostp2m to the altp2m,
> ept_set_entry() updates max_mapped_pfn; but nothing sets it back to zero.
> 
> Also, hostp2m->max_mapped_pfn is never decreased, only increased.
> 
> So either before or after this patch, altp2m->max_remapped_gfn <=
> altp2m->max_mapped_pfn <= hostp2m->max_mapped_pfn.
> 
> In the vast majority of cases, max_mapped_pfn is explicitly being read
> from the hostp2m.
> 
> Below are the cases where it may be read from an altp2m:
> 
>  - ept_get_entry(), ept_walk_tables(): If ==max_remapped_gfn, it will
> return INVALID_MFN early.  If higher than max_remapped_gfn, it falls
> back to walking the altp2m's ept tables, which will give you the same
> answer, just a bit more slowly.
> 
>  - ept_dump_p2m_tables(): If ==max_remapped_gfn, it will dump only up to
> the current map; if >max_remapped_gfn, it will dump a number of
> unnecessary INVALID_MFN entries, but no more entries than the hostp2m.
> 
>  - p2m.c:change_type_range(): If ==max_remapped_gfn, it will only
> invalidate entries in the altp2m as high have been currently remapped.
> If >max_remapped_gfn, it will write invalid ept entries that *haven't*
> yet been copied over.  But I don't think either one should cause a
> correctness issue: either way, accessing a gfn > max_remapped_gfn will
> cause a fault, at which point either the correct value will be copied
> from the hostp2m (perhaps going through resolve_misconfig() on the
> hostp2m in the process) or the correct value will be calculated via
> resolve_misconfig().

... cannot identify any of the three cases above where I understand
you say a max_mapped_pfn == max_remapped_gfn comparison
happens. But as you say - the code is complicated  enough, so I may
easily overlook something.

Jan



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

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

[George Dunlap]

On 11/16/18 12:31 PM, Jan Beulich wrote:
>>>> On 16.11.18 at 13:03, <george.dunlap@citrix.com> wrote:
>> So the basic question is, does "max_mapped_pfn" mean, "Maximum pfn _for
>> the domain_", or "Maximum pfn _for this p2m_".  When the element was
>> added to the p2m struct those were the same thing.  Which do the various
>> use cases expect it to be, and which would be the most robust going forward?
> 
> So with the field getting updated right in ept_set_entry(), as long as
> no copying of entries exists which does not go through that function,
> I then agree that it shouldn't really matter whether the field gets
> copied when setting up a new altp2m.
> 
> However, fair parts of your further response are confusing to me,
> rather than clarifying. That's for one because you talk about the
> max_remapped_gfn field, but you never mention its
> min_remapped_gfn sibling. The only place I could find where
> current code consumes these two is p2m_altp2m_propagate_change().
> This suggests to me that both fields really only exist for optimization
> purposes.
> 
> Furthermore I in particular ...
> 
>> I spent a bunch of time going through the code yesterday, and I'm pretty
>> sure that as long as the value in the p2m is one or the other, there
>> will be at the moment no _correctness_ issues.  It looked to me like in
>> the case where altp2m->max_mapped_pfn > altp2m->max_remapped_gfn, then
>> the p2m machinery would do a certain amount of unnecessary work, but
>> that's all.
>>
>> It also looked to me like before this patch, the value mostly ends up
>> being  "maximum pfn ever mapped in this p2m (even across altp2m
>> desroys)".  That's because when the altp2m is allocated, it starts as 0;
>> every time an entry is propagated from the hostp2m to the altp2m,
>> ept_set_entry() updates max_mapped_pfn; but nothing sets it back to zero.
>>
>> Also, hostp2m->max_mapped_pfn is never decreased, only increased.
>>
>> So either before or after this patch, altp2m->max_remapped_gfn <=
>> altp2m->max_mapped_pfn <= hostp2m->max_mapped_pfn.
>>
>> In the vast majority of cases, max_mapped_pfn is explicitly being read
>> from the hostp2m.
>>
>> Below are the cases where it may be read from an altp2m:
>>
>>  - ept_get_entry(), ept_walk_tables(): If ==max_remapped_gfn, it will
>> return INVALID_MFN early.  If higher than max_remapped_gfn, it falls
>> back to walking the altp2m's ept tables, which will give you the same
>> answer, just a bit more slowly.
>>
>>  - ept_dump_p2m_tables(): If ==max_remapped_gfn, it will dump only up to
>> the current map; if >max_remapped_gfn, it will dump a number of
>> unnecessary INVALID_MFN entries, but no more entries than the hostp2m.
>>
>>  - p2m.c:change_type_range(): If ==max_remapped_gfn, it will only
>> invalidate entries in the altp2m as high have been currently remapped.
>> If >max_remapped_gfn, it will write invalid ept entries that *haven't*
>> yet been copied over.  But I don't think either one should cause a
>> correctness issue: either way, accessing a gfn > max_remapped_gfn will
>> cause a fault, at which point either the correct value will be copied
>> from the hostp2m (perhaps going through resolve_misconfig() on the
>> hostp2m in the process) or the correct value will be calculated via
>> resolve_misconfig().
> 
> ... cannot identify any of the three cases above where I understand
> you say a max_mapped_pfn == max_remapped_gfn comparison
> happens. But as you say - the code is complicated  enough, so I may
> easily overlook something.

Sorry, it seems I took too many shortcuts explaining things. :-)  I was
using max_remapped_gfn as a shorthand for, "the highest gfn mapped in
the altp2m" (since that's what it will be equal to).  Not that an actual
comparison will happen there, but we're considering what will happen,
based on various values of altp2m->max_mapped_pfn, when a gfn that is
higher than the highest *remapped* gfn is encountered.

So in the situation we're considering, the following are always true:
- gfn > altp2m->max_remapped_gfn
- altp2m->max_remapped_gfn <= altp2m->max_mapped_pfn <=
hostp2m->max_mapped_pfn

And we're comparing the results in the following cases:

A: altp2m->max_mapped_pfn == alt2m->max_remapped_gfn
B: altp2m->max_mapped_pfn > altp2m->max_remapped_gfn
  (Perhaps == hostp2m->max_mapped_pfn, perhaps something in between).

Take ept_get_entry().  At this level, it's below all the mem_access /
mem_sharing / mem_paging / altp2m abstractions.  Apart from
resolve_misconfig and pod, it should return the actual contents of the
particular p2m it's given.  In the case of an altp2m, any entries above
what's currently been remapped should return empty.

In case A, it will do this, because the first conditional will find that
gfn > altp2m->max_mapped_pfn.

In case B, it will also do this, because although it passes the first
conditional, when it actually reads the table it will find an empty
entry and return that.

Both results are correct, but A is a tiny bit faster.

Now take change_type_range.  The global effect of change_type_range
should be that reads of the p2m which happen afterwards should have the
new, changed value.

In case A, change_type_range will write invalid entries up to
max_remapped_pfn, leaving the range between max_remapped_pfn and
hostp2m->max_mapped_pfn invalid.  When a gfn in this range is read, an
EPT fault will happen, p2m_altp2m_lazy_copy() will be called, and the
new (correct) value copied from the hostp2m.

In case B, change_type_range will write invalid entries up until
hostp2m->max_mapped_pfn.  When a gfn in this range is accessed, a
MISCONFIG fault will happen, and the correct value will be calculated in
resolve_misconfig.

And at this point, I realize that my previous analysis was probably
wrong, because at this point altp2m->max_remapped_gfn will be wrong:
entries above max_remapped_gfn will have become valid without going
through p2m_altp2m_lazy_copy().

 -George

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

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

[George Dunlap]

On 11/16/18 3:05 PM, George Dunlap wrote:
> Now take change_type_range.  The global effect of change_type_range
> should be that reads of the p2m which happen afterwards should have the
> new, changed value.
> 
> In case A, change_type_range will write invalid entries up to
> max_remapped_pfn, leaving the range between max_remapped_pfn and
> hostp2m->max_mapped_pfn invalid.  When a gfn in this range is read, an
> EPT fault will happen, p2m_altp2m_lazy_copy() will be called, and the
> new (correct) value copied from the hostp2m.
> 
> In case B, change_type_range will write invalid entries up until
> hostp2m->max_mapped_pfn.  When a gfn in this range is accessed, a
> MISCONFIG fault will happen, and the correct value will be calculated in
> resolve_misconfig.

Or, no: resolve_misconfig() will read the current type in the altp2m,
which will be p2m_invalid; p2m_recalc_type() will then return
p2m_invalid, and then set the entry to a "plain" invalid without the
reserved bit set.  Then the fault will happen again, taking the normal
HAP fault path (calling p2m_altp2m_lazy_copy()), at which point...

> And at this point, I realize that my previous analysis was probably
> wrong, because at this point altp2m->max_remapped_gfn will be wrong:
> entries above max_remapped_gfn will have become valid without going
> through p2m_altp2m_lazy_copy().

...altp2m->max_remapped_gfn will be set appropriately.

I think. :-/

 -George

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

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

[Jan Beulich]

>>> On 16.11.18 at 16:52, <george.dunlap@citrix.com> wrote:
> On 11/16/18 3:05 PM, George Dunlap wrote:
>> Now take change_type_range.  The global effect of change_type_range
>> should be that reads of the p2m which happen afterwards should have the
>> new, changed value.
>> 
>> In case A, change_type_range will write invalid entries up to
>> max_remapped_pfn, leaving the range between max_remapped_pfn and
>> hostp2m->max_mapped_pfn invalid.  When a gfn in this range is read, an
>> EPT fault will happen, p2m_altp2m_lazy_copy() will be called, and the
>> new (correct) value copied from the hostp2m.
>> 
>> In case B, change_type_range will write invalid entries up until
>> hostp2m->max_mapped_pfn.  When a gfn in this range is accessed, a
>> MISCONFIG fault will happen, and the correct value will be calculated in
>> resolve_misconfig.
> 
> Or, no: resolve_misconfig() will read the current type in the altp2m,
> which will be p2m_invalid; p2m_recalc_type() will then return
> p2m_invalid, and then set the entry to a "plain" invalid without the
> reserved bit set.  Then the fault will happen again, taking the normal
> HAP fault path (calling p2m_altp2m_lazy_copy()), at which point...
> 
>> And at this point, I realize that my previous analysis was probably
>> wrong, because at this point altp2m->max_remapped_gfn will be wrong:
>> entries above max_remapped_gfn will have become valid without going
>> through p2m_altp2m_lazy_copy().
> 
> ...altp2m->max_remapped_gfn will be set appropriately.
> 
> I think. :-/

And I think I can follow your analysis now, which in turn means I
think your two patches are going to help, but for a proper review
I'd prefer them to be inline in a mail, rather than attachments.

Jan



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

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

[Razvan Cojocaru]

On 11/19/18 2:42 PM, Jan Beulich wrote:
>>>> On 16.11.18 at 16:52, <george.dunlap@citrix.com> wrote:
>> On 11/16/18 3:05 PM, George Dunlap wrote:
>>> Now take change_type_range.  The global effect of change_type_range
>>> should be that reads of the p2m which happen afterwards should have the
>>> new, changed value.
>>>
>>> In case A, change_type_range will write invalid entries up to
>>> max_remapped_pfn, leaving the range between max_remapped_pfn and
>>> hostp2m->max_mapped_pfn invalid.  When a gfn in this range is read, an
>>> EPT fault will happen, p2m_altp2m_lazy_copy() will be called, and the
>>> new (correct) value copied from the hostp2m.
>>>
>>> In case B, change_type_range will write invalid entries up until
>>> hostp2m->max_mapped_pfn.  When a gfn in this range is accessed, a
>>> MISCONFIG fault will happen, and the correct value will be calculated in
>>> resolve_misconfig.
>>
>> Or, no: resolve_misconfig() will read the current type in the altp2m,
>> which will be p2m_invalid; p2m_recalc_type() will then return
>> p2m_invalid, and then set the entry to a "plain" invalid without the
>> reserved bit set.  Then the fault will happen again, taking the normal
>> HAP fault path (calling p2m_altp2m_lazy_copy()), at which point...
>>
>>> And at this point, I realize that my previous analysis was probably
>>> wrong, because at this point altp2m->max_remapped_gfn will be wrong:
>>> entries above max_remapped_gfn will have become valid without going
>>> through p2m_altp2m_lazy_copy().
>>
>> ...altp2m->max_remapped_gfn will be set appropriately.
>>
>> I think. :-/
> 
> And I think I can follow your analysis now, which in turn means I
> think your two patches are going to help, but for a proper review
> I'd prefer them to be inline in a mail, rather than attachments.

Please see my review of the second patch, which won't work with my series.

Also, the patches George has sent are applicable after my series has
been applied - would you and George like me to append his first patch to
the end of my series, or would we be better off if he sends his patch
first for the current code, and I rebase my series on that?


Thanks,
Razvan

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

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

[Razvan Cojocaru]

On 11/16/18 2:03 PM, George Dunlap wrote:
> The code is definitely complicated enough, though, that I may have
> missed something, which is why I asked Razvan if there was a reason he
> changed it.
> 
> For the purposes of this patch, I propose having p2m_altp2m_init_ept()
> set max_mapped_pfn to 0 (if that works), and leaving "get rid of
> max_remapped_pfn" for a future clean-up series.

I've retraced my previous analysis and re-ran some tests, and I now
remember (sorry it took a while) why the p2m->max_mapped_pfn =
hostp2m->max_mapped_pfn was both necessary and not accidental.

Let's say we set it to 0 in p2m_altp2m_init_ept(). Then,
hap_track_dirty_vram() calls p2m_change_type_range(), which calls the
newly added change_type_range().

Change_type_range() looks like this:

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 domain *d = p2m->domain;
    int rc = 0;

    p2m->defer_nested_flush = 1;

    if ( unlikely(end > p2m->max_mapped_pfn) )
    {
        if ( !gfn )
        {
            p2m->change_entry_type_global(p2m, ot, nt);
            gfn = end;
        }
        end = p2m->max_mapped_pfn + 1;
    }
    if ( gfn < end )
        rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
    if ( rc )
    {
        printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from
%d to %d\n",
               rc, d->domain_id, start, end - 1, ot, nt);
        domain_crash(d);
    }

    switch ( nt )
    {
    case p2m_ram_rw:
        if ( ot == p2m_ram_logdirty )
            rc = rangeset_remove_range(p2m->logdirty_ranges, start, end
- 1);
        break;
    case p2m_ram_logdirty:
        if ( ot == p2m_ram_rw )
            rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
        break;
    default:
        break;
    }
    if ( rc )
    {
        printk(XENLOG_G_ERR "Error %d manipulating Dom%d's log-dirty
ranges\n",
               rc, d->domain_id);
        domain_crash(d);
    }

    p2m->defer_nested_flush = 0;
    if ( nestedhvm_enabled(d) )
        p2m_flush_nestedp2m(d);
}

If we set p2m->max_mapped_pfn to 0, we're guaranteed to run into the if
( unlikely(end > p2m->max_mapped_pfn) ) body, where end =
p2m->max_mapped_pfn + 1; will make end 1.

Then, we will crash the hypervisor in rangeset_add_range(), where
there's an ASSERT() stating that start <= end.

So my reasoning was that, since setting p2m->max_mapped_pfn =
hostp2m->max_mapped_pfn is both harmless (as both your and Jan's
analyses appear to confirm) and makes this code correct, that was the
way to go.


Thanks,
Razvan

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

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

[George Dunlap]

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

On 11/16/18 2:10 PM, Razvan Cojocaru wrote:
> On 11/16/18 2:03 PM, George Dunlap wrote:
>> The code is definitely complicated enough, though, that I may have
>> missed something, which is why I asked Razvan if there was a reason he
>> changed it.
>>
>> For the purposes of this patch, I propose having p2m_altp2m_init_ept()
>> set max_mapped_pfn to 0 (if that works), and leaving "get rid of
>> max_remapped_pfn" for a future clean-up series.
> 
> I've retraced my previous analysis and re-ran some tests, and I now
> remember (sorry it took a while) why the p2m->max_mapped_pfn =
> hostp2m->max_mapped_pfn was both necessary and not accidental.
> 
> Let's say we set it to 0 in p2m_altp2m_init_ept(). Then,
> hap_track_dirty_vram() calls p2m_change_type_range(), which calls the
> newly added change_type_range().
> 
> Change_type_range() looks like this:
> 
> 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 domain *d = p2m->domain;
>     int rc = 0;
> 
>     p2m->defer_nested_flush = 1;
> 
>     if ( unlikely(end > p2m->max_mapped_pfn) )
>     {
>         if ( !gfn )
>         {
>             p2m->change_entry_type_global(p2m, ot, nt);
>             gfn = end;
>         }
>         end = p2m->max_mapped_pfn + 1;
>     }
>     if ( gfn < end )
>         rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
>     if ( rc )
>     {
>         printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from
> %d to %d\n",
>                rc, d->domain_id, start, end - 1, ot, nt);
>         domain_crash(d);
>     }
> 
>     switch ( nt )
>     {
>     case p2m_ram_rw:
>         if ( ot == p2m_ram_logdirty )
>             rc = rangeset_remove_range(p2m->logdirty_ranges, start, end
> - 1);
>         break;
>     case p2m_ram_logdirty:
>         if ( ot == p2m_ram_rw )
>             rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
>         break;
>     default:
>         break;
>     }
>     if ( rc )
>     {
>         printk(XENLOG_G_ERR "Error %d manipulating Dom%d's log-dirty
> ranges\n",
>                rc, d->domain_id);
>         domain_crash(d);
>     }
> 
>     p2m->defer_nested_flush = 0;
>     if ( nestedhvm_enabled(d) )
>         p2m_flush_nestedp2m(d);
> }
> 
> If we set p2m->max_mapped_pfn to 0, we're guaranteed to run into the if
> ( unlikely(end > p2m->max_mapped_pfn) ) body, where end =
> p2m->max_mapped_pfn + 1; will make end 1.
> 
> Then, we will crash the hypervisor in rangeset_add_range(), where
> there's an ASSERT() stating that start <= end.

Ah, right, this was the original crash that you ran into several months
ago, which flagged up the whole logdirty range synchronization issue.

But that's partly a logic hole in change_entry_type_range(), which
assumes that start < p2m->max_mapped_pfn.  It would be better to fix
that than to work around it by changing the meaning of max_mapped_pfn.

On the other hand, we want the logdirty rangesets to actually match the
host's rangesets; using altp2m->max_mapped_pfn for this is clearly
wrong. The easiest fix would be just to explicitly use the host's
max_mapped_pfn when calculating the clipping.  A more complete fix would
involve calculating two different ranges -- a "rangeset" range and a
"invalidate" range, the second of which would be clipped on altp2ms by
{min,max}_remapped_gfn.

Something like the attached (compile-tested only).  I'm partial to
having both patches applied, but I'd be open to arguments that we should
only use the first.

 -George

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-p2m-Always-use-hostp2m-when-clipping-rangesets.patch --]
[-- Type: text/x-patch; name="0001-p2m-Always-use-hostp2m-when-clipping-rangesets.patch", Size: 4274 bytes --]

From d92bd123f92d66aef394735a6d836fd104f01867 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 16 Nov 2018 17:17:48 +0000
Subject: [PATCH 1/2] p2m: Always use hostp2m when clipping rangesets

The logdirty rangesets of the altp2ms need to be kept in sync with the
hostp2m.  This means when iterating through the altp2ms, we need to
use the host p2m to clip the rangeset, not the indiviual altp2m's
value.

This change also:

- Documents that the end is non-inclusive

- Calculates an "inclusive" value for the end once, rather than
  open-coding the modification, and (worse) back-modifying updates so
  that the calculation ends up correct

- Clarifies the logic deciding whether to call
  change_entry_type_global() or change_entry_type_range()

- Handles the case where start >= hostp2m->max_mapped_pfn

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
RFC: Wasn't sure what the best thing was to do if start >=
host_max_pfn.  We silently clip the logdirty rangeset to
max_mapped_pfn, and the chosen behavior seems consistent with that.
But it seems like such a request would almost certainly be a bug
somewhere that people might like to find out about.
---
 xen/arch/x86/mm/p2m.c | 46 +++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c401806562..6d764d1e22 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1037,32 +1037,44 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
     return rc;
 }
 
-/* Modify the p2m type of a range of gfns from ot to nt. */
+/* Modify the p2m type of [start, end) from ot to nt. */
 static void change_type_range(struct p2m_domain *p2m,
                               unsigned long start, unsigned long end,
                               p2m_type_t ot, p2m_type_t nt)
 {
-    unsigned long gfn = start;
+    unsigned long rangeset_start, rangeset_end;
     struct domain *d = p2m->domain;
+    unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
     int rc = 0;
-
+    
+    rangeset_start = start;
+    rangeset_end   = end - 1;
+    
+    /* Always clip the rangeset down to the host p2m */
+    if ( unlikely(rangeset_end > host_max_pfn) )
+        rangeset_end = host_max_pfn;
+        
+    /* If the requested range is out of scope, return doing nothing */
+    if ( rangeset_start > rangeset_end )
+        return;
+            
     p2m->defer_nested_flush = 1;
 
-    if ( unlikely(end > p2m->max_mapped_pfn) )
-    {
-        if ( !gfn )
-        {
-            p2m->change_entry_type_global(p2m, ot, nt);
-            gfn = end;
-        }
-        end = p2m->max_mapped_pfn + 1;
-    }
-    if ( gfn < end )
-        rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
+    /* 
+     * If all valid gfns are in the invalidation range, just do a
+     * global type change.  Otherwise, invalidate only the range we
+     * need.
+     */
+    if ( !rangeset_start && rangeset_end >= p2m->max_mapped_pfn)
+        p2m->change_entry_type_global(p2m, ot, nt);
+    else
+        rc = p2m->change_entry_type_range(p2m, ot, nt,
+                                          rangeset_start, rangeset_end);
+    
     if ( rc )
     {
         printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
-               rc, d->domain_id, start, end - 1, ot, nt);
+               rc, d->domain_id, rangeset_start, rangeset_end, ot, nt);
         domain_crash(d);
     }
 
@@ -1070,11 +1082,11 @@ static void change_type_range(struct p2m_domain *p2m,
     {
     case p2m_ram_rw:
         if ( ot == p2m_ram_logdirty )
-            rc = rangeset_remove_range(p2m->logdirty_ranges, start, end - 1);
+            rc = rangeset_remove_range(p2m->logdirty_ranges, rangeset_start, rangeset_end);
         break;
     case p2m_ram_logdirty:
         if ( ot == p2m_ram_rw )
-            rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
+            rc = rangeset_add_range(p2m->logdirty_ranges, rangeset_start, rangeset_end);
         break;
     default:
         break;
-- 
2.19.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-p2m-change_range_type-Only-invalidate-remapped-gfns.patch --]
[-- Type: text/x-patch; name="0002-p2m-change_range_type-Only-invalidate-remapped-gfns.patch", Size: 4499 bytes --]

From c2c6e0b9c27650607a5d15aca0d598ae7251678e Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 16 Nov 2018 16:28:25 +0000
Subject: [PATCH 2/2] p2m: change_range_type: Only invalidate remapped gfns

change_range_type() invalidates gfn ranges to lazily change the type
of a range of gfns, and also modifies the logdirty rangesets of that
p2m. At the moment, it clips both down by the hostp2m.

While this will result in correct behavior, it's not entirely efficient,
since invalidated entries outside that range will, on fault, simply be
modified back to "empty" before faulting normally again.

Separate out the calculation of the two ranges.  Keep using the
hostp2m's max_mapped_pfn to clip the logdirty ranges, but use
{max,min}_remapped_gfn to further clip the invalidation range for
alternate p2ms.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/mm/p2m.c | 59 ++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6d764d1e22..5a68e7fd78 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1043,41 +1043,64 @@ static void change_type_range(struct p2m_domain *p2m,
                               p2m_type_t ot, p2m_type_t nt)
 {
     unsigned long rangeset_start, rangeset_end;
+    unsigned long invalidate_start, invalidate_end;
     struct domain *d = p2m->domain;
     unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
+    unsigned long min_pfn = 0, max_pfn = p2m->max_mapped_pfn;
     int rc = 0;
     
-    rangeset_start = start;
-    rangeset_end   = end - 1;
+    /* 
+     * If we have an altp2m, the logdirty rangeset range needs to
+     * match that of the hostp2m, but for efficiency, we want to clip
+     * down the the invalidation range according to the mapped values
+     * in the altp2m.  Keep track of and clip the ranges separately.
+     */
+    rangeset_start = invalidate_start = start;
+    rangeset_end   = invalidate_end   = end - 1;
     
-    /* Always clip the rangeset down to the host p2m */
+    /* Clip down to the host p2m */
     if ( unlikely(rangeset_end > host_max_pfn) )
-        rangeset_end = host_max_pfn;
+        rangeset_end = invalidate_end = host_max_pfn;
         
     /* If the requested range is out of scope, return doing nothing */
     if ( rangeset_start > rangeset_end )
         return;
             
+    if ( p2m_is_altp2m(p2m) )
+    {
+        max_pfn = p2m->max_remapped_gfn;
+        min_pfn = p2m->min_remapped_gfn;
+        invalidate_end = min(invalidate_end, max_pfn);
+        invalidate_start = max(invalidate_start, min_pfn);
+    }
+        
     p2m->defer_nested_flush = 1;
 
     /* 
-     * If all valid gfns are in the invalidation range, just do a
-     * global type change.  Otherwise, invalidate only the range we
-     * need.
+     * If the p2m is empty, or the range is outside the currently
+     * mapped range, no need to do the invalidation; just update the
+     * rangeset.
      */
-    if ( !rangeset_start && rangeset_end >= p2m->max_mapped_pfn)
-        p2m->change_entry_type_global(p2m, ot, nt);
-    else
-        rc = p2m->change_entry_type_range(p2m, ot, nt,
-                                          rangeset_start, rangeset_end);
-    
-    if ( rc )
+    if ( invalidate_start < invalidate_end )
     {
-        printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
-               rc, d->domain_id, rangeset_start, rangeset_end, ot, nt);
-        domain_crash(d);
+        /* 
+         * If all valid gfns are in the invalidation range, just do a
+         * global type change.  Otherwise, invalidate only the range
+         * we need.
+         */
+        if ( invalidate_start <= min_pfn && invalidate_end >= max_pfn)
+            p2m->change_entry_type_global(p2m, ot, nt);
+        else
+            rc = p2m->change_entry_type_range(p2m, ot, nt,
+                                              invalidate_start, invalidate_end);
+        if ( rc )
+        {
+            printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
+                   rc, d->domain_id, invalidate_start, invalidate_end, ot, nt);
+            domain_crash(d);
+        }
     }
-
+    
     switch ( nt )
     {
     case p2m_ram_rw:
-- 
2.19.1


[-- Attachment #4: 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 V6 4/4] x86/altp2m: fix display frozen when switching to a new view early

[Razvan Cojocaru]

On 11/16/18 7:59 PM, George Dunlap wrote:
> On 11/16/18 2:10 PM, Razvan Cojocaru wrote:
>> On 11/16/18 2:03 PM, George Dunlap wrote:
>>> The code is definitely complicated enough, though, that I may have
>>> missed something, which is why I asked Razvan if there was a reason he
>>> changed it.
>>>
>>> For the purposes of this patch, I propose having p2m_altp2m_init_ept()
>>> set max_mapped_pfn to 0 (if that works), and leaving "get rid of
>>> max_remapped_pfn" for a future clean-up series.
>>
>> I've retraced my previous analysis and re-ran some tests, and I now
>> remember (sorry it took a while) why the p2m->max_mapped_pfn =
>> hostp2m->max_mapped_pfn was both necessary and not accidental.
>>
>> Let's say we set it to 0 in p2m_altp2m_init_ept(). Then,
>> hap_track_dirty_vram() calls p2m_change_type_range(), which calls the
>> newly added change_type_range().
>>
>> Change_type_range() looks like this:
>>
>> 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 domain *d = p2m->domain;
>>     int rc = 0;
>>
>>     p2m->defer_nested_flush = 1;
>>
>>     if ( unlikely(end > p2m->max_mapped_pfn) )
>>     {
>>         if ( !gfn )
>>         {
>>             p2m->change_entry_type_global(p2m, ot, nt);
>>             gfn = end;
>>         }
>>         end = p2m->max_mapped_pfn + 1;
>>     }
>>     if ( gfn < end )
>>         rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
>>     if ( rc )
>>     {
>>         printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from
>> %d to %d\n",
>>                rc, d->domain_id, start, end - 1, ot, nt);
>>         domain_crash(d);
>>     }
>>
>>     switch ( nt )
>>     {
>>     case p2m_ram_rw:
>>         if ( ot == p2m_ram_logdirty )
>>             rc = rangeset_remove_range(p2m->logdirty_ranges, start, end
>> - 1);
>>         break;
>>     case p2m_ram_logdirty:
>>         if ( ot == p2m_ram_rw )
>>             rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
>>         break;
>>     default:
>>         break;
>>     }
>>     if ( rc )
>>     {
>>         printk(XENLOG_G_ERR "Error %d manipulating Dom%d's log-dirty
>> ranges\n",
>>                rc, d->domain_id);
>>         domain_crash(d);
>>     }
>>
>>     p2m->defer_nested_flush = 0;
>>     if ( nestedhvm_enabled(d) )
>>         p2m_flush_nestedp2m(d);
>> }
>>
>> If we set p2m->max_mapped_pfn to 0, we're guaranteed to run into the if
>> ( unlikely(end > p2m->max_mapped_pfn) ) body, where end =
>> p2m->max_mapped_pfn + 1; will make end 1.
>>
>> Then, we will crash the hypervisor in rangeset_add_range(), where
>> there's an ASSERT() stating that start <= end.
> 
> Ah, right, this was the original crash that you ran into several months
> ago, which flagged up the whole logdirty range synchronization issue.
> 
> But that's partly a logic hole in change_entry_type_range(), which
> assumes that start < p2m->max_mapped_pfn.  It would be better to fix
> that than to work around it by changing the meaning of max_mapped_pfn.
> 
> On the other hand, we want the logdirty rangesets to actually match the
> host's rangesets; using altp2m->max_mapped_pfn for this is clearly
> wrong. The easiest fix would be just to explicitly use the host's
> max_mapped_pfn when calculating the clipping.  A more complete fix would
> involve calculating two different ranges -- a "rangeset" range and a
> "invalidate" range, the second of which would be clipped on altp2ms by
> {min,max}_remapped_gfn.
> 
> Something like the attached (compile-tested only).  I'm partial to
> having both patches applied, but I'd be open to arguments that we should
> only use the first.

Thanks! I haven't yet been able to think in depth about the logic, but I
did manage to apply them. Just applying the first one allows me to set
p2m->max_mapped_pfn = 0; without the ASSERT() crashing the hypervisor,
and everything appears to work well.

With both patches applies, the display remains frozen (things appear to
behave - externally - in the same way as before the series).


Razvan

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

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

[Razvan Cojocaru]

On 11/16/18 9:50 PM, Razvan Cojocaru wrote:
> On 11/16/18 7:59 PM, George Dunlap wrote:
>> On the other hand, we want the logdirty rangesets to actually match the
>> host's rangesets; using altp2m->max_mapped_pfn for this is clearly
>> wrong. The easiest fix would be just to explicitly use the host's
>> max_mapped_pfn when calculating the clipping.  A more complete fix would
>> involve calculating two different ranges -- a "rangeset" range and a
>> "invalidate" range, the second of which would be clipped on altp2ms by
>> {min,max}_remapped_gfn.
>>
>> Something like the attached (compile-tested only).  I'm partial to
>> having both patches applied, but I'd be open to arguments that we should
>> only use the first.
> 
> Thanks! I haven't yet been able to think in depth about the logic, but I
> did manage to apply them. Just applying the first one allows me to set
> p2m->max_mapped_pfn = 0; without the ASSERT() crashing the hypervisor,
> and everything appears to work well.
> 
> With both patches applies, the display remains frozen (things appear to
> behave - externally - in the same way as before the series).

Right, I know why the second patch keeps the display frozen. It's
because for altp2ms (where it matters most), the patch basically does
invalidate_start = max(invalidate_start, p2m->min_remapped_gfn) and
invalidate_end = min(invalidate_end, p2m->max_remapped_gfn).

However, as previously requested, I've now made p2m->max_remapped_gfn
begin life as 0 for altp2ms, and p2m->min_remapped_gfn is initialized to
INVALID_GFN, which is decimal 18446744073709551615. So we get
invalidate_end: 0, invalidate_start: 18446744073709551615,
invalidate_end < invalidate_start, resulting in nothing being done for
altp2ms, which is functionally back to square one.

In light of this analysis, I suppose I should now also disregard the
recommendation to clip first_gfn and max_nr to fit within
p2m->min_remapped_gfn and p2m->max_remapped_gfn for altp2ms in
finish_type_change(), for obvious reasons - unless you prefer a
different strategy.


Thanks,
Razvan

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

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

[George Dunlap]

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

On 11/17/18 6:58 PM, Razvan Cojocaru wrote:
> On 11/16/18 9:50 PM, Razvan Cojocaru wrote:
>> On 11/16/18 7:59 PM, George Dunlap wrote:
>>> On the other hand, we want the logdirty rangesets to actually match the
>>> host's rangesets; using altp2m->max_mapped_pfn for this is clearly
>>> wrong. The easiest fix would be just to explicitly use the host's
>>> max_mapped_pfn when calculating the clipping.  A more complete fix would
>>> involve calculating two different ranges -- a "rangeset" range and a
>>> "invalidate" range, the second of which would be clipped on altp2ms by
>>> {min,max}_remapped_gfn.
>>>
>>> Something like the attached (compile-tested only).  I'm partial to
>>> having both patches applied, but I'd be open to arguments that we should
>>> only use the first.
>>
>> Thanks! I haven't yet been able to think in depth about the logic, but I
>> did manage to apply them. Just applying the first one allows me to set
>> p2m->max_mapped_pfn = 0; without the ASSERT() crashing the hypervisor,
>> and everything appears to work well.
>>
>> With both patches applies, the display remains frozen (things appear to
>> behave - externally - in the same way as before the series).
> 
> Right, I know why the second patch keeps the display frozen. It's
> because for altp2ms (where it matters most), the patch basically does
> invalidate_start = max(invalidate_start, p2m->min_remapped_gfn) and
> invalidate_end = min(invalidate_end, p2m->max_remapped_gfn).
> 
> However, as previously requested, I've now made p2m->max_remapped_gfn
> begin life as 0 for altp2ms, and p2m->min_remapped_gfn is initialized to
> INVALID_GFN, which is decimal 18446744073709551615. So we get
> invalidate_end: 0, invalidate_start: 18446744073709551615,
> invalidate_end < invalidate_start, resulting in nothing being done for
> altp2ms, which is functionally back to square one.

But this doesn't explain why my reasoning was wrong; and it's always
dangerous to use a system whose behavior you don't really understand,
even if it seems to work.

It turns out I'd made a mistake in saying that altp2m->max_mapped_pfn ==
alt2m->max_remapped_gfn.  The first is the highest gfn present in the
altp2m, either copied from the hostp2m or changed; the second is the
highest value changed (via p2m_altp2m_change_gfn()).

What about using the attached patch instead?

 -George

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-p2m-change_range_type-Only-invalidate-mapped-gfns.patch --]
[-- Type: text/x-patch; name="0002-p2m-change_range_type-Only-invalidate-mapped-gfns.patch", Size: 4550 bytes --]

From f4c72ecb95cfa5d597b8e28e99e681cfeaa32199 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 16 Nov 2018 16:28:25 +0000
Subject: [PATCH 2/2] p2m: change_range_type: Only invalidate mapped gfns

change_range_type() invalidates gfn ranges to lazily change the type
of a range of gfns, and also modifies the logdirty rangesets of that
p2m. At the moment, it clips both down by the hostp2m.

While this will result in correct behavior, it's not entirely efficient,
since invalidated entries outside that range will, on fault, simply be
modified back to "empty" before faulting normally again.

Separate out the calculation of the two ranges.  Keep using the
hostp2m's max_mapped_pfn to clip the logdirty ranges, but use the
current p2m's max_mapped_pfn to further clip the invalidation range
for alternate p2ms.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/mm/p2m.c | 61 +++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6d764d1e22..b97e138452 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1043,41 +1043,64 @@ static void change_type_range(struct p2m_domain *p2m,
                               p2m_type_t ot, p2m_type_t nt)
 {
     unsigned long rangeset_start, rangeset_end;
+    unsigned long invalidate_start, invalidate_end;
     struct domain *d = p2m->domain;
     unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
+    unsigned long max_pfn = p2m->max_mapped_pfn;
     int rc = 0;
     
-    rangeset_start = start;
-    rangeset_end   = end - 1;
+    /* 
+     * If we have an altp2m, the logdirty rangeset range needs to
+     * match that of the hostp2m, but for efficiency, we want to clip
+     * down the the invalidation range according to the mapped values
+     * in the altp2m.  Keep track of and clip the ranges separately.
+     */
+    rangeset_start = invalidate_start = start;
+    rangeset_end   = invalidate_end   = end - 1;
     
-    /* Always clip the rangeset down to the host p2m */
+    /* Clip down to the host p2m */
     if ( unlikely(rangeset_end > host_max_pfn) )
-        rangeset_end = host_max_pfn;
+        rangeset_end = invalidate_end = host_max_pfn;
         
     /* If the requested range is out of scope, return doing nothing */
     if ( rangeset_start > rangeset_end )
         return;
             
+    if ( p2m_is_altp2m(p2m) )
+        invalidate_end = min(invalidate_end, max_pfn);
+        
     p2m->defer_nested_flush = 1;
 
     /* 
-     * If all valid gfns are in the invalidation range, just do a
-     * global type change.  Otherwise, invalidate only the range we
-     * need.
+     * If the p2m is empty, or the range is outside the currently
+     * mapped range, no need to do the invalidation; just update the
+     * rangeset.
      */
-    if ( !rangeset_start && rangeset_end >= p2m->max_mapped_pfn)
-        p2m->change_entry_type_global(p2m, ot, nt);
-    else
-        rc = p2m->change_entry_type_range(p2m, ot, nt,
-                                          rangeset_start, rangeset_end);
-    
-    if ( rc )
-    {
-        printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
-               rc, d->domain_id, rangeset_start, rangeset_end, ot, nt);
-        domain_crash(d);
+    if ( invalidate_start < invalidate_end )
+    {
+        /* 
+         * If all valid gfns are in the invalidation range, just do a
+         * global type change.  Otherwise, invalidate only the range
+         * we need.  
+         *
+         * NB that invalidate_end can't logically be >max_pfn at this
+         * point.  If this changes, the == will need to be changed to
+         * >=.
+         */
+        ASSERT(invalidate_end <= max_pfn);
+        if ( !invalidate_start && invalidate_end == max_pfn)
+            p2m->change_entry_type_global(p2m, ot, nt);
+        else
+            rc = p2m->change_entry_type_range(p2m, ot, nt,
+                                              invalidate_start, invalidate_end);
+        if ( rc )
+        {
+            printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from %d to %d\n",
+                   rc, d->domain_id, invalidate_start, invalidate_end, ot, nt);
+            domain_crash(d);
+        }
     }
-
+    
     switch ( nt )
     {
     case p2m_ram_rw:
-- 
2.19.1


[-- 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 V6 4/4] x86/altp2m: fix display frozen when switching to a new view early

[Razvan Cojocaru]

On 11/19/18 5:58 PM, George Dunlap wrote:
> On 11/17/18 6:58 PM, Razvan Cojocaru wrote:
>> On 11/16/18 9:50 PM, Razvan Cojocaru wrote:
>>> On 11/16/18 7:59 PM, George Dunlap wrote:
>>>> On the other hand, we want the logdirty rangesets to actually match the
>>>> host's rangesets; using altp2m->max_mapped_pfn for this is clearly
>>>> wrong. The easiest fix would be just to explicitly use the host's
>>>> max_mapped_pfn when calculating the clipping.  A more complete fix would
>>>> involve calculating two different ranges -- a "rangeset" range and a
>>>> "invalidate" range, the second of which would be clipped on altp2ms by
>>>> {min,max}_remapped_gfn.
>>>>
>>>> Something like the attached (compile-tested only).  I'm partial to
>>>> having both patches applied, but I'd be open to arguments that we should
>>>> only use the first.
>>>
>>> Thanks! I haven't yet been able to think in depth about the logic, but I
>>> did manage to apply them. Just applying the first one allows me to set
>>> p2m->max_mapped_pfn = 0; without the ASSERT() crashing the hypervisor,
>>> and everything appears to work well.
>>>
>>> With both patches applies, the display remains frozen (things appear to
>>> behave - externally - in the same way as before the series).
>>
>> Right, I know why the second patch keeps the display frozen. It's
>> because for altp2ms (where it matters most), the patch basically does
>> invalidate_start = max(invalidate_start, p2m->min_remapped_gfn) and
>> invalidate_end = min(invalidate_end, p2m->max_remapped_gfn).
>>
>> However, as previously requested, I've now made p2m->max_remapped_gfn
>> begin life as 0 for altp2ms, and p2m->min_remapped_gfn is initialized to
>> INVALID_GFN, which is decimal 18446744073709551615. So we get
>> invalidate_end: 0, invalidate_start: 18446744073709551615,
>> invalidate_end < invalidate_start, resulting in nothing being done for
>> altp2ms, which is functionally back to square one.
> 
> But this doesn't explain why my reasoning was wrong; and it's always
> dangerous to use a system whose behavior you don't really understand,
> even if it seems to work.
> 
> It turns out I'd made a mistake in saying that altp2m->max_mapped_pfn ==
> alt2m->max_remapped_gfn.  The first is the highest gfn present in the
> altp2m, either copied from the hostp2m or changed; the second is the
> highest value changed (via p2m_altp2m_change_gfn()).
> 
> What about using the attached patch instead?

The attached patch does work, thanks! Shall I append them both to the
end of the series and send out V7?


Thanks,
Razvan

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

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

[George Dunlap]

On 11/19/18 4:17 PM, Razvan Cojocaru wrote:
> On 11/19/18 5:58 PM, George Dunlap wrote:
>> On 11/17/18 6:58 PM, Razvan Cojocaru wrote:
>>> On 11/16/18 9:50 PM, Razvan Cojocaru wrote:
>>>> On 11/16/18 7:59 PM, George Dunlap wrote:
>>>>> On the other hand, we want the logdirty rangesets to actually match the
>>>>> host's rangesets; using altp2m->max_mapped_pfn for this is clearly
>>>>> wrong. The easiest fix would be just to explicitly use the host's
>>>>> max_mapped_pfn when calculating the clipping.  A more complete fix would
>>>>> involve calculating two different ranges -- a "rangeset" range and a
>>>>> "invalidate" range, the second of which would be clipped on altp2ms by
>>>>> {min,max}_remapped_gfn.
>>>>>
>>>>> Something like the attached (compile-tested only).  I'm partial to
>>>>> having both patches applied, but I'd be open to arguments that we should
>>>>> only use the first.
>>>>
>>>> Thanks! I haven't yet been able to think in depth about the logic, but I
>>>> did manage to apply them. Just applying the first one allows me to set
>>>> p2m->max_mapped_pfn = 0; without the ASSERT() crashing the hypervisor,
>>>> and everything appears to work well.
>>>>
>>>> With both patches applies, the display remains frozen (things appear to
>>>> behave - externally - in the same way as before the series).
>>>
>>> Right, I know why the second patch keeps the display frozen. It's
>>> because for altp2ms (where it matters most), the patch basically does
>>> invalidate_start = max(invalidate_start, p2m->min_remapped_gfn) and
>>> invalidate_end = min(invalidate_end, p2m->max_remapped_gfn).
>>>
>>> However, as previously requested, I've now made p2m->max_remapped_gfn
>>> begin life as 0 for altp2ms, and p2m->min_remapped_gfn is initialized to
>>> INVALID_GFN, which is decimal 18446744073709551615. So we get
>>> invalidate_end: 0, invalidate_start: 18446744073709551615,
>>> invalidate_end < invalidate_start, resulting in nothing being done for
>>> altp2ms, which is functionally back to square one.
>>
>> But this doesn't explain why my reasoning was wrong; and it's always
>> dangerous to use a system whose behavior you don't really understand,
>> even if it seems to work.
>>
>> It turns out I'd made a mistake in saying that altp2m->max_mapped_pfn ==
>> alt2m->max_remapped_gfn.  The first is the highest gfn present in the
>> altp2m, either copied from the hostp2m or changed; the second is the
>> highest value changed (via p2m_altp2m_change_gfn()).
>>
>> What about using the attached patch instead?
> 
> The attached patch does work, thanks! Shall I append them both to the
> end of the series and send out V7?

Yes, please.

 -George

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

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

[George Dunlap]

> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
> 
> When an new altp2m view is created very early on guest boot, the
> display will freeze (although the guest will run normally). This
> may also happen on resizing the display. The reason is the way
> Xen currently (mis)handles logdirty VGA: it intentionally
> misconfigures VGA pages so that they will fault.
> 
> The problem is that it only does this in the host p2m. Once we
> switch to a new altp2m, the misconfigured entries will no longer
> fault, so the display will not be updated.
> 
> This patch:
> * updates ept_handle_misconfig() to use the active altp2m instead
>  of the hostp2m;
> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>  and p2m_change_type_range() to propagate their changes to all
>  valid altp2ms.
> 
> With the introduction of altp2m fields in p2m_memory_type_changed()
> the whole function has been put under CONFIG_HVM.

Sorry — actually, I think there’s yet another issue lurking here:  p2m_finish_type_change(). I’ll poke around a bit more tomorrow.

 -George

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

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

[Razvan Cojocaru]

On 11/15/18 10:26 PM, George Dunlap wrote:
> 
> 
>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
>>
>> When an new altp2m view is created very early on guest boot, the
>> display will freeze (although the guest will run normally). This
>> may also happen on resizing the display. The reason is the way
>> Xen currently (mis)handles logdirty VGA: it intentionally
>> misconfigures VGA pages so that they will fault.
>>
>> The problem is that it only does this in the host p2m. Once we
>> switch to a new altp2m, the misconfigured entries will no longer
>> fault, so the display will not be updated.
>>
>> This patch:
>> * updates ept_handle_misconfig() to use the active altp2m instead
>>  of the hostp2m;
>> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>>  and p2m_change_type_range() to propagate their changes to all
>>  valid altp2ms.
>>
>> With the introduction of altp2m fields in p2m_memory_type_changed()
>> the whole function has been put under CONFIG_HVM.
> 
> Sorry — actually, I think there’s yet another issue lurking here:  p2m_finish_type_change(). I’ll poke around a bit more tomorrow.

OK, I'll wait for your recommendation before working on the last patch
of the series.


Thanks,
Razvan

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

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

[George Dunlap]

On 11/15/18 8:51 PM, Razvan Cojocaru wrote:
> On 11/15/18 10:26 PM, George Dunlap wrote:
>>
>>
>>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote:
>>>
>>> When an new altp2m view is created very early on guest boot, the
>>> display will freeze (although the guest will run normally). This
>>> may also happen on resizing the display. The reason is the way
>>> Xen currently (mis)handles logdirty VGA: it intentionally
>>> misconfigures VGA pages so that they will fault.
>>>
>>> The problem is that it only does this in the host p2m. Once we
>>> switch to a new altp2m, the misconfigured entries will no longer
>>> fault, so the display will not be updated.
>>>
>>> This patch:
>>> * updates ept_handle_misconfig() to use the active altp2m instead
>>>  of the hostp2m;
>>> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>>>  and p2m_change_type_range() to propagate their changes to all
>>>  valid altp2ms.
>>>
>>> With the introduction of altp2m fields in p2m_memory_type_changed()
>>> the whole function has been put under CONFIG_HVM.
>>
>> Sorry — actually, I think there’s yet another issue lurking here:  p2m_finish_type_change(). I’ll poke around a bit more tomorrow.
> 
> OK, I'll wait for your recommendation before working on the last patch
> of the series.

It looks like you'll want to do the same thing to
p2m_finish_type_change() that you did for p2m_change_type_range():
create a finish_type_change() function which loops over the appropriate
range calling p2m->recalc(), and then have p2m_finish_type_change()
first call this on the hostp2m, then one-by-one on all valid altp2ms.

In finish_type_change() you'll want to clip first_gfn and max_nr to fit
within p2m->min_remapped_gfn and p2m->max_remapped_gfn for altp2ms.

I think that should do the trick.

 -George

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