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 <[email protected]> >>>>>> Suggested-by: George Dunlap <[email protected]> >>>>> >>>>> 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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
