[PATCH 0/2] Introspection optimization helpers

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

[PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep
[PATCH 2/2] xen: Introduce VM_EVENT_FLAG_SET_EIP


Thanks,
Razvan

[PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[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>
---
 tools/libxc/include/xenctrl.h    |   11 +++++++++++
 tools/libxc/xc_domain.c          |   18 ++++++++++++++++++
 xen/arch/x86/hvm/emulate.c       |    2 +-
 xen/common/domctl.c              |    5 +++++
 xen/include/asm-x86/hvm/domain.h |    1 +
 xen/include/public/domctl.h      |    8 ++++++++
 6 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3482544..4ece851 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -643,6 +643,17 @@ int xc_domain_node_getaffinity(xc_interface *xch,
                                xc_nodemap_t nodemap);
 
 /**
+ * This function enables / disables emulation for each REP for a
+ * REP-compatible instruction.
+ *
+ * @parm xch a handle to an open hypervisor interface.
+ * @parm domid 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_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable);
+
+/**
  * This function specifies the CPU affinity for a vcpu.
  *
  * There are two kinds of affinity. Soft affinity is on what CPUs a vcpu
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index e7278dd..19b2e46 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2555,6 +2555,24 @@ int xc_domain_soft_reset(xc_interface *xch,
     domctl.domain = (domid_t)domid;
     return do_domctl(xch, &domctl);
 }
+
+int xc_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable)
+{
+    int ret = -1;
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_emulate_each_rep;
+    domctl.domain = (domid_t)domid;
+    domctl.u.emulate_each_rep.op = enable;
+
+    ret = do_domctl(xch, &domctl);
+
+    if ( ret == -ESRCH )
+        errno = ENOENT;
+
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 5934c72..649eb7f 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -514,7 +514,7 @@ 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.hvm_domain.emulate_each_rep)
                            ? 1 : 4096);
 
     reg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 9e0fef5..214a22a 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1180,6 +1180,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             copyback = 1;
         break;
 
+    case XEN_DOMCTL_emulate_each_rep:
+        d->arch.hvm_domain.emulate_each_rep = !!op->u.emulate_each_rep.op;
+        ret = 0;
+        break;
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 992d5d1..b8fbe5e 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -136,6 +136,7 @@ struct hvm_domain {
     bool_t                 mem_sharing_enabled;
     bool_t                 qemu_mapcache_invalidate;
     bool_t                 is_s3_suspended;
+    bool_t                 emulate_each_rep;
 
     /*
      * TSC value that VCPUs use to calculate their tsc_offset value.
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 794d4d5..efc42a8 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1063,6 +1063,12 @@ struct xen_domctl_psr_cat_op {
 typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
 
+struct xen_domctl_emulate_each_rep {
+    int32_t op;
+};
+typedef struct xen_domctl_emulate_each_rep xen_domctl_emulate_each_rep_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_emulate_each_rep_t);
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -1140,6 +1146,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_monitor_op                    77
 #define XEN_DOMCTL_psr_cat_op                    78
 #define XEN_DOMCTL_soft_reset                    79
+#define XEN_DOMCTL_emulate_each_rep              80
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1202,6 +1209,7 @@ struct xen_domctl {
         struct xen_domctl_psr_cmt_op        psr_cmt_op;
         struct xen_domctl_monitor_op        monitor_op;
         struct xen_domctl_psr_cat_op        psr_cat_op;
+        struct xen_domctl_emulate_each_rep  emulate_each_rep;
         uint8_t                             pad[128];
     } u;
 };
-- 
1.7.9.5

Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Ian Campbell]

On Tue, 2015-09-15 at 12:19 +0300, Razvan Cojocaru wrote:
> 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>

Tools side is trivial and correct, so assuming the interface is agreed by
the hypervisor guys:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Julien Grall]

Hi Razvan,

On 15/09/15 10:19, Razvan Cojocaru wrote:
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 9e0fef5..214a22a 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -1180,6 +1180,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              copyback = 1;
>          break;
>  
> +    case XEN_DOMCTL_emulate_each_rep:
> +        d->arch.hvm_domain.emulate_each_rep = !!op->u.emulate_each_rep.op;

The common code should never access directly to an arch field.

Actually this will break on ARM because you introduced the field only
for x86.

Please move this new domctl in arch_do_domctl.

> +        ret = 0;
> +        break;
> +
>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
>          break;
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index 992d5d1..b8fbe5e 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -136,6 +136,7 @@ struct hvm_domain {
>      bool_t                 mem_sharing_enabled;
>      bool_t                 qemu_mapcache_invalidate;
>      bool_t                 is_s3_suspended;
> +    bool_t                 emulate_each_rep;
>  
>      /*
>       * TSC value that VCPUs use to calculate their tsc_offset value.
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 794d4d5..efc42a8 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1063,6 +1063,12 @@ struct xen_domctl_psr_cat_op {
>  typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>  
> +struct xen_domctl_emulate_each_rep {
> +    int32_t op;
> +};
> +typedef struct xen_domctl_emulate_each_rep xen_domctl_emulate_each_rep_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_emulate_each_rep_t);
> +
>  struct xen_domctl {
>      uint32_t cmd;
>  #define XEN_DOMCTL_createdomain                   1
> @@ -1140,6 +1146,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_monitor_op                    77
>  #define XEN_DOMCTL_psr_cat_op                    78
>  #define XEN_DOMCTL_soft_reset                    79
> +#define XEN_DOMCTL_emulate_each_rep              80
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -1202,6 +1209,7 @@ struct xen_domctl {
>          struct xen_domctl_psr_cmt_op        psr_cmt_op;
>          struct xen_domctl_monitor_op        monitor_op;
>          struct xen_domctl_psr_cat_op        psr_cat_op;
> +        struct xen_domctl_emulate_each_rep  emulate_each_rep;

IHMO this should be exposed only for x86 as we don't have a such
instruction on ARM.

>          uint8_t                             pad[128];
>      } u;
>  };
> 
	
Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Razvan Cojocaru]

On 09/15/2015 06:36 PM, Julien Grall wrote:
> Hi Razvan,
> 
> On 15/09/15 10:19, Razvan Cojocaru wrote:
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index 9e0fef5..214a22a 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -1180,6 +1180,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>              copyback = 1;
>>          break;
>>  
>> +    case XEN_DOMCTL_emulate_each_rep:
>> +        d->arch.hvm_domain.emulate_each_rep = !!op->u.emulate_each_rep.op;
> 
> The common code should never access directly to an arch field.
> 
> Actually this will break on ARM because you introduced the field only
> for x86.
> 
> Please move this new domctl in arch_do_domctl.

Of course, thanks for pointing that out!

>> +        ret = 0;
>> +        break;
>> +
>>      default:
>>          ret = arch_do_domctl(op, d, u_domctl);
>>          break;
>> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
>> index 992d5d1..b8fbe5e 100644
>> --- a/xen/include/asm-x86/hvm/domain.h
>> +++ b/xen/include/asm-x86/hvm/domain.h
>> @@ -136,6 +136,7 @@ struct hvm_domain {
>>      bool_t                 mem_sharing_enabled;
>>      bool_t                 qemu_mapcache_invalidate;
>>      bool_t                 is_s3_suspended;
>> +    bool_t                 emulate_each_rep;
>>  
>>      /*
>>       * TSC value that VCPUs use to calculate their tsc_offset value.
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 794d4d5..efc42a8 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1063,6 +1063,12 @@ struct xen_domctl_psr_cat_op {
>>  typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>>  
>> +struct xen_domctl_emulate_each_rep {
>> +    int32_t op;
>> +};
>> +typedef struct xen_domctl_emulate_each_rep xen_domctl_emulate_each_rep_t;
>> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_emulate_each_rep_t);
>> +
>>  struct xen_domctl {
>>      uint32_t cmd;
>>  #define XEN_DOMCTL_createdomain                   1
>> @@ -1140,6 +1146,7 @@ struct xen_domctl {
>>  #define XEN_DOMCTL_monitor_op                    77
>>  #define XEN_DOMCTL_psr_cat_op                    78
>>  #define XEN_DOMCTL_soft_reset                    79
>> +#define XEN_DOMCTL_emulate_each_rep              80
>>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
>> @@ -1202,6 +1209,7 @@ struct xen_domctl {
>>          struct xen_domctl_psr_cmt_op        psr_cmt_op;
>>          struct xen_domctl_monitor_op        monitor_op;
>>          struct xen_domctl_psr_cat_op        psr_cat_op;
>> +        struct xen_domctl_emulate_each_rep  emulate_each_rep;
> 
> IHMO this should be exposed only for x86 as we don't have a such
> instruction on ARM.

Sure, I'll #ifdef it with the other __i386__ and __x86_64__ things.


Thanks for the quick review,
Razvan

Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Andrew Cooper]

On 15/09/15 10:19, Razvan Cojocaru wrote:
> 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>

This disables all rep optimisations by default, so on its own is
inappropriate.

I am also not sure that an individual domctl subop is appropriate.  Its
purpose is to undo a performance hit caused by introspection, so should
live as an introspection subop IMO.

> ---
>  tools/libxc/include/xenctrl.h    |   11 +++++++++++
>  tools/libxc/xc_domain.c          |   18 ++++++++++++++++++
>  xen/arch/x86/hvm/emulate.c       |    2 +-
>  xen/common/domctl.c              |    5 +++++
>  xen/include/asm-x86/hvm/domain.h |    1 +
>  xen/include/public/domctl.h      |    8 ++++++++
>  6 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 3482544..4ece851 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -643,6 +643,17 @@ int xc_domain_node_getaffinity(xc_interface *xch,
>                                 xc_nodemap_t nodemap);
>  
>  /**
> + * This function enables / disables emulation for each REP for a
> + * REP-compatible instruction.
> + *
> + * @parm xch a handle to an open hypervisor interface.
> + * @parm domid 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_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable);
> +
> +/**
>   * This function specifies the CPU affinity for a vcpu.
>   *
>   * There are two kinds of affinity. Soft affinity is on what CPUs a vcpu
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index e7278dd..19b2e46 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -2555,6 +2555,24 @@ int xc_domain_soft_reset(xc_interface *xch,
>      domctl.domain = (domid_t)domid;
>      return do_domctl(xch, &domctl);
>  }
> +
> +int xc_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable)
> +{
> +    int ret = -1;
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_emulate_each_rep;
> +    domctl.domain = (domid_t)domid;
> +    domctl.u.emulate_each_rep.op = enable;
> +
> +    ret = do_domctl(xch, &domctl);
> +
> +    if ( ret == -ESRCH )
> +        errno = ENOENT;

Why do you override ESRCH?  ESRCH is the expected error for a missing
domain.

~Andrew

Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Razvan Cojocaru]

On 09/17/2015 03:59 PM, Andrew Cooper wrote:
> On 15/09/15 10:19, Razvan Cojocaru wrote:
>> 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>
> 
> This disables all rep optimisations by default, so on its own is
> inappropriate.

REP optimizations are enabled by default. Emulate_each_rep is initially
set to 0, when struct hvm_domain is being initialized, which means that
REP optimizations are enabled. I've tested this and it does work, am I
missing something?

> I am also not sure that an individual domctl subop is appropriate.  Its
> purpose is to undo a performance hit caused by introspection, so should
> live as an introspection subop IMO.

Do you mean xc_monitor_emulate_each_rep() instead of
xc_domain_emulate_each_rep()?

I've placed this in its own domctl subop because it's not introspection
(or vm_event) specific. The change in
xen/arch/x86/hvm/emulate.c enables / disables REP emulation
optimizations regardless of whether there's a vm_event client or not. I
thought this might come handy for somebody else too.

>> ---
>>  tools/libxc/include/xenctrl.h    |   11 +++++++++++
>>  tools/libxc/xc_domain.c          |   18 ++++++++++++++++++
>>  xen/arch/x86/hvm/emulate.c       |    2 +-
>>  xen/common/domctl.c              |    5 +++++
>>  xen/include/asm-x86/hvm/domain.h |    1 +
>>  xen/include/public/domctl.h      |    8 ++++++++
>>  6 files changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 3482544..4ece851 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -643,6 +643,17 @@ int xc_domain_node_getaffinity(xc_interface *xch,
>>                                 xc_nodemap_t nodemap);
>>  
>>  /**
>> + * This function enables / disables emulation for each REP for a
>> + * REP-compatible instruction.
>> + *
>> + * @parm xch a handle to an open hypervisor interface.
>> + * @parm domid 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_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable);
>> +
>> +/**
>>   * This function specifies the CPU affinity for a vcpu.
>>   *
>>   * There are two kinds of affinity. Soft affinity is on what CPUs a vcpu
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index e7278dd..19b2e46 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -2555,6 +2555,24 @@ int xc_domain_soft_reset(xc_interface *xch,
>>      domctl.domain = (domid_t)domid;
>>      return do_domctl(xch, &domctl);
>>  }
>> +
>> +int xc_domain_emulate_each_rep(xc_interface *xch, uint32_t domid, int enable)
>> +{
>> +    int ret = -1;
>> +    DECLARE_DOMCTL;
>> +
>> +    domctl.cmd = XEN_DOMCTL_emulate_each_rep;
>> +    domctl.domain = (domid_t)domid;
>> +    domctl.u.emulate_each_rep.op = enable;
>> +
>> +    ret = do_domctl(xch, &domctl);
>> +
>> +    if ( ret == -ESRCH )
>> +        errno = ENOENT;
> 
> Why do you override ESRCH?  ESRCH is the expected error for a missing
> domain.

I shouldn't, really. This is a copy/paste issue - I've copied and
changed a similar function in Xen 4.4, and that code just got carried
over to this patch. I'll remove that check.


Thanks,
Razvan

Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Andrew Cooper]

On 17/09/15 14:20, Razvan Cojocaru wrote:
> On 09/17/2015 03:59 PM, Andrew Cooper wrote:
>> On 15/09/15 10:19, Razvan Cojocaru wrote:
>>> 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>
>> This disables all rep optimisations by default, so on its own is
>> inappropriate.
> REP optimizations are enabled by default. Emulate_each_rep is initially
> set to 0, when struct hvm_domain is being initialized, which means that
> REP optimizations are enabled. I've tested this and it does work, am I
> missing something?

Oops - you are completely correct.  I got the logic reversed in my
head.  Sorry for the noise.

>
>> I am also not sure that an individual domctl subop is appropriate.  Its
>> purpose is to undo a performance hit caused by introspection, so should
>> live as an introspection subop IMO.
> Do you mean xc_monitor_emulate_each_rep() instead of
> xc_domain_emulate_each_rep()?
>
> I've placed this in its own domctl subop because it's not introspection
> (or vm_event) specific. The change in
> xen/arch/x86/hvm/emulate.c enables / disables REP emulation
> optimizations regardless of whether there's a vm_event client or not. I
> thought this might come handy for somebody else too.

I can't think of a rational reason for anyone to disable rep
optimisations for the sake of it.

I am concerned about introducing options with which people can
needlessly shoot themselves in the foot.  On the other hand, there are
already enough of those.

~Andrew

Re: [PATCH 1/2] xen, libxc: Introduced XEN_DOMCTL_emulate_each_rep

[Razvan Cojocaru]

On 09/17/2015 04:37 PM, Andrew Cooper wrote:
> On 17/09/15 14:20, Razvan Cojocaru wrote:
>> On 09/17/2015 03:59 PM, Andrew Cooper wrote:
>>> On 15/09/15 10:19, Razvan Cojocaru wrote:
>>>> 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>
>>> This disables all rep optimisations by default, so on its own is
>>> inappropriate.
>> REP optimizations are enabled by default. Emulate_each_rep is initially
>> set to 0, when struct hvm_domain is being initialized, which means that
>> REP optimizations are enabled. I've tested this and it does work, am I
>> missing something?
> 
> Oops - you are completely correct.  I got the logic reversed in my
> head.  Sorry for the noise.

No problem at all, thanks for the review!

>>> I am also not sure that an individual domctl subop is appropriate.  Its
>>> purpose is to undo a performance hit caused by introspection, so should
>>> live as an introspection subop IMO.
>> Do you mean xc_monitor_emulate_each_rep() instead of
>> xc_domain_emulate_each_rep()?
>>
>> I've placed this in its own domctl subop because it's not introspection
>> (or vm_event) specific. The change in
>> xen/arch/x86/hvm/emulate.c enables / disables REP emulation
>> optimizations regardless of whether there's a vm_event client or not. I
>> thought this might come handy for somebody else too.
> 
> I can't think of a rational reason for anyone to disable rep
> optimisations for the sake of it.
> 
> I am concerned about introducing options with which people can
> needlessly shoot themselves in the foot.  On the other hand, there are
> already enough of those.

Understood, in that case I'll move it to an xc_monitor_() function and
add an extra check for that in emulate.c in V2 (unless anyone has an
objection to that?).


Thanks,
Razvan

[PATCH 2/2] xen: Introduce VM_EVENT_FLAG_SET_EIP

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

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/mm/p2m.c          |   25 ++++++++++++++++---------
 xen/include/asm-x86/vm_event.h |    1 +
 xen/include/public/vm_event.h  |    5 +++++
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c4329d2..ef45b15 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1596,6 +1596,8 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
 
         if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
             v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
+        else if ( (rsp->flags & VM_EVENT_FLAG_SET_EIP) )
+            v->arch.vm_event->set_eip = rsp->data.regs.x86.rip;
     }
 }
 
@@ -1694,17 +1696,22 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
 
     if ( unlikely(v->arch.vm_event) && v->arch.vm_event->emulate_flags )
     {
-        enum emul_kind kind = EMUL_KIND_NORMAL;
+        if ( v->arch.vm_event->emulate_flags & VM_EVENT_FLAG_SET_EIP )
+            guest_cpu_user_regs()->eip = v->arch.vm_event->set_eip;
+        else
+        {
+            enum emul_kind kind = EMUL_KIND_NORMAL;
 
-        if ( v->arch.vm_event->emulate_flags &
-             VM_EVENT_FLAG_SET_EMUL_READ_DATA )
-            kind = EMUL_KIND_SET_CONTEXT;
-        else if ( v->arch.vm_event->emulate_flags &
-                  VM_EVENT_FLAG_EMULATE_NOWRITE )
-            kind = EMUL_KIND_NOWRITE;
+            if ( v->arch.vm_event->emulate_flags &
+                 VM_EVENT_FLAG_SET_EMUL_READ_DATA )
+                kind = EMUL_KIND_SET_CONTEXT;
+            else if ( v->arch.vm_event->emulate_flags &
+                      VM_EVENT_FLAG_EMULATE_NOWRITE )
+                kind = EMUL_KIND_NOWRITE;
 
-        hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
-                                   HVM_DELIVER_NO_ERROR_CODE);
+            hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
+                                       HVM_DELIVER_NO_ERROR_CODE);
+        }
 
         v->arch.vm_event->emulate_flags = 0;
         return 1;
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 2ff2cab..310fc5a 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -30,6 +30,7 @@ struct arch_vm_event {
     uint32_t emulate_flags;
     unsigned long gpa;
     unsigned long eip;
+    unsigned long set_eip;
     struct vm_event_emul_read_data emul_read_data;
     struct monitor_write_data write_data;
 };
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index ff2f217..0109bdf 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -89,6 +89,11 @@
  * by the altp2m_idx response field if possible.
  */
 #define VM_EVENT_FLAG_ALTERNATE_P2M      (1 << 7)
+/*
+ * Move the guest's instruction pointer to data.regs.x86.rip from the vm_event
+ * response.
+ */
+#define VM_EVENT_FLAG_SET_EIP            (1 << 8)
 
 /*
  * Reasons for the vm event request
-- 
1.7.9.5

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

[Tamas K Lengyel]

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

On Tue, Sep 15, 2015 at 5:19 AM, Razvan Cojocaru <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.
>

So in my opinion this patch introduces a feature that is not strictly tied
to emulation related vm_event paths. I could use this feature to update the
instruction pointer any time we respond to a vm_event and furthermore, it
may be benefitial to expand the scope of which registers can be updated
this way. For example, I have tools that update not just the instruction
pointer but also the stack pointer and registers used to pass function
inputs. Since we already send a snapshot of select registers to the user
with each event, we could introduce a response flag that indicates that all
registers included in that snapshot should be set to the values sent back
by the user. The user then could choose which registers need to be updated
in bulk.

What do you think?

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 1669 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 2/2] xen: Introduce VM_EVENT_FLAG_SET_EIP

[Razvan Cojocaru]

On 09/16/2015 06:57 PM, Tamas K Lengyel wrote:
> 
> 
> On Tue, Sep 15, 2015 at 5:19 AM, Razvan Cojocaru
> <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.
> 
> 
> So in my opinion this patch introduces a feature that is not strictly
> tied to emulation related vm_event paths. I could use this feature to
> update the instruction pointer any time we respond to a vm_event and
> furthermore, it may be benefitial to expand the scope of which registers
> can be updated this way. For example, I have tools that update not just
> the instruction pointer but also the stack pointer and registers used to
> pass function inputs. Since we already send a snapshot of select
> registers to the user with each event, we could introduce a response
> flag that indicates that all registers included in that snapshot should
> be set to the values sent back by the user. The user then could choose
> which registers need to be updated in bulk.
> 
> What do you think?

Hello Tamas, thanks for the reply!

Yes, I thought it might come up that this doesn't have to be
emulation-specific, but thought I'd hitch it there since I've assumed
that at the moment this is the only case where it's actually used.

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?


Thanks,
Razvan

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

[Tamas K Lengyel]

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

On Wed, Sep 16, 2015 at 12:12 PM, Razvan Cojocaru <rcojocaru@bitdefender.com
> wrote:

> On 09/16/2015 06:57 PM, Tamas K Lengyel wrote:
> >
> >
> > On Tue, Sep 15, 2015 at 5:19 AM, Razvan Cojocaru
> > <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.
> >
> >
> > So in my opinion this patch introduces a feature that is not strictly
> > tied to emulation related vm_event paths. I could use this feature to
> > update the instruction pointer any time we respond to a vm_event and
> > furthermore, it may be benefitial to expand the scope of which registers
> > can be updated this way. For example, I have tools that update not just
> > the instruction pointer but also the stack pointer and registers used to
> > pass function inputs. Since we already send a snapshot of select
> > registers to the user with each event, we could introduce a response
> > flag that indicates that all registers included in that snapshot should
> > be set to the values sent back by the user. The user then could choose
> > which registers need to be updated in bulk.
> >
> > What do you think?
>
> Hello Tamas, thanks for the reply!
>
> Yes, I thought it might come up that this doesn't have to be
> emulation-specific, but thought I'd hitch it there since I've assumed
> that at the moment this is the only case where it's actually used.
>
> 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.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 3160 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 2/2] xen: Introduce VM_EVENT_FLAG_SET_EIP

[Jan Beulich]

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

Jan

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

[Razvan Cojocaru]

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