Re: Weird altp2m behaviour when switching early to a new view

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 04/18/2018 01:45 PM, George Dunlap wrote:
> On 04/18/2018 09:20 AM, Razvan Cojocaru wrote:
>> On 04/17/2018 04:53 PM, George Dunlap wrote:
>>> On 04/17/2018 11:50 AM, Razvan Cojocaru wrote:
>>>>  void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>>>  {
>>>>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>>> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>      struct ept_data *ept;
>>>>  
>>>> +    p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
>>>> +    p2m->default_access = hostp2m->default_access;
>>>> +    p2m->domain = hostp2m->domain;
>>>> +    p2m->logdirty_ranges = hostp2m->logdirty_ranges;
>>>> +    p2m->global_logdirty = hostp2m->global_logdirty;
>>>
>>> This would certainly be one approach.  But then we'd need to keep a lot
>>> more of these things in sync -- for instance, we'd have to have similar
>>> code to enable and disable global_logdirty on all active altp2m entries.
>>>
>>> [...]
>>>
>>> The other thing that seems to be missing from synchronization is that in
>>> p2m-ept.c:ept_enable_pml() sets the p2m->ept.ad bit (which ends up being
>>> part of the eptp).  The code seems to indicate that this is required for
>>> PML (hardware-assisted logdirty), but I don't see anywhere this is set
>>> or copied from the host p2m.
>>>
>>> It might be nice to have a more structured way of keeping all these
>>> changes in sync, rather than relying on this open-coding everywhere.
>>
>> For logdirty_ranges and global_logdirty, I propose that we put them both
>> in a dynamically allocated struct, and have all p2ms share a pointer to
>> them. That way, all that's required is for the pointer to be set up in
>> p2m_init_altp2m_ept() and the actual data will be automatically shared
>> between p2ms. If I've read the code correctly, the hostp2m is the last
>> to be destroyed and the first to be initialized, so it should work well
>> as long as all p2ms synchronize access to logdirty_ranges and
>> global_logdirty (which I assume they already do).
> 
> That's an interesting idea; one potential disadvantage is that it would
> make locking even more complex than it already is.  Enabling / disabling
> logdirty isn't a hot path, so looping through the altp2ms shouldn't have
> much of a performance impact; *reading* is much more common, so having
> to grab an extra set of locks is more likely to have a performance
> impact.  And it's not clear to me that the complexity of keeping several
> copies in sync is that much higher than the complexity of adding Yet
> Another MM Lock.
> 
> Those are just initial reactions though -- feel free to explore the
> solution space and/or argue otherwise. :-)

No, you're right, there's no point in complicating things, I'll just
loop over the p2ms.


Thanks,
Razvan

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