Re: [PATCH 2/2] xen: Introduce VM_EVENT_FLAG_SET_EIP

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/21/2015 11:53 AM, Jan Beulich wrote:
>>>> On 18.09.15 at 21:19, <tamas@tklengyel.com> wrote:
>> On Wed, Sep 16, 2015 at 12:12 PM, Razvan Cojocaru <rcojocaru@bitdefender.com 
>>> wrote:
>>> I have nothing in principle against having a SET_REGISTERS flag instead
>>> of a SET_EIP one, but I am unsure of how (and where) that would be best
>>> implemented. What do you have in mind? A handler similar to void
>>> vm_event_register_write_resume() where we set these registers for the
>>> respective vcpu? Is this even possible at vm_event response handling time?
>>>
>>
>> No, that function falls under a switch on rsp.reason, for which we have a
>> 1:1 unofficial and not really enforced rule to match the type of event that
>> was sent. This should fall under a flag on rsp.flags and be handled similar
>> to how vm_event_toggle_singlestep is.
> 
> I.e. I take this to mean that we should wait for a new patch
> rather than further looking at the current one.

Yes, I've already modified the first patch on Andrew Cooper's suggestion
(to switch from xc_domain_emulate_each_rep() to
xc_monitor_emulate_each_rep(), and gate the emulation disable condition
on mem_access_emulate_enable as well as mem_access_emulate_each_rep),
and I'm working on switching from SET_EIP to SET_REGISTERS as we speak,
after which I'll do a test run and send a new version, hopefully no
later than tomorrow.

For this patch, I'm slightly unsure if I should expect trouble for
trying to do it this way (I know I have to abstract that raw code away
for x86 and ARM in their respective functions, but let's just assume x86
for the example):

418         if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
419         {
420             if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
421                 v->arch.user_regs.eip = rsp.data.regs.x86.rip;
422
423             if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
424                 vm_event_toggle_singlestep(d, v);
425
426             vm_event_vcpu_unpause(v);
427         }

at the end of vm_event_resume() in common/vm_event.c. Looks like it
should be safe, but I'm not sure.


Thanks,
Razvan