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

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

On 05/02/2018 09:17 AM, 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.
> 
> 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.

No, obviously having everything in a single place makes more sense.
There's just the little issue of locking. :-)  But we should be able to
sort something out.  It would be nice if we could do something like have
the data covered by the hostp2m lock, so that guests not using the
altp2m functionality wouldn't need to do any extra locking.  But I'd
have to wrap my head around the p2m locking discipline again to know
whether that would work or not.

 -George

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