[PATCH V2 0/2] Introspection optimization helpers

[PATCH V2 0/2] Introspection optimization helpers

[Razvan Cojocaru]

Hello,

This series adds two minor patches. The first one allows finer-grained
control over the emulation behaviour of REP instructions. Previously,
once vm_event-based emulation was enabled, no optimizations were allowed.
However, this has a performance impact on the monitored guest, so I've
added a new libxc function to enable / disable this at will at any point.

The second patch addresses an older issue: in my initial series a few
years back, there was a (longish) patch that computed the length of the
current instruction, and advanced the instruction pointer past it if
it was required by the vm_event reply. However, integrating that code
has not been trivial, and so the second patch in this series simply
allows a vm_event reply to say that the instruction pointer should be
set to whatever value it sends back to the hypervisor. This way, the
computation of the instruction length is left to the userspace
application.

This new version addresses the comments received for V1: the first
patch retires xc_domain_emulate_each_rep() in favour of
xc_monitor_emulate_each_rep(), and the second patch now follows
Tamas' suggestion to replace SET_EIP with SET_REGISTERS and allow
any vm_event reply to set the guest registers (though this currently
only applies to EIP).

[PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation
[PATCH V2 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS


Thanks,
Razvan

[PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Razvan Cojocaru]

Previously, if vm_event emulation support was enabled, then REP
optimizations were disabled when emulating REP-compatible
instructions. This patch allows fine-tuning of this behaviour by
providing a dedicated libxc helper function.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V1:
 - Renamed the patch, since this is now a xc_monitor_ function,
   so XEN_DOMCTL_emulate_each_rep no longer applies.
 - As suggested by Andrew Cooper, the function is a no-op unless
   mem_access emulation is enabled.
---
 tools/libxc/include/xenctrl.h |   12 ++++++++++++
 tools/libxc/xc_monitor.c      |   13 +++++++++++++
 xen/arch/x86/hvm/emulate.c    |    3 ++-
 xen/arch/x86/monitor.c        |    6 ++++++
 xen/include/asm-x86/domain.h  |    1 +
 xen/include/public/domctl.h   |    1 +
 6 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3482544..3bfa00b 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2428,6 +2428,18 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
                              bool enable, bool sync);
 
+/**
+ * This function enables / disables emulation for each REP for a
+ * REP-compatible instruction.
+ *
+ * @parm xch a handle to an open hypervisor interface.
+ * @parm domain_id the domain id one wants to get the node affinity of.
+ * @parm enable if 0 optimize when possible, else emulate each REP.
+ * @return 0 on success, -1 on failure.
+ */
+int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
+                                bool enable);
+
 /***
  * Memory sharing operations.
  *
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 065669c..b1705dd 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -143,3 +143,16 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id, bool enable,
 
     return do_domctl(xch, &domctl);
 }
+
+int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
+                                bool enable)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    domctl.u.monitor_op.op = XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP;
+    domctl.u.monitor_op.event = enable;
+
+    return do_domctl(xch, &domctl);
+}
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 5934c72..c39a883 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -514,7 +514,8 @@ static int hvmemul_virtual_to_linear(
      * being triggered for repeated writes to a whole page.
      */
     *reps = min_t(unsigned long, *reps,
-                  unlikely(current->domain->arch.mem_access_emulate_enabled)
+                  unlikely(current->domain->arch.mem_access_emulate_enabled &&
+                           current->domain->arch.mem_access_emulate_each_rep)
                            ? 1 : 4096);
 
     reg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 3d52135..3cb7519 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -79,6 +79,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         return 0;
     }
 
+    if ( mop->op == XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP )
+    {
+        d->arch.mem_access_emulate_each_rep = !!mop->event;
+        return 0;
+    }
+
     /*
      * Sanity check
      */
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 59cf826..a088110 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -386,6 +386,7 @@ struct arch_domain
 
     /* Mem_access emulation control */
     bool_t mem_access_emulate_enabled;
+    bool_t mem_access_emulate_each_rep;
 } __cacheline_aligned;
 
 #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 794d4d5..ae241f2 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1007,6 +1007,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_OP_ENABLE            0
 #define XEN_DOMCTL_MONITOR_OP_DISABLE           1
 #define XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES  2
+#define XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP  3
 
 #define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG         0
 #define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            1
-- 
1.7.9.5

Re: [PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Jan Beulich]

>>> On 21.09.15 at 15:31, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -514,7 +514,8 @@ static int hvmemul_virtual_to_linear(
>       * being triggered for repeated writes to a whole page.
>       */
>      *reps = min_t(unsigned long, *reps,
> -                  unlikely(current->domain->arch.mem_access_emulate_enabled)
> +                  unlikely(current->domain->arch.mem_access_emulate_enabled &&
> +                           current->domain->arch.mem_access_emulate_each_rep)
>                             ? 1 : 4096);

unlikely() should not wrap compound conditions, or else its effect of
eliminating mis-predicted branches from the fast path won't be
achieved. In the case here I wonder though whether you couldn't
simply test only ->arch.mem_access_emulate_each_rep.

> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -79,6 +79,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>          return 0;
>      }
>  
> +    if ( mop->op == XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP )
> +    {
> +        d->arch.mem_access_emulate_each_rep = !!mop->event;
> +        return 0;
> +    }

Considering that there's another "if(mop->op == ...)" right above
this, these two together should become another switch().

Jan

Re: [PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Razvan Cojocaru]

On 09/22/2015 06:17 PM, Jan Beulich wrote:
>>>> On 21.09.15 at 15:31, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -514,7 +514,8 @@ static int hvmemul_virtual_to_linear(
>>       * being triggered for repeated writes to a whole page.
>>       */
>>      *reps = min_t(unsigned long, *reps,
>> -                  unlikely(current->domain->arch.mem_access_emulate_enabled)
>> +                  unlikely(current->domain->arch.mem_access_emulate_enabled &&
>> +                           current->domain->arch.mem_access_emulate_each_rep)
>>                             ? 1 : 4096);
> 
> unlikely() should not wrap compound conditions, or else its effect of
> eliminating mis-predicted branches from the fast path won't be
> achieved. In the case here I wonder though whether you couldn't
> simply test only ->arch.mem_access_emulate_each_rep.

I'll unfold the unlikely().

Testing only ->arch.mem_access_emulate_each_rep is what I had done in
the original patch, however on Andrew Cooper's suggestion I've now gated
this on ->domain->arch.mem_access_emulate_enabled as well.

Otherwise, somebody might set mem_access_emulate_each_rep via its
xc_monitor_*() call, but then after calling xc_monitor_disable() it
would still be in effect, even if the guest is no longer being monitored.

If this is not a problem, I'm happy to check just
->arch.mem_access_emulate_each_rep.

>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -79,6 +79,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>>          return 0;
>>      }
>>  
>> +    if ( mop->op == XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP )
>> +    {
>> +        d->arch.mem_access_emulate_each_rep = !!mop->event;
>> +        return 0;
>> +    }
> 
> Considering that there's another "if(mop->op == ...)" right above
> this, these two together should become another switch().

Understood.


Thanks,
Razvan

Re: [PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Jan Beulich]

>>> On 22.09.15 at 17:28, <rcojocaru@bitdefender.com> wrote:
> On 09/22/2015 06:17 PM, Jan Beulich wrote:
>>>>> On 21.09.15 at 15:31, <rcojocaru@bitdefender.com> wrote:
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -514,7 +514,8 @@ static int hvmemul_virtual_to_linear(
>>>       * being triggered for repeated writes to a whole page.
>>>       */
>>>      *reps = min_t(unsigned long, *reps,
>>> -                  unlikely(current->domain->arch.mem_access_emulate_enabled)
>>> +                  unlikely(current->domain->arch.mem_access_emulate_enabled &&
>>> +                           current->domain->arch.mem_access_emulate_each_rep)
>>>                             ? 1 : 4096);
>> 
>> unlikely() should not wrap compound conditions, or else its effect of
>> eliminating mis-predicted branches from the fast path won't be
>> achieved. In the case here I wonder though whether you couldn't
>> simply test only ->arch.mem_access_emulate_each_rep.
> 
> I'll unfold the unlikely().
> 
> Testing only ->arch.mem_access_emulate_each_rep is what I had done in
> the original patch, however on Andrew Cooper's suggestion I've now gated
> this on ->domain->arch.mem_access_emulate_enabled as well.
> 
> Otherwise, somebody might set mem_access_emulate_each_rep via its
> xc_monitor_*() call, but then after calling xc_monitor_disable() it
> would still be in effect, even if the guest is no longer being monitored.
> 
> If this is not a problem, I'm happy to check just
> ->arch.mem_access_emulate_each_rep.

Or perhaps "disabled" should just clear that flag, also to not surprise
the next one to "enable"?

Jan

Re: [PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Razvan Cojocaru]

On 09/22/2015 06:39 PM, Jan Beulich wrote:
>>>> On 22.09.15 at 17:28, <rcojocaru@bitdefender.com> wrote:
>> On 09/22/2015 06:17 PM, Jan Beulich wrote:
>>>>>> On 21.09.15 at 15:31, <rcojocaru@bitdefender.com> wrote:
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -514,7 +514,8 @@ static int hvmemul_virtual_to_linear(
>>>>       * being triggered for repeated writes to a whole page.
>>>>       */
>>>>      *reps = min_t(unsigned long, *reps,
>>>> -                  unlikely(current->domain->arch.mem_access_emulate_enabled)
>>>> +                  unlikely(current->domain->arch.mem_access_emulate_enabled &&
>>>> +                           current->domain->arch.mem_access_emulate_each_rep)
>>>>                             ? 1 : 4096);
>>>
>>> unlikely() should not wrap compound conditions, or else its effect of
>>> eliminating mis-predicted branches from the fast path won't be
>>> achieved. In the case here I wonder though whether you couldn't
>>> simply test only ->arch.mem_access_emulate_each_rep.
>>
>> I'll unfold the unlikely().
>>
>> Testing only ->arch.mem_access_emulate_each_rep is what I had done in
>> the original patch, however on Andrew Cooper's suggestion I've now gated
>> this on ->domain->arch.mem_access_emulate_enabled as well.
>>
>> Otherwise, somebody might set mem_access_emulate_each_rep via its
>> xc_monitor_*() call, but then after calling xc_monitor_disable() it
>> would still be in effect, even if the guest is no longer being monitored.
>>
>> If this is not a problem, I'm happy to check just
>> ->arch.mem_access_emulate_each_rep.
> 
> Or perhaps "disabled" should just clear that flag, also to not surprise
> the next one to "enable"?

Fair point, I'll do that.


Thanks,
Razvan

Re: [PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Ian Campbell]

On Mon, 2015-09-21 at 16:31 +0300, Razvan Cojocaru wrote:
> diff --git a/tools/libxc/include/xenctrl.h
> b/tools/libxc/include/xenctrl.h
> index 3482544..3bfa00b 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2428,6 +2428,18 @@ int xc_monitor_software_breakpoint(xc_interface
> *xch, domid_t domain_id,
>  int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
>                               bool enable, bool sync);
>  
> +/**
> + * This function enables / disables emulation for each REP for a
> + * REP-compatible instruction.
> + *
> + * @parm xch a handle to an open hypervisor interface.
> + * @parm domain_id the domain id one wants to get the node affinity of.
> + * @parm enable if 0 optimize when possible, else emulate each REP.
> + * @return 0 on success, -1 on failure.
> + */
> +int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
> +                                bool enable);
> +
>  /***
>   * Memory sharing operations.
>   *
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index 065669c..b1705dd 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -143,3 +143,16 @@ int xc_monitor_guest_request(xc_interface *xch,
> domid_t domain_id, bool enable,
>  
>      return do_domctl(xch, &domctl);
>  }
> +
> +int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
> +                                bool enable)
> +{
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_monitor_op;
> +    domctl.domain = domain_id;
> +    domctl.u.monitor_op.op = XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP;
> +    domctl.u.monitor_op.event = enable;
> +
> +    return do_domctl(xch, &domctl);
> +}

This is a plausible binding of a hypercall interface so from the toolside
if the hypervisor people are happy with the nderlying inteface:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH V2 2/2] xen: Introduce VM_EVENT_FLAG_SET_REGISTERS

[Razvan Cojocaru]

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.
---
 xen/arch/x86/vm_event.c        |    5 +++++
 xen/common/vm_event.c          |    3 +++
 xen/include/asm-arm/vm_event.h |    6 ++++++
 xen/include/asm-x86/vm_event.h |    2 ++
 xen/include/public/vm_event.h  |    6 ++++++
 5 files changed, 22 insertions(+)

diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e4e0aa4..a59ba79 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -95,6 +95,11 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
     }
 }
 
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
+{
+    v->arch.user_regs.eip = rsp->data.regs.x86.rip;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index ef84b0f..e1f9580 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -417,6 +417,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 
         if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
         {
+            if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
+                vm_event_set_registers(v, &rsp);
+
             if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
                 vm_event_toggle_singlestep(d, v);
 
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 976fdf1..4d0fbf7 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -47,4 +47,10 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
     /* Not supported on ARM. */
 }
 
+static inline
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
+{
+    /* Not supported on ARM. */
+}
+
 #endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 2ff2cab..5aff834 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -42,4 +42,6 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
 
 void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
+void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp);
+
 #endif /* __ASM_X86_VM_EVENT_H__ */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index ff2f217..51539af 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -89,6 +89,12 @@
  * by the altp2m_idx response field if possible.
  */
 #define VM_EVENT_FLAG_ALTERNATE_P2M      (1 << 7)
+/*
+ * Set the vCPU registers to the values in the  vm_event response.
+ * Currently only applies to EIP.
+ * Requires the vCPU to be paused already (synchronous events only).
+ */
+#define VM_EVENT_FLAG_SET_REGISTERS      (1 << 8)
 
 /*
  * Reasons for the vm event request
-- 
1.7.9.5

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

[Jan Beulich]

>>> 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).

Jan

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1505 bytes --]

On Tue, Sep 22, 2015 at 9:19 AM, Jan Beulich <JBeulich@suse.com> 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).
>
> Jan
>

IMHO you should just add setting all registers included in the snapshot
here rather then postpone it to a later patch.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2209 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Razvan Cojocaru]

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

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

[Razvan Cojocaru]

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

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2424 bytes --]

On Tue, Sep 22, 2015 at 9:35 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> 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.
>

Yeap, my idea would be to just set all registers to the values included in
the snapshot sent back by the user. If the user doesn't change a register
value in the snapshot he received, that register would effectively be reset
to the same value it already had. Not perfect but IMHO harmless and reduces
the code required to keep track of things.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 3212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH V2 0/2] Introspection optimization helpers

[Razvan Cojocaru]

Hello,

This doesn't have much to do with this series, but when running
scripts/get-maintainer.pl on my patches, I got "Stefano Stabellini
<stefano.stabellini@eu.citrix.com>" for my first patch, and "Stefano
Stabellini <stefano.stabellini@citrix.com>" for the second one (i.e. the
second address is missing the ".eu" part).

I don't know if this is intended, so this is a heads-up.


Thanks,
Razvan

Re: [PATCH V2 0/2] Introspection optimization helpers

[Julien Grall]

On 21/09/15 14:58, Razvan Cojocaru wrote:
> Hello,

Hi Razvan,

> This doesn't have much to do with this series, but when running
> scripts/get-maintainer.pl on my patches, I got "Stefano Stabellini
> <stefano.stabellini@eu.citrix.com>" for my first patch, and "Stefano
> Stabellini <stefano.stabellini@citrix.com>" for the second one (i.e. the
> second address is missing the ".eu" part).
> 
> I don't know if this is intended, so this is a heads-up.

Both emails are valid and redirect to the same person.

Regards,

-- 
Julien Grall