On Feb 12, 2019, at 11:42 AM, Razvan Cojocaru <rcojoc...@bitdefender.com> wrote:
HVMOP_altp2m_set_domain_state does not domain_pause(), presumably
on purpose (as it was originally supposed to cater to a in-guest
agent, and a domain pausing itself is not a good idea).
Sorry to come in here on v4 and suggest changing everything, but I don’t really
like the solution you have here. Not setting altp2m to ‘active’ until after
the vcpus are set up makes sense; but passing this true/false value in seems
ugly, and still seems a bit racy (i.e., what if p2m_active() is disabled
between the check in HVMOP_altp2m_switch_p2m and the time we actually call
altp2m_vcpu_update_p2m()?)
I certainly don’t think domain_pause() should be our go-to solution for race
avoidance, but in this case it really seems like switching the global p2m for
every vcpu at once makes the most sense; and trying to safely change this on an
unpaused domain is not only overly complicated, but probably not what we wanted
anyway.
p2m_altp2m_destroy_by_id() and p2m_switch_domain_altp2m_by_id() already use
domain_pause_except_self(); so it seems like not using it for
altp2m_set_domain_state was probably more of an oversight than an intentional
decision. Using that here seems like a more robust solution.
The one issue is that domain_pause_except_self() currently is actually a
deadlock risk if two different vcpus start it at the same time. I think the
attached patch (compile-tested only) should fix this issue; after this patch
you should be able to use domain_pause_except_self() in altp2m_set_domain_state
instead.
Does that sound reasonable?
Not only does that sound reasonable, I personally prefer this approach.
I'll apply this patch and give it a spin.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
