[PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[Razvan Cojocaru]

p2m_change_type_range() handles end > max_mapped_pfn, but not
start > max_mapped_pfn. Check the latter just after grabbing the
lock and bail if true.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Suggested-by: George Dunlap <george.dunlap@citrix.com>

---
Changes since V1:
 - Added ASSERT()s.
 - Wrapped the new condition in an unlikely().
---
 xen/arch/x86/mm/p2m.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c53cab4..e09b256 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -978,8 +978,19 @@ void p2m_change_type_range(struct domain *d,
     p2m_lock(p2m);
     p2m->defer_nested_flush = 1;
 
+    ASSERT(start < end);
+
+    if ( unlikely(start > p2m->max_mapped_pfn) )
+    {
+        ASSERT(!p2m_is_hostp2m(p2m));
+        p2m_unlock(p2m);
+        return;
+    }
+
     if ( unlikely(end > p2m->max_mapped_pfn) )
     {
+        ASSERT(end == ~0UL || !p2m_is_hostp2m(p2m));
+
         if ( !gfn )
         {
             p2m->change_entry_type_global(p2m, ot, nt);
-- 
2.7.4


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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[Razvan Cojocaru]

On 04/18/2018 04:12 PM, Razvan Cojocaru wrote:
> p2m_change_type_range() handles end > max_mapped_pfn, but not
> start > max_mapped_pfn. Check the latter just after grabbing the
> lock and bail if true.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
> ---
> Changes since V1:
>  - Added ASSERT()s.
>  - Wrapped the new condition in an unlikely().
> ---
>  xen/arch/x86/mm/p2m.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index c53cab4..e09b256 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -978,8 +978,19 @@ void p2m_change_type_range(struct domain *d,
>      p2m_lock(p2m);
>      p2m->defer_nested_flush = 1;
>  
> +    ASSERT(start < end);
> +
> +    if ( unlikely(start > p2m->max_mapped_pfn) )
> +    {
> +        ASSERT(!p2m_is_hostp2m(p2m));
> +        p2m_unlock(p2m);
> +        return;
> +    }
> +
>      if ( unlikely(end > p2m->max_mapped_pfn) )
>      {
> +        ASSERT(end == ~0UL || !p2m_is_hostp2m(p2m));
> +
>          if ( !gfn )
>          {
>              p2m->change_entry_type_global(p2m, ot, nt);
> 

I think I've addressed George's comments on the altp2m VGA logdirty
issue and I'm ready to send the current version of the patch, however it
depends on this one.

Is the above patch uncontroversial enough to find its way into staging
soon (in which case I should probably wait until it's in to send out the
other patch), or should I perhaps create a series of two patches and
send that out? Or is it maybe customary to proceed somehow else in a
situation like this?


Thank you,
Razvan

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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[Jan Beulich]

>>> On 23.04.18 at 09:23, <rcojocaru@bitdefender.com> wrote:
> On 04/18/2018 04:12 PM, Razvan Cojocaru wrote:
>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>> start > max_mapped_pfn. Check the latter just after grabbing the
>> lock and bail if true.
>> 
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>> 
>> ---
>> Changes since V1:
>>  - Added ASSERT()s.
>>  - Wrapped the new condition in an unlikely().
>> ---
>>  xen/arch/x86/mm/p2m.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index c53cab4..e09b256 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -978,8 +978,19 @@ void p2m_change_type_range(struct domain *d,
>>      p2m_lock(p2m);
>>      p2m->defer_nested_flush = 1;
>>  
>> +    ASSERT(start < end);
>> +
>> +    if ( unlikely(start > p2m->max_mapped_pfn) )
>> +    {
>> +        ASSERT(!p2m_is_hostp2m(p2m));
>> +        p2m_unlock(p2m);
>> +        return;
>> +    }
>> +
>>      if ( unlikely(end > p2m->max_mapped_pfn) )
>>      {
>> +        ASSERT(end == ~0UL || !p2m_is_hostp2m(p2m));
>> +
>>          if ( !gfn )
>>          {
>>              p2m->change_entry_type_global(p2m, ot, nt);
>> 
> 
> I think I've addressed George's comments on the altp2m VGA logdirty
> issue and I'm ready to send the current version of the patch, however it
> depends on this one.
> 
> Is the above patch uncontroversial enough to find its way into staging
> soon (in which case I should probably wait until it's in to send out the
> other patch), or should I perhaps create a series of two patches and
> send that out?

Well, you'll need George's ack and (for the tree being frozen) you'd need
to justify why this is important to fix for 4.11 (to convince Jürgen to give
a release ack).

> Or is it maybe customary to proceed somehow else in a situation like this?

A series is one way. Adding a post-commit-message note to clarify the
dependency (textual and/or functional) is another.

Jan


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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[George Dunlap]

On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
> p2m_change_type_range() handles end > max_mapped_pfn, but not
> start > max_mapped_pfn. Check the latter just after grabbing the
> lock and bail if true.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Suggested-by: George Dunlap <george.dunlap@citrix.com>

Sorry, I meant to reply to this earlier but I haven't been able to make
the time.

On reflection, I think this is the wrong approach actually.  First, my
assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
max_mapped_gfn shouldn't cause 'unnecessary' propagations.

Secondly, we do actually need to keep the logdirty ranges of all the
p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
could have the following situation:
* altp2m created, max_remapped_gfn 0x1000
* screen resized, logdirty range [0x2000-0x3000]; change dropped
* guest accesses 0x4000, max_remapped_gfn set to 0x4000
* change_p2m_type happens, and the 0x2000-0x3000 range is not marked
logrdirty #

So while it would be an improvement to make the assertion more explicit,
I don't (anymore) think it would actually be an improvement to discard
changes that are above max_mapped_gfn.  (And thus your original patch,
which copied max_mapped_gfn into the altp2ms, was probably closer to the
right approach).

Sorry for the confusion -- we obviously need a bit more thought about
how altp2m and logdirty interact.

 -George

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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[Razvan Cojocaru]

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

On 04/23/2018 02:47 PM, George Dunlap wrote:
> On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>> start > max_mapped_pfn. Check the latter just after grabbing the
>> lock and bail if true.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
> Sorry, I meant to reply to this earlier but I haven't been able to make
> the time.
> 
> On reflection, I think this is the wrong approach actually.  First, my
> assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
> p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
> max_mapped_gfn shouldn't cause 'unnecessary' propagations.
> 
> Secondly, we do actually need to keep the logdirty ranges of all the
> p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
> could have the following situation:
> * altp2m created, max_remapped_gfn 0x1000
> * screen resized, logdirty range [0x2000-0x3000]; change dropped
> * guest accesses 0x4000, max_remapped_gfn set to 0x4000
> * change_p2m_type happens, and the 0x2000-0x3000 range is not marked
> logrdirty #
> 
> So while it would be an improvement to make the assertion more explicit,
> I don't (anymore) think it would actually be an improvement to discard
> changes that are above max_mapped_gfn.  (And thus your original patch,
> which copied max_mapped_gfn into the altp2ms, was probably closer to the
> right approach).
> 
> Sorry for the confusion -- we obviously need a bit more thought about
> how altp2m and logdirty interact.

Thanks for the reply! Fair enough.

FWIW, the attached patch works well for me, resizes and all (but it
could very well be just luck).


Thanks,
Razvan

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

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 14b5939..b1df904 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,8 +1407,16 @@ 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;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c53cab4..c8f4d8f 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>
@@ -248,7 +249,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;
@@ -257,11 +257,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));
 
@@ -271,6 +269,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);
@@ -964,12 +978,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);
@@ -1022,6 +1036,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

[-- 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 V2] x86/p2m: fixed p2m_change_type_range() start / end check

[George Dunlap]

On 04/23/2018 12:56 PM, Razvan Cojocaru wrote:
> On 04/23/2018 02:47 PM, George Dunlap wrote:
>> On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
>>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>>> start > max_mapped_pfn. Check the latter just after grabbing the
>>> lock and bail if true.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>>
>> Sorry, I meant to reply to this earlier but I haven't been able to make
>> the time.
>>
>> On reflection, I think this is the wrong approach actually.  First, my
>> assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
>> p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
>> max_mapped_gfn shouldn't cause 'unnecessary' propagations.
>>
>> Secondly, we do actually need to keep the logdirty ranges of all the
>> p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
>> could have the following situation:
>> * altp2m created, max_remapped_gfn 0x1000
>> * screen resized, logdirty range [0x2000-0x3000]; change dropped
>> * guest accesses 0x4000, max_remapped_gfn set to 0x4000
>> * change_p2m_type happens, and the 0x2000-0x3000 range is not marked
>> logrdirty #
>>
>> So while it would be an improvement to make the assertion more explicit,
>> I don't (anymore) think it would actually be an improvement to discard
>> changes that are above max_mapped_gfn.  (And thus your original patch,
>> which copied max_mapped_gfn into the altp2ms, was probably closer to the
>> right approach).
>>
>> Sorry for the confusion -- we obviously need a bit more thought about
>> how altp2m and logdirty interact.
> 
> Thanks for the reply! Fair enough.
> 
> FWIW, the attached patch works well for me, resizes and all (but it
> could very well be just luck).

I think we really want some sort of analysis of all the ways the two
features might interact, and some justification as to why a solution is
complete.

You're not aiming to get a patch like this into 4.11 though, are you?

 -George

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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[Razvan Cojocaru]

On 04/23/2018 05:28 PM, George Dunlap wrote:
> On 04/23/2018 12:56 PM, Razvan Cojocaru wrote:
>> On 04/23/2018 02:47 PM, George Dunlap wrote:
>>> On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
>>>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>>>> start > max_mapped_pfn. Check the latter just after grabbing the
>>>> lock and bail if true.
>>>>
>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>>>
>>> Sorry, I meant to reply to this earlier but I haven't been able to make
>>> the time.
>>>
>>> On reflection, I think this is the wrong approach actually.  First, my
>>> assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
>>> p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
>>> max_mapped_gfn shouldn't cause 'unnecessary' propagations.
>>>
>>> Secondly, we do actually need to keep the logdirty ranges of all the
>>> p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
>>> could have the following situation:
>>> * altp2m created, max_remapped_gfn 0x1000
>>> * screen resized, logdirty range [0x2000-0x3000]; change dropped
>>> * guest accesses 0x4000, max_remapped_gfn set to 0x4000
>>> * change_p2m_type happens, and the 0x2000-0x3000 range is not marked
>>> logrdirty #
>>>
>>> So while it would be an improvement to make the assertion more explicit,
>>> I don't (anymore) think it would actually be an improvement to discard
>>> changes that are above max_mapped_gfn.  (And thus your original patch,
>>> which copied max_mapped_gfn into the altp2ms, was probably closer to the
>>> right approach).
>>>
>>> Sorry for the confusion -- we obviously need a bit more thought about
>>> how altp2m and logdirty interact.
>>
>> Thanks for the reply! Fair enough.
>>
>> FWIW, the attached patch works well for me, resizes and all (but it
>> could very well be just luck).
> 
> I think we really want some sort of analysis of all the ways the two
> features might interact, and some justification as to why a solution is
> complete.
> 
> You're not aiming to get a patch like this into 4.11 though, are you?

No (although it would have been nice if possible). A good solution to
the problem is the goal here, 4.11 or not. Nobody wants a rushed hack.

Thanks for all the help so far, and please let me know if you have any
suggestions I should try out.


Thanks,
Razvan

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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[Razvan Cojocaru]

On 04/23/2018 02:47 PM, George Dunlap wrote:
> On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>> start > max_mapped_pfn. Check the latter just after grabbing the
>> lock and bail if true.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
> Sorry, I meant to reply to this earlier but I haven't been able to make
> the time.
> 
> On reflection, I think this is the wrong approach actually.  First, my
> assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
> p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
> max_mapped_gfn shouldn't cause 'unnecessary' propagations.
> 
> Secondly, we do actually need to keep the logdirty ranges of all the
> p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
> could have the following situation:
> * altp2m created, max_remapped_gfn 0x1000
> * screen resized, logdirty range [0x2000-0x3000]; change dropped
> * guest accesses 0x4000, max_remapped_gfn set to 0x4000
> * change_p2m_type happens, and the 0x2000-0x3000 range is not marked
> logrdirty #
> 
> So while it would be an improvement to make the assertion more explicit,
> I don't (anymore) think it would actually be an improvement to discard
> changes that are above max_mapped_gfn.  (And thus your original patch,
> which copied max_mapped_gfn into the altp2ms, was probably closer to the
> right approach).
> 
> Sorry for the confusion -- we obviously need a bit more thought about
> how altp2m and logdirty interact.

Re-reading this, again the simple solution to me implies having all this
bookkeeping information in a struct, and have all p2ms share a poninter
to it. That way, even code that uses the wrong p2m still updates the
correct logdirty data.

That would also simplify all the copying of supposed-to-be-kept-in-sync
data on switches.

But of course you're the maintainer (and more to the point the most
knowledgeable about the code), so I'm quite probably missing something.


Thanks,
Razvan

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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[George Dunlap]

On 05/02/2018 09:17 AM, Razvan Cojocaru wrote:
> On 04/23/2018 02:47 PM, George Dunlap wrote:
>> On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
>>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>>> start > max_mapped_pfn. Check the latter just after grabbing the
>>> lock and bail if true.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>>
>> Sorry, I meant to reply to this earlier but I haven't been able to make
>> the time.
>>
>> On reflection, I think this is the wrong approach actually.  First, my
>> assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
>> p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
>> max_mapped_gfn shouldn't cause 'unnecessary' propagations.
>>
>> Secondly, we do actually need to keep the logdirty ranges of all the
>> p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
>> could have the following situation:
>> * altp2m created, max_remapped_gfn 0x1000
>> * screen resized, logdirty range [0x2000-0x3000]; change dropped
>> * guest accesses 0x4000, max_remapped_gfn set to 0x4000
>> * change_p2m_type happens, and the 0x2000-0x3000 range is not marked
>> logrdirty #
>>
>> So while it would be an improvement to make the assertion more explicit,
>> I don't (anymore) think it would actually be an improvement to discard
>> changes that are above max_mapped_gfn.  (And thus your original patch,
>> which copied max_mapped_gfn into the altp2ms, was probably closer to the
>> right approach).
>>
>> Sorry for the confusion -- we obviously need a bit more thought about
>> how altp2m and logdirty interact.
> 
> Re-reading this, again the simple solution to me implies having all this
> bookkeeping information in a struct, and have all p2ms share a poninter
> to it. That way, even code that uses the wrong p2m still updates the
> correct logdirty data.
> 
> That would also simplify all the copying of supposed-to-be-kept-in-sync
> data on switches.

No, obviously having everything in a single place makes more sense.
There's just the little issue of locking. :-)  But we should be able to
sort something out.  It would be nice if we could do something like have
the data covered by the hostp2m lock, so that guests not using the
altp2m functionality wouldn't need to do any extra locking.  But I'd
have to wrap my head around the p2m locking discipline again to know
whether that would work or not.

 -George

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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[Razvan Cojocaru]

On 04/23/2018 05:33 PM, Razvan Cojocaru wrote:
> On 04/23/2018 05:28 PM, George Dunlap wrote:
>> On 04/23/2018 12:56 PM, Razvan Cojocaru wrote:
>>> On 04/23/2018 02:47 PM, George Dunlap wrote:
>>>> On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
>>>>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>>>>> start > max_mapped_pfn. Check the latter just after grabbing the
>>>>> lock and bail if true.
>>>>>
>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>>>>
>>>> Sorry, I meant to reply to this earlier but I haven't been able to make
>>>> the time.
>>>>
>>>> On reflection, I think this is the wrong approach actually.  First, my
>>>> assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
>>>> p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
>>>> max_mapped_gfn shouldn't cause 'unnecessary' propagations.
>>>>
>>>> Secondly, we do actually need to keep the logdirty ranges of all the
>>>> p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
>>>> could have the following situation:
>>>> * altp2m created, max_remapped_gfn 0x1000
>>>> * screen resized, logdirty range [0x2000-0x3000]; change dropped
>>>> * guest accesses 0x4000, max_remapped_gfn set to 0x4000
>>>> * change_p2m_type happens, and the 0x2000-0x3000 range is not marked
>>>> logrdirty #
>>>>
>>>> So while it would be an improvement to make the assertion more explicit,
>>>> I don't (anymore) think it would actually be an improvement to discard
>>>> changes that are above max_mapped_gfn.  (And thus your original patch,
>>>> which copied max_mapped_gfn into the altp2ms, was probably closer to the
>>>> right approach).
>>>>
>>>> Sorry for the confusion -- we obviously need a bit more thought about
>>>> how altp2m and logdirty interact.
>>>
>>> Thanks for the reply! Fair enough.
>>>
>>> FWIW, the attached patch works well for me, resizes and all (but it
>>> could very well be just luck).
>>
>> I think we really want some sort of analysis of all the ways the two
>> features might interact, and some justification as to why a solution is
>> complete.
>>
>> You're not aiming to get a patch like this into 4.11 though, are you?
> 
> No (although it would have been nice if possible). A good solution to
> the problem is the goal here, 4.11 or not. Nobody wants a rushed hack.
> 
> Thanks for all the help so far, and please let me know if you have any
> suggestions I should try out.

George, would this be a better time to try to thoroughly fix this? It's
clearly a major obstacle in being able to use altp2m. I've done more
tests since we've last discussed this on xen-devel, and I did see a
frozen rectangle of pixels quite a while after booting (during "normal"
Windows operation), so the patch I've attached last time does indeed
seem to be incomplete somewhere. But I haven't managed to reproduce it
since, so it's still quite unclear what corner case I've hit.

I was wondering if you have any suggestions on how to proceed in fixing
this for good upstream (I certainly don't have your expertise in the p2m
code).

Thanks!

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

Re: [PATCH V2] x86/p2m: fixed p2m_change_type_range() start / end check

[George Dunlap]

On 07/09/2018 09:31 AM, Razvan Cojocaru wrote:
> On 04/23/2018 05:33 PM, Razvan Cojocaru wrote:
>> On 04/23/2018 05:28 PM, George Dunlap wrote:
>>> On 04/23/2018 12:56 PM, Razvan Cojocaru wrote:
>>>> On 04/23/2018 02:47 PM, George Dunlap wrote:
>>>>> On 04/18/2018 02:12 PM, Razvan Cojocaru wrote:
>>>>>> p2m_change_type_range() handles end > max_mapped_pfn, but not
>>>>>> start > max_mapped_pfn. Check the latter just after grabbing the
>>>>>> lock and bail if true.
>>>>>>
>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
>>>>>
>>>>> Sorry, I meant to reply to this earlier but I haven't been able to make
>>>>> the time.
>>>>>
>>>>> On reflection, I think this is the wrong approach actually.  First, my
>>>>> assertion was incorrect: the p2m_altp2m_propagate_change() is gated on
>>>>> p2m->max_remapped_gfn, not max_mapped_gfn (nb the 're').  So setting
>>>>> max_mapped_gfn shouldn't cause 'unnecessary' propagations.
>>>>>
>>>>> Secondly, we do actually need to keep the logdirty ranges of all the
>>>>> p2ms in sync, even if they're past the max_remapped_gfn.  Otherwise we
>>>>> could have the following situation:
>>>>> * altp2m created, max_remapped_gfn 0x1000
>>>>> * screen resized, logdirty range [0x2000-0x3000]; change dropped
>>>>> * guest accesses 0x4000, max_remapped_gfn set to 0x4000
>>>>> * change_p2m_type happens, and the 0x2000-0x3000 range is not marked
>>>>> logrdirty #
>>>>>
>>>>> So while it would be an improvement to make the assertion more explicit,
>>>>> I don't (anymore) think it would actually be an improvement to discard
>>>>> changes that are above max_mapped_gfn.  (And thus your original patch,
>>>>> which copied max_mapped_gfn into the altp2ms, was probably closer to the
>>>>> right approach).
>>>>>
>>>>> Sorry for the confusion -- we obviously need a bit more thought about
>>>>> how altp2m and logdirty interact.
>>>>
>>>> Thanks for the reply! Fair enough.
>>>>
>>>> FWIW, the attached patch works well for me, resizes and all (but it
>>>> could very well be just luck).
>>>
>>> I think we really want some sort of analysis of all the ways the two
>>> features might interact, and some justification as to why a solution is
>>> complete.
>>>
>>> You're not aiming to get a patch like this into 4.11 though, are you?
>>
>> No (although it would have been nice if possible). A good solution to
>> the problem is the goal here, 4.11 or not. Nobody wants a rushed hack.
>>
>> Thanks for all the help so far, and please let me know if you have any
>> suggestions I should try out.
> 
> George, would this be a better time to try to thoroughly fix this? It's
> clearly a major obstacle in being able to use altp2m. I've done more
> tests since we've last discussed this on xen-devel, and I did see a
> frozen rectangle of pixels quite a while after booting (during "normal"
> Windows operation), so the patch I've attached last time does indeed
> seem to be incomplete somewhere. But I haven't managed to reproduce it
> since, so it's still quite unclear what corner case I've hit.
> 
> I was wondering if you have any suggestions on how to proceed in fixing
> this for good upstream (I certainly don't have your expertise in the p2m
> code).
> 
> Thanks!

Yes, let me see if I can carve out some time to take a look at this.

 -George

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