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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/08/2016 03:47 PM, Razvan Cojocaru wrote:
> On 08/08/2016 03:01 PM, Andrew Cooper wrote:
>> On 08/08/16 11:31, Razvan Cojocaru wrote:
>>> Hello,
>>>
>>> We've been mostly setting registers by using xc_vcpu_setcontext():
>>>
>>> https://github.com/razvan-cojocaru/libbdvmi/blob/master/src/bdvmixendriver.cpp#L504
>>>
>>> but lately trying to push as much of that as possible to the
>>> VM_EVENT_FLAG_SET_REGISTERS-related code (i.e. via the vm_event replies)
>>> to save processing time.
>>>
>>> So with the xc_vcpu_setcontext() call removed, I've found that there are
>>> cases where vm_event_set_registers() won't work properly unless I kept
>>> the xc_vcpu_getcontext() call. This would appear to be not because of
>>> anything that arch_get_info_guest() does (please see the implementation
>>> for XEN_DOMCTL_getvcpucontext), but because of the vcpu_pause() call, or
>>> more specifically, because of its calling sync_vcpu_execstate().
>>>
>>> So it would appear that a sync_vcpu_execstate(v) call is necessary at
>>> the start of vm_event_set_registers() for the vcpu struct instance to be
>>> synchronized with the current VCPU state.
>>>
>>> Any objections to a patch with this simple modification?
>>
>> It would be helpful to identify exactly what is currently going wrong,
>> and why sync_vcpu_execstate(v) helps.
> 
> I've placed a
> 
> printk("EIP: 0x%016lx, 0x%016lx\n", v->arch.user_regs.eip,
> rsp->data.regs.x86.rip);
> 
> at the top of vm_event_set_registers(), so I could see what the old
> value is (v->arch.user_regs.eip) vs. the new value (rsp->data.regs.x86.rip).
> 
> I'm also logging these in my application, the difference there being
> that the old value is the value that came with the vm_event request,
> which has been obtained with guest_cpu_user_regs()->eip (where in
> vm_event_set_registers() we set v->arch.user_regs.eip, since v != current).
> 
> Here's a short test run, with xl dmesg:
> 
> (XEN) [  395.739543] EIP: 0xfffff80001be5957, 0xfffff80001be595b
> (XEN) [  409.795311] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  416.675023] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  421.475567] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  428.275125] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  435.507009] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  441.318224] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  445.514807] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  452.539190] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  454.762810] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  459.538651] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  462.027222] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  481.770470] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  483.298493] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  486.522344] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  491.042325] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  494.874468] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  497.450765] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  500.562738] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  509.179754] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  510.826236] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  512.106206] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  518.658092] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  520.450156] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  521.882088] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  523.250092] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  524.577987] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  525.962042] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  527.353942] EIP: 0xfffff80001be4812, 0xfffff80001be4812
> (XEN) [  528.714089] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  530.065994] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  531.357762] EIP: 0xfffff80001be4812, 0xfffff80001be4812
> (XEN) [  532.594016] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  533.849886] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  535.145879] EIP: 0xfffff80001be4812, 0xfffff80001be4812
> (XEN) [  536.546846] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  537.905756] EIP: 0xfffff80001be480f, 0xfffff80001be4812
> (XEN) [  539.209454] EIP: 0xfffff80001be4812, 0xfffff80001be4812
> 
> and the corresponding part in the userspace application's log:
> 
> GET EIP: 0xfffff80001be5957 SET EIP: 0xfffff80001be595b
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> GET EIP: 0xfffff80001be480f SET EIP: 0xfffff80001be4812
> 
> As you can see, they're different (and correct in the application log),
> but (for example for the last one) they both already have the same value
> in the hypervisor. So it would appear that guest_cpu_user_regs()->eip !=
> v->arch.user_regs.eip at that point (and likely even more state than
> that differs there).
> 
> These are EPT fault events, all of them, and I just reply with
> VM_EVENT_FLAG_SET_REGISTERS to them here, and nothing else.

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.


Thanks,
Razvan

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