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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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.


Thanks,
Razvan

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