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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
>>>>> Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel