Re: VM_EVENT_FLAG_SET_REGISTERS and sync_vcpu_execstate()

[Tamas K Lengyel <tamas@tklengyel.com>]

On Mon, Aug 8, 2016 at 10:29 AM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> 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.
>

Using _nosync() predates me touching this code by a couple years,
according to git blame it goes all the way back to when mem_access was
introduced:

commit fbbedcae8c0c5374f8c0a869f49784b37baf04bb
Joe Epstein <jepstein98@gmail.com>
Date:   Fri Jan 7 11:54:40 2011 +0000

    mem_access: mem event additions for access

My only concern with changing to sync only in the set registers route
is that we potentially keep a buggy interface where we might run into
this problem in the future. IMHO just shifting all cases to do sync()
is safer, provided the performance difference is unnoticeable.

Tamas

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