Re: [PATCH V2 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/22/2015 06:19 PM, Jan Beulich wrote:
>>>> On 21.09.15 at 15:31, <rcojocaru@bitdefender.com> wrote:
>> A previous version of this patch dealing with support for skipping
>> the current instruction when a vm_event response requested it
>> computed the instruction length in the hypervisor, adding non-trivial
>> code dependencies. This patch allows a userspace vm_event client to
>> simply request that the guest's EIP is set to an arbitary value,
>> computed by the introspection application. In the future, other
>> registers can also be set via a vm_event reply by using this flag.
>> The VCPU needs to be paused for this flag to take effect.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>
>> ---
>> Changes since V1:
>>  - Renamed the patch (VM_EVENT_FLAG_SET_EIP ->
>>    VM_EVENT_FLAG_SET_REGISTERS).
>>  - As suggested by Tamas Lengyel, EIP is now being set via a dedicated
>>    generic vm_event_set_registers() function that can be extended to
>>    set other registers in the future.
> 
> Isn't it a bad move to call the thing "set registers" but have it set
> just EIP? If going forward you were to add more registers, you'd
> need new flags anyway I suppose, and hence the public interface
> part of this should be reverted (while the other internal
> abstraction seems fine to me).

Well, the way I've read Tamas' request is that he'd like other registers
to be set as well in vm_event_set_registers() (such as EAX, and so on) -
but since I'm not sure what he'd like added and how to test his
scenarios, I thought I could just set EIP for now, and either add what
he's requesting in future versions of the series, or allow him to extend
the code as needed with future patches.

I think that the end goal for Tamas would be to just, if this flag is
set, to set at least some of the registers that come back from the
vm_event reply to the VCPU that caused the vm_event request.


Thanks,
Razvan