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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 09/22/2015 06:34 PM, Tamas K Lengyel wrote:
> 
> 
> On Tue, Sep 22, 2015 at 9:19 AM, Jan Beulich <JBeulich@suse.com
> <mailto:JBeulich@suse.com>> wrote:
> 
>     >>> On 21.09.15 at 15:31, <rcojocaru@bitdefender.com <mailto: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 <mailto: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).
> 
>     Jan
> 
> 
> IMHO you should just add setting all registers included in the snapshot
> here rather then postpone it to a later patch.

Right, but setting some of the registers in the reply has side-effects
(such as the control registers), so I thought it better to not just try
to copy them if it's not needed (though I suppose we could check if the
new value differs from the old and only set it if it is at least).


Thanks,
Razvan