Re: [Xen-devel] VM_EVENT_FLAG_SET_REGISTERS and sync_vcpu_execstate()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/08/16 18:52, Tamas K Lengyel wrote:
>> I think the issue might be that vm_event_vcpu_pause() uses
>> > vcpu_pause_nosync(), and it's being used everywhere we pause the VCPU in
>> > the course of sending out a vm_event.
>> >
>> > So this creates the premise for a race condition: either some more
>> > things happen between sending out the vm_event and replying to it that
>> > cause v->arch.user_regs to be synchronized - in which case everything
>> > works (this has been the case when I was reading the VCPU context via a
>> > domctl that did vcpu_pause()) -, or not, in which case all bets are off.
>> >
>> > In this case, we should either drop vm_event_vcpu_pause() completely and
>> > just use vcpu_pause() everywhere, modify it to use vcpu_sleep_sync()
>> > (and basically turn it into vcpu_pause()), or only sync in
>> > vm_event_set_registers() as I've suggested.
>> >
> I would say just using vcpu_pause() would make sense as it's the least
> complicated route. Is there any downside of doing the sync() in all
> cases? Was the current way implemented perhaps the way it is for
> performance reasons? If so, is it noticeable at all?

I was hoping you'd know why it's implemented like this :), I think maybe
Tim Deegan (CCd, hopefully the email address is not out of date) did the
original implementation?

That's why I proposed to only sync in vm_event_set_registers() - for all
the other cases we can then keep the current performance (if
significant). The least complicated route (at least as far as the patch
changes go) I think is also this one, since it only requires a new line
of code in vm_event_set_registers() - using vcpu_pause() everywhere else
requires removing vm_event_pause_vcpu() as well as replacing the call
everywhere else.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel