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

[George Dunlap <george.dunlap@citrix.com>]

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