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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel