From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V2 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS Date: Tue, 22 Sep 2015 18:39:27 +0300 Message-ID: <5601762F.50803@bitdefender.com> References: <1442842294-6123-1-git-send-email-rcojocaru@bitdefender.com> <1442842294-6123-3-git-send-email-rcojocaru@bitdefender.com> <56018DA402000078000A4872@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel , Jan Beulich Cc: "wei.liu2@citrix.com" , Ian Campbell , Andrew Cooper , Ian Jackson , Xen-devel , Stefano Stabellini , Stefano Stabellini , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 09/22/2015 06:34 PM, Tamas K Lengyel wrote: > > > On Tue, Sep 22, 2015 at 9:19 AM, Jan Beulich > wrote: > > >>> On 21.09.15 at 15:31, > 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 > > > > > --- > > 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