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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

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

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?

Tamas

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