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. :-)
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
