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

[PATCH RFC] 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;
* has p2m_init_altp2m_ept() copy over max_mapped_pfn,
  logdirty_ranges, global_logdirty, ept.ad and default_access
  from the hostp2m (the latter more for completeness than for any
  other reason). We should discuss if just copying over
  logdirty_ranges (which is a pointer) is sufficient, or if
  this code requires more synchronization). Also, it's worth
  clarifying if these variables (and which of them) should be
  copied over from the hostp2m or the currently active p2m;
* modifies p2m_change_entry_type_global() and
  p2m_change_type_range() to propagate their changes to all
  valid altp2ms.

Another aspect is that, while new modifications should work with
these changes, _old_ modifications (written to the host2pm
_before_ we've created the new altp2m) will, if I understand the
code correctly be lost. That is to say, misconfigurations
performed before p2m_init_altp2m_ept() in the hostp2m will
presumably not trigger the necessary faults after switching to
the new altp2m.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/mm/p2m-ept.c | 53 +++++++++++++++++++++++++++++++++++++++++------
 xen/arch/x86/mm/p2m.c     | 49 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 87 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 14b5939..76de6b3 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>
@@ -656,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;
@@ -1209,32 +1213,60 @@ static void ept_tlb_flush(struct p2m_domain *p2m)
 
 static void ept_enable_pml(struct p2m_domain *p2m)
 {
+    unsigned int i;
+    struct domain *d = p2m->domain;
+
     /* Domain must have been paused */
-    ASSERT(atomic_read(&p2m->domain->pause_count));
+    ASSERT(atomic_read(&d->pause_count));
 
     /*
      * No need to return whether vmx_domain_enable_pml has succeeded, as
      * ept_p2m_type_to_flags will do the check, and write protection will be
      * used if PML is not enabled.
      */
-    if ( vmx_domain_enable_pml(p2m->domain) )
+    if ( vmx_domain_enable_pml(d) )
         return;
 
     /* Enable EPT A/D bit for PML */
     p2m->ept.ad = 1;
-    vmx_domain_update_eptp(p2m->domain);
+
+    if ( altp2m_active(d) )
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+        {
+            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+                continue;
+
+            p2m = d->arch.altp2m_p2m[i];
+            p2m->ept.ad = 1;
+        }
+
+    vmx_domain_update_eptp(d);
 }
 
 static void ept_disable_pml(struct p2m_domain *p2m)
 {
+    unsigned int i;
+    struct domain *d = p2m->domain;
+
     /* Domain must have been paused */
-    ASSERT(atomic_read(&p2m->domain->pause_count));
+    ASSERT(atomic_read(&d->pause_count));
 
-    vmx_domain_disable_pml(p2m->domain);
+    vmx_domain_disable_pml(d);
 
     /* Disable EPT A/D bit */
     p2m->ept.ad = 0;
-    vmx_domain_update_eptp(p2m->domain);
+
+    if ( altp2m_active(d) )
+        for ( i = 0; i < MAX_ALTP2M; i++ )
+        {
+            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
+                continue;
+
+            p2m = d->arch.altp2m_p2m[i];
+            p2m->ept.ad = 0;
+        }
+
+    vmx_domain_update_eptp(d);
 }
 
 static void ept_flush_pml_buffers(struct p2m_domain *p2m)
@@ -1375,11 +1407,20 @@ 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->max_mapped_pfn = hostp2m->max_mapped_pfn;
+    p2m->default_access = hostp2m->default_access;
+    p2m->domain = hostp2m->domain;
+    p2m->logdirty_ranges = hostp2m->logdirty_ranges;
+    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;
     ept = &p2m->ept;
+
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
     d->arch.altp2m_eptp[i] = ept->eptp;
 }
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6020553..bbbc0bf 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -28,6 +28,7 @@
 #include <xen/vm_event.h>
 #include <xen/event.h>
 #include <public/vm_event.h>
+#include <asm/altp2m.h>
 #include <asm/domain.h>
 #include <asm/page.h>
 #include <asm/paging.h>
@@ -249,7 +250,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;
@@ -258,11 +258,9 @@ int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
     return 0;
 }
 
-void p2m_change_entry_type_global(struct domain *d,
-                                  p2m_type_t ot, p2m_type_t nt)
+static void _p2m_change_entry_type_global(struct p2m_domain *p2m,
+                                          p2m_type_t ot, p2m_type_t nt)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
-
     ASSERT(ot != nt);
     ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
 
@@ -272,6 +270,22 @@ void p2m_change_entry_type_global(struct domain *d,
     p2m_unlock(p2m);
 }
 
+void p2m_change_entry_type_global(struct domain *d,
+                                  p2m_type_t ot, p2m_type_t nt)
+{
+    unsigned int i;
+
+    if ( !altp2m_active(d) )
+    {
+        _p2m_change_entry_type_global(p2m_get_hostp2m(d), ot, nt);
+        return;
+    }
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+        if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            _p2m_change_entry_type_global(d->arch.altp2m_p2m[i], ot, nt);
+}
+
 void p2m_memory_type_changed(struct domain *d)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -965,12 +979,12 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
 }
 
 /* Modify the p2m type of a range of gfns from ot to nt. */
-void p2m_change_type_range(struct domain *d, 
-                           unsigned long start, unsigned long end,
-                           p2m_type_t ot, p2m_type_t nt)
+static void _p2m_change_type_range(struct p2m_domain *p2m,
+                                   unsigned long start, unsigned long end,
+                                   p2m_type_t ot, p2m_type_t nt)
 {
+    struct domain *d = p2m->domain;
     unsigned long gfn = start;
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
     int rc = 0;
 
     ASSERT(ot != nt);
@@ -1023,6 +1037,23 @@ void p2m_change_type_range(struct domain *d,
     p2m_unlock(p2m);
 }
 
+void p2m_change_type_range(struct domain *d,
+                           unsigned long start, unsigned long end,
+                           p2m_type_t ot, p2m_type_t nt)
+{
+    unsigned int i;
+
+    if ( !altp2m_active(d) )
+    {
+        _p2m_change_type_range(p2m_get_hostp2m(d), start, end, ot, nt);
+        return;
+    }
+
+    for ( i = 0; i < MAX_ALTP2M; i++ )
+        if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
+            _p2m_change_type_range(d->arch.altp2m_p2m[i], start, end, ot, nt);
+}
+
 /*
  * Finish p2m type change for gfns which are marked as need_recalc in a range.
  * Returns: 0/1 for success, negative for failure
-- 
2.7.4


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

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

[Jan Beulich]

>>> On 03.09.18 at 10:25, <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;

Wouldn't you, as a prereq to this, first need to make sure global
changes leading to EPT misconfig exits get mirrored to all altp2m-s?
The prime example is p2m_memory_type_changed(), which only
acts on the hostp2m as well.

Jan



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

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

[Razvan Cojocaru]

On 9/3/18 4:27 PM, Jan Beulich wrote:
>>>> On 03.09.18 at 10:25, <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;
> 
> Wouldn't you, as a prereq to this, first need to make sure global
> changes leading to EPT misconfig exits get mirrored to all altp2m-s?
> The prime example is p2m_memory_type_changed(), which only
> acts on the hostp2m as well.

Right, I think that might be needed as well. There's always a corner
case there too, I think: the userspace agent may always activate altp2m
on a domain _after_ the misconfiguration has already happened (on the
hostp2m only), so nothing got propagated when it should have.

Ideally all the code would be updated to use the active (alt)p2m instead
of the hostp2m. However, altp2ms are second-class citizens in Xen - for
example the current code pays no attention to altp2m->logdirty_ranges
(which is only allocated in p2m_init_hostp2m(), and left as 0 - or NULL
- by p2m_init_altp2m()).

I think what we need is:

1. Create altp2ms with as much information as the hostp2m.

2. On switch, copy over all relevant information from the currently
active p2m (which may or may not be the host p2m).

This can get very subtle, since one approach would be to duplicate
information in altp2ms, another would be to share (and thus have to
synchronize access to) that information between views (i.e.
pointer-to-struct). logdirty_ranges is the best example of that - it
should have the exact same contents for all views as far as I can tell.
The EPT information for misconfigurations is another example.

Clearly this is quite complicated code, hopefully George will chime in
when he gets a chance.


Thanks,
Razvan

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

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

[George Dunlap]

On 09/03/2018 09:25 AM, Razvan Cojocaru wrote:
> When an new altp2m view is created very early on guest boot, the
> display will freeze (although the guest will run normally). This
> may also happen on resizing the display. The reason is the way
> Xen currently (mis)handles logdirty VGA: it intentionally
> misconfigures VGA pages so that they will fault.
> 
> The problem is that it only does this in the host p2m. Once we
> switch to a new altp2m, the misconfigured entries will no longer
> fault, so the display will not be updated.

Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.

> This patch:
> 
> * updates ept_handle_misconfig() to use the active altp2m instead
>   of the hostp2m;

This is probably necessary.

> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>   logdirty_ranges, global_logdirty, ept.ad and default_access
>   from the hostp2m (the latter more for completeness than for any
>   other reason).

I think this is probably the right approach.  These values change
rarely, but after a misconfig are read repeatedly.  So it's probably a
lot more efficient to propagate changes when they happen, rather than
trying to keep a single master copy.  However...

> We should discuss if just copying over
>   logdirty_ranges (which is a pointer) is sufficient, or if
>   this code requires more synchronization).

It's clearly not sufficient. :-)  The logdirty_ranges struct is
protected by the lock of the p2m structure that contains it; if you
point to it from a different p2m structure, then you'll have
inconsistent logging, and you'll have problems if one vcpu is reading
the structure while another is modifying it.

Another issue is that it doesn't look like you're propagating updates to
this shared state either -- if someone enables or disables
global_logdirty, or changes default_access, the altp2ms will still have
the old value.

I wonder if we should collect the various bits that need to be kept in
sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within
the p2m, and enforce using a function / macro to modify the values inside.

> Also, it's worth
>   clarifying if these variables (and which of them) should be
>   copied over from the hostp2m or the currently active p2m;
> * modifies p2m_change_entry_type_global() and
>   p2m_change_type_range() to propagate their changes to all
>   valid altp2ms.
> 
> Another aspect is that, while new modifications should work with
> these changes, _old_ modifications (written to the host2pm
> _before_ we've created the new altp2m) will, if I understand the
> code correctly be lost. That is to say, misconfigurations
> performed before p2m_init_altp2m_ept() in the hostp2m will
> presumably not trigger the necessary faults after switching to
> the new altp2m.

You're worried about the following sequence?

1. Misconfigure hostp2m
2. Enable altp2m
3. Switch to altp2m 1
4. Fault on a previously-misconfigured p2m entry

Actually, I'm pretty sure you don't have to worry about this (unless
I've completely forgotten how things work). Correct me if I'm wrong:

* The altp2ms start out as empty, and entries are copied from the host
p2m as needed, using hostp2m->get_entry() (at some level)

* There are a *lot* of users that call p2m->get_entry() without causing
a fault; these callers need to get the right values during a
misconfigure (the remnants of which may last indefinitely -- i.e.,
misconfigured entries may *never* be fixed up for regions of the p2m
which aren't touched)

So the fault in 4 should end up copying over the correct value (unless I
missed something).

Just one comment on the code...

>  static void ept_enable_pml(struct p2m_domain *p2m)
>  {
> +    unsigned int i;
> +    struct domain *d = p2m->domain;
> +
>      /* Domain must have been paused */
> -    ASSERT(atomic_read(&p2m->domain->pause_count));
> +    ASSERT(atomic_read(&d->pause_count));
>  
>      /*
>       * No need to return whether vmx_domain_enable_pml has succeeded, as
>       * ept_p2m_type_to_flags will do the check, and write protection will be
>       * used if PML is not enabled.
>       */
> -    if ( vmx_domain_enable_pml(p2m->domain) )
> +    if ( vmx_domain_enable_pml(d) )
>          return;
>  
>      /* Enable EPT A/D bit for PML */
>      p2m->ept.ad = 1;
> -    vmx_domain_update_eptp(p2m->domain);
> +
> +    if ( altp2m_active(d) )
> +        for ( i = 0; i < MAX_ALTP2M; i++ )
> +        {
> +            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> +                continue;
> +
> +            p2m = d->arch.altp2m_p2m[i];
> +            p2m->ept.ad = 1;
> +        }

You're not grabbing the respective p2m locks here -- I'm pretty sure
this will end up being three separate instructions (read, set ad bit,
write).

But there would something a bit funny here about grabbing the main p2m
lock in p2m.c, and then grabbing altp2m locks within the function.  But
on the other hand, you clearly only want to call this...

> +    vmx_domain_update_eptp(d);

...once.  Some refactoring might be wanted.

 -George

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

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

[George Dunlap]

On 09/19/2018 01:15 PM, George Dunlap wrote:
> On 09/03/2018 09:25 AM, Razvan Cojocaru wrote:
>> When an new altp2m view is created very early on guest boot, the
>> display will freeze (although the guest will run normally). This
>> may also happen on resizing the display. The reason is the way
>> Xen currently (mis)handles logdirty VGA: it intentionally
>> misconfigures VGA pages so that they will fault.
>>
>> The problem is that it only does this in the host p2m. Once we
>> switch to a new altp2m, the misconfigured entries will no longer
>> fault, so the display will not be updated.
> 
> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.
> 
>> This patch:
>>
>> * updates ept_handle_misconfig() to use the active altp2m instead
>>   of the hostp2m;
> 
> This is probably necessary.
> 
>> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>>   logdirty_ranges, global_logdirty, ept.ad and default_access
>>   from the hostp2m (the latter more for completeness than for any
>>   other reason).
> 
> I think this is probably the right approach.  These values change
> rarely, but after a misconfig are read repeatedly.  So it's probably a
> lot more efficient to propagate changes when they happen, rather than
> trying to keep a single master copy.  However...
> 
>> We should discuss if just copying over
>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>   this code requires more synchronization).
> 
> It's clearly not sufficient. :-)  The logdirty_ranges struct is
> protected by the lock of the p2m structure that contains it; if you
> point to it from a different p2m structure, then you'll have
> inconsistent logging, and you'll have problems if one vcpu is reading
> the structure while another is modifying it.

...and therefore, if we believe that it's more efficient to duplicate
structures than to share it and use a lock, we need to do a deep copy of
the data structure on altp2m creation, and propagate changes as we do
for the other "synced" data.

 -George

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

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

[Razvan Cojocaru]

On 9/19/18 3:15 PM, George Dunlap wrote:
> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.

No problem, thanks for the review!

>> We should discuss if just copying over
>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>   this code requires more synchronization).
> 
> It's clearly not sufficient. :-)  The logdirty_ranges struct is
> protected by the lock of the p2m structure that contains it; if you
> point to it from a different p2m structure, then you'll have
> inconsistent logging, and you'll have problems if one vcpu is reading
> the structure while another is modifying it.
> 
> Another issue is that it doesn't look like you're propagating updates to
> this shared state either -- if someone enables or disables
> global_logdirty, or changes default_access, the altp2ms will still have
> the old value.
> 
> I wonder if we should collect the various bits that need to be kept in
> sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within
> the p2m, and enforce using a function / macro to modify the values inside.

Right, I'll get on with the sync structure then.

>> Another aspect is that, while new modifications should work with
>> these changes, _old_ modifications (written to the host2pm
>> _before_ we've created the new altp2m) will, if I understand the
>> code correctly be lost. That is to say, misconfigurations
>> performed before p2m_init_altp2m_ept() in the hostp2m will
>> presumably not trigger the necessary faults after switching to
>> the new altp2m.
> 
> You're worried about the following sequence?
> 
> 1. Misconfigure hostp2m
> 2. Enable altp2m
> 3. Switch to altp2m 1
> 4. Fault on a previously-misconfigured p2m entry

No, I'm worried that at step 4 the fault will no longer happen, because
the EPTP index corresponding to altp2m 1 points to an EPT where the
entries misconfigured in the hostp2m are _not_ misconfigured.

But actually the sequence I'm worried about is:

1. Misconfigure hostp2m
2. Create altp2m
3. Enable altp2m
4. Switch to altp2m 1
5. A would-be-fault in the hostp2m no longer occurs

Please note the additional step 2. At this point, the hostp2m has been
misconfigured, but the creation of altp2m came too late - so the
misconfiguration of the hostp2m could not have propagated to altp2m 1,
since it didn't yet exist when it was misconfigured.

Does not switching to altp2m 1 then lose the EPT misconfiguration?

> Just one comment on the code...
> 
>>  static void ept_enable_pml(struct p2m_domain *p2m)
>>  {
>> +    unsigned int i;
>> +    struct domain *d = p2m->domain;
>> +
>>      /* Domain must have been paused */
>> -    ASSERT(atomic_read(&p2m->domain->pause_count));
>> +    ASSERT(atomic_read(&d->pause_count));
>>  
>>      /*
>>       * No need to return whether vmx_domain_enable_pml has succeeded, as
>>       * ept_p2m_type_to_flags will do the check, and write protection will be
>>       * used if PML is not enabled.
>>       */
>> -    if ( vmx_domain_enable_pml(p2m->domain) )
>> +    if ( vmx_domain_enable_pml(d) )
>>          return;
>>  
>>      /* Enable EPT A/D bit for PML */
>>      p2m->ept.ad = 1;
>> -    vmx_domain_update_eptp(p2m->domain);
>> +
>> +    if ( altp2m_active(d) )
>> +        for ( i = 0; i < MAX_ALTP2M; i++ )
>> +        {
>> +            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
>> +                continue;
>> +
>> +            p2m = d->arch.altp2m_p2m[i];
>> +            p2m->ept.ad = 1;
>> +        }
> 
> You're not grabbing the respective p2m locks here -- I'm pretty sure
> this will end up being three separate instructions (read, set ad bit,
> write).
> 
> But there would something a bit funny here about grabbing the main p2m
> lock in p2m.c, and then grabbing altp2m locks within the function.  But
> on the other hand, you clearly only want to call this...
> 
>> +    vmx_domain_update_eptp(d);
> 
> ...once.  Some refactoring might be wanted.

I'll see about that as well.


Thanks,
Razvan

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

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

[Razvan Cojocaru]

> But actually the sequence I'm worried about is:
> 
> 1. Misconfigure hostp2m
> 2. Create altp2m
> 3. Enable altp2m
> 4. Switch to altp2m 1
> 5. A would-be-fault in the hostp2m no longer occurs
> 
> Please note the additional step 2. At this point, the hostp2m has been
> misconfigured, but the creation of altp2m came too late - so the
> misconfiguration of the hostp2m could not have propagated to altp2m 1,
> since it didn't yet exist when it was misconfigured.
> 
> Does not switching to altp2m 1 then lose the EPT misconfiguration?

Sorry, please switch steps 2 and 3 - clearly we need to first enable
altp2m to be able to create a new altp2m. :) After the first typo the
text became a bit distorted as well.


Thanks,
Razvan

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

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

[George Dunlap]

On 09/19/2018 02:01 PM, Razvan Cojocaru wrote:
> On 9/19/18 3:15 PM, George Dunlap wrote:
>> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.
> 
> No problem, thanks for the review!
> 
>>> We should discuss if just copying over
>>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>>   this code requires more synchronization).
>>
>> It's clearly not sufficient. :-)  The logdirty_ranges struct is
>> protected by the lock of the p2m structure that contains it; if you
>> point to it from a different p2m structure, then you'll have
>> inconsistent logging, and you'll have problems if one vcpu is reading
>> the structure while another is modifying it.
>>
>> Another issue is that it doesn't look like you're propagating updates to
>> this shared state either -- if someone enables or disables
>> global_logdirty, or changes default_access, the altp2ms will still have
>> the old value.
>>
>> I wonder if we should collect the various bits that need to be kept in
>> sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within
>> the p2m, and enforce using a function / macro to modify the values inside.
> 
> Right, I'll get on with the sync structure then.
> 
>>> Another aspect is that, while new modifications should work with
>>> these changes, _old_ modifications (written to the host2pm
>>> _before_ we've created the new altp2m) will, if I understand the
>>> code correctly be lost. That is to say, misconfigurations
>>> performed before p2m_init_altp2m_ept() in the hostp2m will
>>> presumably not trigger the necessary faults after switching to
>>> the new altp2m.
>>
>> You're worried about the following sequence?
>>
>> 1. Misconfigure hostp2m
>> 2. Enable altp2m
>> 3. Switch to altp2m 1
>> 4. Fault on a previously-misconfigured p2m entry
> 
> No, I'm worried that at step 4 the fault will no longer happen, because
> the EPTP index corresponding to altp2m 1 points to an EPT where the
> entries misconfigured in the hostp2m are _not_ misconfigured.
> 
> But actually the sequence I'm worried about is:
> 
> 1. Misconfigure hostp2m
> 2. Create altp2m
> 3. Enable altp2m
> 4. Switch to altp2m 1
> 5. A would-be-fault in the hostp2m no longer occurs

But how would a fault not occur?  The altp2m at this point won't have
*any* entries; any p2m access at all should fault, yes?  At which point
the altp2m code will say, "Oh, look, here's an entry I didn't have; I'll
copy it from the host p2m".  That will call hostp2m->get_entry(), which
will resolve the misconfig.

Or do I need to go back and review the altp2m code again so I have a
clue how it works?

 -George

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

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

[Razvan Cojocaru]

On 9/19/18 4:08 PM, George Dunlap wrote:
> On 09/19/2018 02:01 PM, Razvan Cojocaru wrote:
>> On 9/19/18 3:15 PM, George Dunlap wrote:
>>> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.
>>
>> No problem, thanks for the review!
>>
>>>> We should discuss if just copying over
>>>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>>>   this code requires more synchronization).
>>>
>>> It's clearly not sufficient. :-)  The logdirty_ranges struct is
>>> protected by the lock of the p2m structure that contains it; if you
>>> point to it from a different p2m structure, then you'll have
>>> inconsistent logging, and you'll have problems if one vcpu is reading
>>> the structure while another is modifying it.
>>>
>>> Another issue is that it doesn't look like you're propagating updates to
>>> this shared state either -- if someone enables or disables
>>> global_logdirty, or changes default_access, the altp2ms will still have
>>> the old value.
>>>
>>> I wonder if we should collect the various bits that need to be kept in
>>> sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within
>>> the p2m, and enforce using a function / macro to modify the values inside.
>>
>> Right, I'll get on with the sync structure then.
>>
>>>> Another aspect is that, while new modifications should work with
>>>> these changes, _old_ modifications (written to the host2pm
>>>> _before_ we've created the new altp2m) will, if I understand the
>>>> code correctly be lost. That is to say, misconfigurations
>>>> performed before p2m_init_altp2m_ept() in the hostp2m will
>>>> presumably not trigger the necessary faults after switching to
>>>> the new altp2m.
>>>
>>> You're worried about the following sequence?
>>>
>>> 1. Misconfigure hostp2m
>>> 2. Enable altp2m
>>> 3. Switch to altp2m 1
>>> 4. Fault on a previously-misconfigured p2m entry
>>
>> No, I'm worried that at step 4 the fault will no longer happen, because
>> the EPTP index corresponding to altp2m 1 points to an EPT where the
>> entries misconfigured in the hostp2m are _not_ misconfigured.
>>
>> But actually the sequence I'm worried about is:
>>
>> 1. Misconfigure hostp2m
>> 2. Create altp2m
>> 3. Enable altp2m
>> 4. Switch to altp2m 1
>> 5. A would-be-fault in the hostp2m no longer occurs
> 
> But how would a fault not occur?  The altp2m at this point won't have
> *any* entries; any p2m access at all should fault, yes?  At which point
> the altp2m code will say, "Oh, look, here's an entry I didn't have; I'll
> copy it from the host p2m".  That will call hostp2m->get_entry(), which
> will resolve the misconfig.
> 
> Or do I need to go back and review the altp2m code again so I have a
> clue how it works?

No no, I think you're right - I'd in any case bet on your knowledge of
this over mine, I was just trying to make sure I'm not losing sight of
anything.

I'll start working on the next version of the patch.


Thanks again,
Razvan

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

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

[Razvan Cojocaru]

On 9/19/18 3:15 PM, George Dunlap wrote:
>> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>>   logdirty_ranges, global_logdirty, ept.ad and default_access
>>   from the hostp2m (the latter more for completeness than for any
>>   other reason).
> I think this is probably the right approach.  These values change
> rarely, but after a misconfig are read repeatedly.  So it's probably a
> lot more efficient to propagate changes when they happen, rather than
> trying to keep a single master copy.  However...
> 
>> We should discuss if just copying over
>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>   this code requires more synchronization).
> It's clearly not sufficient. :-)  The logdirty_ranges struct is
> protected by the lock of the p2m structure that contains it; if you
> point to it from a different p2m structure, then you'll have
> inconsistent logging, and you'll have problems if one vcpu is reading
> the structure while another is modifying it.
> 
> Another issue is that it doesn't look like you're propagating updates to
> this shared state either -- if someone enables or disables
> global_logdirty, or changes default_access, the altp2ms will still have
> the old value.
> 
> I wonder if we should collect the various bits that need to be kept in
> sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within
> the p2m, and enforce using a function / macro to modify the values inside.

I'm not sure how to put ept.ad in the new sync sub-struct - putting the
full ept in there feels like overkill, and just putting a new variable
to symbolize .ad feels convoluted.


Thanks,
Razvan

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

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

[George Dunlap]

On 09/19/2018 03:09 PM, Razvan Cojocaru wrote:
> On 9/19/18 3:15 PM, George Dunlap wrote:
>>> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>>>   logdirty_ranges, global_logdirty, ept.ad and default_access
>>>   from the hostp2m (the latter more for completeness than for any
>>>   other reason).
>> I think this is probably the right approach.  These values change
>> rarely, but after a misconfig are read repeatedly.  So it's probably a
>> lot more efficient to propagate changes when they happen, rather than
>> trying to keep a single master copy.  However...
>>
>>> We should discuss if just copying over
>>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>>   this code requires more synchronization).
>> It's clearly not sufficient. :-)  The logdirty_ranges struct is
>> protected by the lock of the p2m structure that contains it; if you
>> point to it from a different p2m structure, then you'll have
>> inconsistent logging, and you'll have problems if one vcpu is reading
>> the structure while another is modifying it.
>>
>> Another issue is that it doesn't look like you're propagating updates to
>> this shared state either -- if someone enables or disables
>> global_logdirty, or changes default_access, the altp2ms will still have
>> the old value.
>>
>> I wonder if we should collect the various bits that need to be kept in
>> sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within
>> the p2m, and enforce using a function / macro to modify the values inside.
> 
> I'm not sure how to put ept.ad in the new sync sub-struct - putting the
> full ept in there feels like overkill, and just putting a new variable
> to symbolize .ad feels convoluted.

I'm pretty sure the ept there is something we need to pass to hardware
to implement the separate p2m tables.  That's definitely not shared. :-)

 -George

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

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

[Razvan Cojocaru]

On 9/19/18 3:44 PM, George Dunlap wrote:
> On 09/19/2018 01:15 PM, George Dunlap wrote:
>> On 09/03/2018 09:25 AM, Razvan Cojocaru wrote:
>>> When an new altp2m view is created very early on guest boot, the
>>> display will freeze (although the guest will run normally). This
>>> may also happen on resizing the display. The reason is the way
>>> Xen currently (mis)handles logdirty VGA: it intentionally
>>> misconfigures VGA pages so that they will fault.
>>>
>>> The problem is that it only does this in the host p2m. Once we
>>> switch to a new altp2m, the misconfigured entries will no longer
>>> fault, so the display will not be updated.
>>
>> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.
>>
>>> This patch:
>>>
>>> * updates ept_handle_misconfig() to use the active altp2m instead
>>>   of the hostp2m;
>>
>> This is probably necessary.
>>
>>> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>>>   logdirty_ranges, global_logdirty, ept.ad and default_access
>>>   from the hostp2m (the latter more for completeness than for any
>>>   other reason).
>>
>> I think this is probably the right approach.  These values change
>> rarely, but after a misconfig are read repeatedly.  So it's probably a
>> lot more efficient to propagate changes when they happen, rather than
>> trying to keep a single master copy.  However...
>>
>>> We should discuss if just copying over
>>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>>   this code requires more synchronization).
>>
>> It's clearly not sufficient. :-)  The logdirty_ranges struct is
>> protected by the lock of the p2m structure that contains it; if you
>> point to it from a different p2m structure, then you'll have
>> inconsistent logging, and you'll have problems if one vcpu is reading
>> the structure while another is modifying it.
> 
> ...and therefore, if we believe that it's more efficient to duplicate
> structures than to share it and use a lock, we need to do a deep copy of
> the data structure on altp2m creation, and propagate changes as we do
> for the other "synced" data.

The biggest problem here is p2m->logdirty_ranges. This patch will
(justly) not work, because struct rangeset is only forward-declared in
xen/rangeset.h, so an incomplete type here:

-void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
+int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
 {
     struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;

+    if ( !p2m->logdirty_ranges )
+        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
+                                            RANGESETF_prettyprint_hex);
+    if ( !p2m->logdirty_ranges )
+        return -ENOMEM;
+
+    *p2m->logdirty_ranges = *hostp2m->logdirty_ranges;
+
     p2m->ept.ad = hostp2m->ept.ad;
+    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->min_remapped_gfn = gfn_x(INVALID_GFN);
     p2m->max_remapped_gfn = 0;
     ept = &p2m->ept;
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
     d->arch.altp2m_eptp[i] = ept->eptp;
+
+    return 0;
+}

But that's not even the biggest problem: even if that would compile, it
would still be wrong, because logdirty_pages has pointers of its own,
which means that two bitwise-copied distinct rangesets can still point
to the same data and thus be vulnerable to race conditions and wanting
synchronization.

Furthermore there's no rangeset_copy() function in sight in rangeset.h
(though there is a rangeset_swap()).

Would you like me to add a rangeset_copy() function (presumably another
intermediary patch) and proceed in that manner?


Thanks,
Razvan

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

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

[Jan Beulich]

>>> On 04.10.18 at 16:56, <rcojocaru@bitdefender.com> wrote:
> The biggest problem here is p2m->logdirty_ranges. This patch will
> (justly) not work, because struct rangeset is only forward-declared in
> xen/rangeset.h, so an incomplete type here:
> 
> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> +int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>  {
>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>      struct ept_data *ept;
> 
> +    if ( !p2m->logdirty_ranges )
> +        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
> +                                            RANGESETF_prettyprint_hex);
> +    if ( !p2m->logdirty_ranges )
> +        return -ENOMEM;
> +
> +    *p2m->logdirty_ranges = *hostp2m->logdirty_ranges;
> +
>      p2m->ept.ad = hostp2m->ept.ad;
> +    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->min_remapped_gfn = gfn_x(INVALID_GFN);
>      p2m->max_remapped_gfn = 0;
>      ept = &p2m->ept;
>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>      d->arch.altp2m_eptp[i] = ept->eptp;
> +
> +    return 0;
> +}
> 
> But that's not even the biggest problem: even if that would compile, it
> would still be wrong, because logdirty_pages has pointers of its own,
> which means that two bitwise-copied distinct rangesets can still point
> to the same data and thus be vulnerable to race conditions and wanting
> synchronization.
> 
> Furthermore there's no rangeset_copy() function in sight in rangeset.h
> (though there is a rangeset_swap()).
> 
> Would you like me to add a rangeset_copy() function (presumably another
> intermediary patch) and proceed in that manner?

Roger recently has posted a patch adding rangeset_merge(), which I think
is more general than your rangeset_copy(). That said, I'm in no way
convinced copying (and then keeping in sync) the range sets across the
altp2m-s is the best approach. It may well be that the optimal solution is
somewhere in the middle between sharing everything and copying
everything.

Jan



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

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

[George Dunlap]

On 10/04/2018 04:20 PM, Jan Beulich wrote:
>>>> On 04.10.18 at 16:56, <rcojocaru@bitdefender.com> wrote:
>> The biggest problem here is p2m->logdirty_ranges. This patch will
>> (justly) not work, because struct rangeset is only forward-declared in
>> xen/rangeset.h, so an incomplete type here:
>>
>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>> +int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>  {
>>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>      struct ept_data *ept;
>>
>> +    if ( !p2m->logdirty_ranges )
>> +        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>> +                                            RANGESETF_prettyprint_hex);
>> +    if ( !p2m->logdirty_ranges )
>> +        return -ENOMEM;
>> +
>> +    *p2m->logdirty_ranges = *hostp2m->logdirty_ranges;
>> +
>>      p2m->ept.ad = hostp2m->ept.ad;
>> +    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->min_remapped_gfn = gfn_x(INVALID_GFN);
>>      p2m->max_remapped_gfn = 0;
>>      ept = &p2m->ept;
>>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>      d->arch.altp2m_eptp[i] = ept->eptp;
>> +
>> +    return 0;
>> +}
>>
>> But that's not even the biggest problem: even if that would compile, it
>> would still be wrong, because logdirty_pages has pointers of its own,
>> which means that two bitwise-copied distinct rangesets can still point
>> to the same data and thus be vulnerable to race conditions and wanting
>> synchronization.

Yes, so "deep copy" means if a structure has pointers, you copy the
structures pointed to; and if that structure has pointers, you copy
those, all the way down.  After a deep copy, any operations on the
structure should be operating on completely separate bits of memory and
pointers.

>> Furthermore there's no rangeset_copy() function in sight in rangeset.h
>> (though there is a rangeset_swap()).
>>
>> Would you like me to add a rangeset_copy() function (presumably another
>> intermediary patch) and proceed in that manner?
> 
> Roger recently has posted a patch adding rangeset_merge(), which I think
> is more general than your rangeset_copy(). That said, I'm in no way
> convinced copying (and then keeping in sync) the range sets across the
> altp2m-s is the best approach. It may well be that the optimal solution is
> somewhere in the middle between sharing everything and copying
> everything.

Er, you mean maybe we should share logdirty ranges and copy other
things?  Or do you actually mean somehow to share bits of the logdirty
range structure?

I think we can reasonably start with a simple-and-correct approach, and
try to come up with an optimization later if we decide it's necessary.
The two basic simple-but-correct approaches would be:

1. Share logdirty_ranges.  This would mean not duplicating the structure
and keeping it in sync; but it would mean grabbing the main p2m lock on
every resolv_misconfig().

2. Duplicate the structure and keep it in sync.  This  means not
grabbing the main p2m lock on every resolv_misconfig(); but it does mean
duplicating memory, as well as grabbing the lock of every altp2m
structure every time logdirty_ranges changes.

As I've said before, I think #2 is the most promising, since
resolv_misconfig will be called (potentially) for each entry in the p2m
table, but logdirty_ranges only changes once or twice during the entire
lifetime of the guest.

 -George

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

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

[Jan Beulich]

>>> On 04.10.18 at 17:34, <george.dunlap@citrix.com> wrote:
> On 10/04/2018 04:20 PM, Jan Beulich wrote:
>>>>> On 04.10.18 at 16:56, <rcojocaru@bitdefender.com> wrote:
>>> The biggest problem here is p2m->logdirty_ranges. This patch will
>>> (justly) not work, because struct rangeset is only forward-declared in
>>> xen/rangeset.h, so an incomplete type here:
>>>
>>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>> +int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>  {
>>>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>      struct ept_data *ept;
>>>
>>> +    if ( !p2m->logdirty_ranges )
>>> +        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>>> +                                            RANGESETF_prettyprint_hex);
>>> +    if ( !p2m->logdirty_ranges )
>>> +        return -ENOMEM;
>>> +
>>> +    *p2m->logdirty_ranges = *hostp2m->logdirty_ranges;
>>> +
>>>      p2m->ept.ad = hostp2m->ept.ad;
>>> +    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->min_remapped_gfn = gfn_x(INVALID_GFN);
>>>      p2m->max_remapped_gfn = 0;
>>>      ept = &p2m->ept;
>>>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>      d->arch.altp2m_eptp[i] = ept->eptp;
>>> +
>>> +    return 0;
>>> +}
>>>
>>> But that's not even the biggest problem: even if that would compile, it
>>> would still be wrong, because logdirty_pages has pointers of its own,
>>> which means that two bitwise-copied distinct rangesets can still point
>>> to the same data and thus be vulnerable to race conditions and wanting
>>> synchronization.
> 
> Yes, so "deep copy" means if a structure has pointers, you copy the
> structures pointed to; and if that structure has pointers, you copy
> those, all the way down.  After a deep copy, any operations on the
> structure should be operating on completely separate bits of memory and
> pointers.
> 
>>> Furthermore there's no rangeset_copy() function in sight in rangeset.h
>>> (though there is a rangeset_swap()).
>>>
>>> Would you like me to add a rangeset_copy() function (presumably another
>>> intermediary patch) and proceed in that manner?
>> 
>> Roger recently has posted a patch adding rangeset_merge(), which I think
>> is more general than your rangeset_copy(). That said, I'm in no way
>> convinced copying (and then keeping in sync) the range sets across the
>> altp2m-s is the best approach. It may well be that the optimal solution is
>> somewhere in the middle between sharing everything and copying
>> everything.
> 
> Er, you mean maybe we should share logdirty ranges and copy other
> things?  Or do you actually mean somehow to share bits of the logdirty
> range structure?

The former, of course. I'm sorry for the ambiguity.

> I think we can reasonably start with a simple-and-correct approach, and
> try to come up with an optimization later if we decide it's necessary.
> The two basic simple-but-correct approaches would be:
> 
> 1. Share logdirty_ranges.  This would mean not duplicating the structure
> and keeping it in sync; but it would mean grabbing the main p2m lock on
> every resolv_misconfig().
> 
> 2. Duplicate the structure and keep it in sync.  This  means not
> grabbing the main p2m lock on every resolv_misconfig(); but it does mean
> duplicating memory, as well as grabbing the lock of every altp2m
> structure every time logdirty_ranges changes.
> 
> As I've said before, I think #2 is the most promising, since
> resolv_misconfig will be called (potentially) for each entry in the p2m
> table, but logdirty_ranges only changes once or twice during the entire
> lifetime of the guest.

So perhaps some r/w lock wants to be introduced?

Jan



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

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

[George Dunlap]

On 10/04/2018 04:45 PM, Jan Beulich wrote:
>>>> On 04.10.18 at 17:34, <george.dunlap@citrix.com> wrote:
>> On 10/04/2018 04:20 PM, Jan Beulich wrote:
>>>>>> On 04.10.18 at 16:56, <rcojocaru@bitdefender.com> wrote:
>>>> The biggest problem here is p2m->logdirty_ranges. This patch will
>>>> (justly) not work, because struct rangeset is only forward-declared in
>>>> xen/rangeset.h, so an incomplete type here:
>>>>
>>>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>> +int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>  {
>>>>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>      struct ept_data *ept;
>>>>
>>>> +    if ( !p2m->logdirty_ranges )
>>>> +        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>>>> +                                            RANGESETF_prettyprint_hex);
>>>> +    if ( !p2m->logdirty_ranges )
>>>> +        return -ENOMEM;
>>>> +
>>>> +    *p2m->logdirty_ranges = *hostp2m->logdirty_ranges;
>>>> +
>>>>      p2m->ept.ad = hostp2m->ept.ad;
>>>> +    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->min_remapped_gfn = gfn_x(INVALID_GFN);
>>>>      p2m->max_remapped_gfn = 0;
>>>>      ept = &p2m->ept;
>>>>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>>      d->arch.altp2m_eptp[i] = ept->eptp;
>>>> +
>>>> +    return 0;
>>>> +}
>>>>
>>>> But that's not even the biggest problem: even if that would compile, it
>>>> would still be wrong, because logdirty_pages has pointers of its own,
>>>> which means that two bitwise-copied distinct rangesets can still point
>>>> to the same data and thus be vulnerable to race conditions and wanting
>>>> synchronization.
>>
>> Yes, so "deep copy" means if a structure has pointers, you copy the
>> structures pointed to; and if that structure has pointers, you copy
>> those, all the way down.  After a deep copy, any operations on the
>> structure should be operating on completely separate bits of memory and
>> pointers.
>>
>>>> Furthermore there's no rangeset_copy() function in sight in rangeset.h
>>>> (though there is a rangeset_swap()).
>>>>
>>>> Would you like me to add a rangeset_copy() function (presumably another
>>>> intermediary patch) and proceed in that manner?
>>>
>>> Roger recently has posted a patch adding rangeset_merge(), which I think
>>> is more general than your rangeset_copy(). That said, I'm in no way
>>> convinced copying (and then keeping in sync) the range sets across the
>>> altp2m-s is the best approach. It may well be that the optimal solution is
>>> somewhere in the middle between sharing everything and copying
>>> everything.
>>
>> Er, you mean maybe we should share logdirty ranges and copy other
>> things?  Or do you actually mean somehow to share bits of the logdirty
>> range structure?
> 
> The former, of course. I'm sorry for the ambiguity.
> 
>> I think we can reasonably start with a simple-and-correct approach, and
>> try to come up with an optimization later if we decide it's necessary.
>> The two basic simple-but-correct approaches would be:
>>
>> 1. Share logdirty_ranges.  This would mean not duplicating the structure
>> and keeping it in sync; but it would mean grabbing the main p2m lock on
>> every resolv_misconfig().
>>
>> 2. Duplicate the structure and keep it in sync.  This  means not
>> grabbing the main p2m lock on every resolv_misconfig(); but it does mean
>> duplicating memory, as well as grabbing the lock of every altp2m
>> structure every time logdirty_ranges changes.
>>
>> As I've said before, I think #2 is the most promising, since
>> resolv_misconfig will be called (potentially) for each entry in the p2m
>> table, but logdirty_ranges only changes once or twice during the entire
>> lifetime of the guest.
> 
> So perhaps some r/w lock wants to be introduced?

There will also be locking order issues to consider if we do that.

What's your main reason for not wanting #2?

 -George

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

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

[Jan Beulich]

>>> On 04.10.18 at 17:52, <george.dunlap@citrix.com> wrote:
> On 10/04/2018 04:45 PM, Jan Beulich wrote:
>>>>> On 04.10.18 at 17:34, <george.dunlap@citrix.com> wrote:
>>> On 10/04/2018 04:20 PM, Jan Beulich wrote:
>>>>>>> On 04.10.18 at 16:56, <rcojocaru@bitdefender.com> wrote:
>>>>> The biggest problem here is p2m->logdirty_ranges. This patch will
>>>>> (justly) not work, because struct rangeset is only forward-declared in
>>>>> xen/rangeset.h, so an incomplete type here:
>>>>>
>>>>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>> +int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>>  {
>>>>>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>>>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>>      struct ept_data *ept;
>>>>>
>>>>> +    if ( !p2m->logdirty_ranges )
>>>>> +        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>>>>> +                                            RANGESETF_prettyprint_hex);
>>>>> +    if ( !p2m->logdirty_ranges )
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    *p2m->logdirty_ranges = *hostp2m->logdirty_ranges;
>>>>> +
>>>>>      p2m->ept.ad = hostp2m->ept.ad;
>>>>> +    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->min_remapped_gfn = gfn_x(INVALID_GFN);
>>>>>      p2m->max_remapped_gfn = 0;
>>>>>      ept = &p2m->ept;
>>>>>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>>>      d->arch.altp2m_eptp[i] = ept->eptp;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>>
>>>>> But that's not even the biggest problem: even if that would compile, it
>>>>> would still be wrong, because logdirty_pages has pointers of its own,
>>>>> which means that two bitwise-copied distinct rangesets can still point
>>>>> to the same data and thus be vulnerable to race conditions and wanting
>>>>> synchronization.
>>>
>>> Yes, so "deep copy" means if a structure has pointers, you copy the
>>> structures pointed to; and if that structure has pointers, you copy
>>> those, all the way down.  After a deep copy, any operations on the
>>> structure should be operating on completely separate bits of memory and
>>> pointers.
>>>
>>>>> Furthermore there's no rangeset_copy() function in sight in rangeset.h
>>>>> (though there is a rangeset_swap()).
>>>>>
>>>>> Would you like me to add a rangeset_copy() function (presumably another
>>>>> intermediary patch) and proceed in that manner?
>>>>
>>>> Roger recently has posted a patch adding rangeset_merge(), which I think
>>>> is more general than your rangeset_copy(). That said, I'm in no way
>>>> convinced copying (and then keeping in sync) the range sets across the
>>>> altp2m-s is the best approach. It may well be that the optimal solution is
>>>> somewhere in the middle between sharing everything and copying
>>>> everything.
>>>
>>> Er, you mean maybe we should share logdirty ranges and copy other
>>> things?  Or do you actually mean somehow to share bits of the logdirty
>>> range structure?
>> 
>> The former, of course. I'm sorry for the ambiguity.
>> 
>>> I think we can reasonably start with a simple-and-correct approach, and
>>> try to come up with an optimization later if we decide it's necessary.
>>> The two basic simple-but-correct approaches would be:
>>>
>>> 1. Share logdirty_ranges.  This would mean not duplicating the structure
>>> and keeping it in sync; but it would mean grabbing the main p2m lock on
>>> every resolv_misconfig().
>>>
>>> 2. Duplicate the structure and keep it in sync.  This  means not
>>> grabbing the main p2m lock on every resolv_misconfig(); but it does mean
>>> duplicating memory, as well as grabbing the lock of every altp2m
>>> structure every time logdirty_ranges changes.
>>>
>>> As I've said before, I think #2 is the most promising, since
>>> resolv_misconfig will be called (potentially) for each entry in the p2m
>>> table, but logdirty_ranges only changes once or twice during the entire
>>> lifetime of the guest.
>> 
>> So perhaps some r/w lock wants to be introduced?
> 
> There will also be locking order issues to consider if we do that.
> 
> What's your main reason for not wanting #2?

Duplicating and keeping in sync data for which no separate allocations
are needed seems fine to me. Duplicating allocations, otoh, seems not
only a waste of resources to me, but I also expect error handling to
become ugly: You'll need to undo all prior syncing if e.g. the allocation
for altp2m #5 fails.

Jan


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

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

[Razvan Cojocaru]

On 10/4/18 6:20 PM, Jan Beulich wrote:
> Roger recently has posted a patch adding rangeset_merge(), which I think
> is more general than your rangeset_copy(). That said, I'm in no way
> convinced copying (and then keeping in sync) the range sets across the
> altp2m-s is the best approach. It may well be that the optimal solution is
> somewhere in the middle between sharing everything and copying
> everything.

Would it be possible to get Roger's patch into staging?

https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg01331.html

It looks simple enough and it has already acked by Wei.


Thanks,
Razvan

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

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

[Andrew Cooper]

On 17/10/18 14:26, Razvan Cojocaru wrote:
> On 10/4/18 6:20 PM, Jan Beulich wrote:
>> Roger recently has posted a patch adding rangeset_merge(), which I think
>> is more general than your rangeset_copy(). That said, I'm in no way
>> convinced copying (and then keeping in sync) the range sets across the
>> altp2m-s is the best approach. It may well be that the optimal solution is
>> somewhere in the middle between sharing everything and copying
>> everything.
> Would it be possible to get Roger's patch into staging?
>
> https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg01331.html
>
> It looks simple enough and it has already acked by Wei.

Looks good enough to me.  Pushed.

~Andrew

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

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

[Razvan Cojocaru]

On 10/17/18 4:30 PM, Andrew Cooper wrote:
> On 17/10/18 14:26, Razvan Cojocaru wrote:
>> On 10/4/18 6:20 PM, Jan Beulich wrote:
>>> Roger recently has posted a patch adding rangeset_merge(), which I think
>>> is more general than your rangeset_copy(). That said, I'm in no way
>>> convinced copying (and then keeping in sync) the range sets across the
>>> altp2m-s is the best approach. It may well be that the optimal solution is
>>> somewhere in the middle between sharing everything and copying
>>> everything.
>> Would it be possible to get Roger's patch into staging?
>>
>> https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg01331.html
>>
>> It looks simple enough and it has already acked by Wei.
> 
> Looks good enough to me.  Pushed.

Thanks!

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

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

[Jan Beulich]

>>> On 17.10.18 at 15:30, <andrew.cooper3@citrix.com> wrote:
> On 17/10/18 14:26, Razvan Cojocaru wrote:
>> On 10/4/18 6:20 PM, Jan Beulich wrote:
>>> Roger recently has posted a patch adding rangeset_merge(), which I think
>>> is more general than your rangeset_copy(). That said, I'm in no way
>>> convinced copying (and then keeping in sync) the range sets across the
>>> altp2m-s is the best approach. It may well be that the optimal solution is
>>> somewhere in the middle between sharing everything and copying
>>> everything.
>> Would it be possible to get Roger's patch into staging?
>>
>> https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg01331.html 
>>
>> It looks simple enough and it has already acked by Wei.
> 
> Looks good enough to me.  Pushed.

Well, the reason I didn't want to commit it on its own is that now
we have it in the tree as dead code. Any change of yours could
have included Roger's patch (until it might have gone in from his
series).

Jan



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