[PATCH] x86/monitor: add get_capabilities to monitor_op domctl

[PATCH] x86/monitor: add get_capabilities to monitor_op domctl

[Tamas K Lengyel]

Add option to monitor_op domctl to determine the monitor capabilities of the
system.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
---
 tools/libxc/include/xenctrl.h |  6 ++++++
 tools/libxc/xc_monitor.c      | 21 +++++++++++++++++++++
 xen/arch/x86/hvm/hvm.c        |  5 +++++
 xen/arch/x86/monitor.c        | 25 +++++++++++++++++++++++++
 xen/common/domctl.c           |  1 +
 xen/include/asm-x86/hvm/hvm.h |  2 ++
 xen/include/public/domctl.h   | 18 +++++++++++++++---
 7 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index d1d2ab3..d930f02 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2376,6 +2376,12 @@ int xc_mem_access_disable_emulate(xc_interface *xch, domid_t domain_id);
 void *xc_monitor_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
 int xc_monitor_disable(xc_interface *xch, domid_t domain_id);
 int xc_monitor_resume(xc_interface *xch, domid_t domain_id);
+/*
+ * Get a bitmap of supported monitor events in the form
+ * (1 << XEN_DOMCTL_MONITOR_EVENT_*).
+ */
+int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id,
+                                uint32_t *capabilities);
 int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
                              uint16_t index, bool enable, bool sync,
                              bool onchangeonly);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 63013de..3221bdd 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -45,6 +45,27 @@ int xc_monitor_resume(xc_interface *xch, domid_t domain_id)
                                NULL);
 }
 
+int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id,
+                                uint32_t *capabilities)
+{
+    int rc;
+    DECLARE_DOMCTL;
+
+    if ( !capabilities )
+        return -EINVAL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    domctl.u.monitor_op.op = XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES;
+
+    rc = do_domctl(xch, &domctl);
+    if ( rc )
+        return rc;
+
+    *capabilities = domctl.u.monitor_op.event;
+    return 0;
+}
+
 int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
                              uint16_t index, bool enable, bool sync,
                              bool onchangeonly)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 535d622..d981f98 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6431,6 +6431,11 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
     return rc;
 }
 
+bool_t hvm_is_singlestep_supported(void)
+{
+    return cpu_has_monitor_trap_flag;
+}
+
 int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
 {
     if (hvm_funcs.nhvm_vcpu_hostrestore)
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 896acf7..c41efb1 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -42,6 +42,22 @@ int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
     return 0;
 }
 
+static inline
+void get_capabilities(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+    mop->event = 0;
+
+    if ( !is_hvm_domain(d) || !cpu_has_vmx )
+        return;
+
+    mop->event = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
+                 (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
+                 (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
+
+    if ( hvm_is_singlestep_supported() )
+        mop->event |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
+}
+
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 {
     int rc;
@@ -51,6 +67,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
     if ( rc )
         return rc;
 
+    if ( mop->op == XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES )
+    {
+        get_capabilities(d, mop);
+        return 0;
+    }
+
     /*
      * At the moment only Intel HVM domains are supported. However, event
      * delivery could be extended to AMD and PV domains.
@@ -141,6 +163,9 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
         if ( rc )
             return rc;
 
+        if ( !hvm_is_singlestep_supported() )
+            return -EOPNOTSUPP;
+
         domain_pause(d);
         ad->monitor.singlestep_enabled = !status;
         domain_unpause(d);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 2a2d203..3b806af 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1167,6 +1167,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             break;
 
         ret = monitor_domctl(d, &op->u.monitor_op);
+        copyback = 1;
         break;
 
     default:
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 57f9605..b2ce302 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -448,6 +448,8 @@ static inline void hvm_set_info_guest(struct vcpu *v)
 
 int hvm_debug_op(struct vcpu *v, int32_t op);
 
+bool_t hvm_is_singlestep_supported(void);
+
 static inline void hvm_invalidate_regs_fields(struct cpu_user_regs *regs)
 {
 #ifndef NDEBUG
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index bc45ea5..5d0fcdf 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1001,12 +1001,16 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
  * via the ring buffer "MONITOR". The ring has to be first enabled
  * with the domctl XEN_DOMCTL_VM_EVENT_OP_MONITOR.
  *
+ * GET_CAPABILITIES can be used to determine which of these features is
+ * available on a given platform.
+ *
  * NOTICE: mem_access events are also delivered via the "MONITOR" ring buffer;
  * however, enabling/disabling those events is performed with the use of
  * memory_op hypercalls!
  */
-#define XEN_DOMCTL_MONITOR_OP_ENABLE   0
-#define XEN_DOMCTL_MONITOR_OP_DISABLE  1
+#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_EVENT_WRITE_CTRLREG         0
 #define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            1
@@ -1015,7 +1019,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
-    uint32_t event; /* XEN_DOMCTL_MONITOR_EVENT_* */
+
+    /*
+     * When used with ENABLE/DISABLE this has be set to
+     * the requested XEN_DOMCTL_MONITOR_EVENT_* value.
+     * With GET_CAPABILITIES this field returns a bitmap of
+     * events supported by the platform, in the format
+     * (1 << XEN_DOMCTL_MONITOR_EVENT_*).
+     */
+    uint32_t event;
 
     /*
      * Further options when issuing XEN_DOMCTL_MONITOR_OP_ENABLE.
-- 
2.1.4

Re: [PATCH] x86/monitor: add get_capabilities to monitor_op domctl

[Razvan Cojocaru]

On 07/07/2015 03:43 AM, Tamas K Lengyel wrote:
> Add option to monitor_op domctl to determine the monitor capabilities of the
> system.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> ---
>  tools/libxc/include/xenctrl.h |  6 ++++++
>  tools/libxc/xc_monitor.c      | 21 +++++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c        |  5 +++++
>  xen/arch/x86/monitor.c        | 25 +++++++++++++++++++++++++
>  xen/common/domctl.c           |  1 +
>  xen/include/asm-x86/hvm/hvm.h |  2 ++
>  xen/include/public/domctl.h   | 18 +++++++++++++++---
>  7 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index d1d2ab3..d930f02 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2376,6 +2376,12 @@ int xc_mem_access_disable_emulate(xc_interface *xch, domid_t domain_id);
>  void *xc_monitor_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
>  int xc_monitor_disable(xc_interface *xch, domid_t domain_id);
>  int xc_monitor_resume(xc_interface *xch, domid_t domain_id);
> +/*
> + * Get a bitmap of supported monitor events in the form
> + * (1 << XEN_DOMCTL_MONITOR_EVENT_*).
> + */
> +int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id,
> +                                uint32_t *capabilities);
>  int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
>                               uint16_t index, bool enable, bool sync,
>                               bool onchangeonly);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index 63013de..3221bdd 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -45,6 +45,27 @@ int xc_monitor_resume(xc_interface *xch, domid_t domain_id)
>                                 NULL);
>  }
>  
> +int xc_monitor_get_capabilities(xc_interface *xch, domid_t domain_id,
> +                                uint32_t *capabilities)
> +{
> +    int rc;
> +    DECLARE_DOMCTL;
> +
> +    if ( !capabilities )
> +        return -EINVAL;
> +
> +    domctl.cmd = XEN_DOMCTL_monitor_op;
> +    domctl.domain = domain_id;
> +    domctl.u.monitor_op.op = XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES;
> +
> +    rc = do_domctl(xch, &domctl);
> +    if ( rc )
> +        return rc;
> +
> +    *capabilities = domctl.u.monitor_op.event;
> +    return 0;
> +}
> +
>  int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
>                               uint16_t index, bool enable, bool sync,
>                               bool onchangeonly)
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 535d622..d981f98 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6431,6 +6431,11 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>      return rc;
>  }
>  
> +bool_t hvm_is_singlestep_supported(void)
> +{
> +    return cpu_has_monitor_trap_flag;
> +}
> +
>  int nhvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>  {
>      if (hvm_funcs.nhvm_vcpu_hostrestore)
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 896acf7..c41efb1 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -42,6 +42,22 @@ int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
>      return 0;
>  }
>  
> +static inline
> +void get_capabilities(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> +    mop->event = 0;
> +
> +    if ( !is_hvm_domain(d) || !cpu_has_vmx )
> +        return;

We've tested the guest-requested vm_event with a PV domain on ARM (ARM
patch to come after the release of 4.6), so should we keep these HVM /
VMX checks until the first patch that changes the situation?


Regards,
Razvan

Re: [PATCH] x86/monitor: add get_capabilities to monitor_op domctl

[Jan Beulich]

>>> On 07.07.15 at 02:43, <tlengyel@novetta.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6431,6 +6431,11 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>      return rc;
>  }
>  
> +bool_t hvm_is_singlestep_supported(void)
> +{
> +    return cpu_has_monitor_trap_flag;
> +}

This being a VMX flag, it should be tested in VMX code, i.e. you'd
want to add a new hook to hvm_funcs.

> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -42,6 +42,22 @@ int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
>      return 0;
>  }
>  
> +static inline
> +void get_capabilities(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> +    mop->event = 0;
> +
> +    if ( !is_hvm_domain(d) || !cpu_has_vmx )
> +        return;
> +
> +    mop->event = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> +                 (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
> +                 (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
> +
> +    if ( hvm_is_singlestep_supported() )
> +        mop->event |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> +}

And with this in mind I suppose the hook then would return a bit
mask to be or-ed in here, instead of just a boolean flag.

> @@ -51,6 +67,12 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>      if ( rc )
>          return rc;
>  
> +    if ( mop->op == XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES )
> +    {
> +        get_capabilities(d, mop);
> +        return 0;
> +    }
> +
>      /*
>       * At the moment only Intel HVM domains are supported. However, event
>       * delivery could be extended to AMD and PV domains.

Considering that capabilities could be domain specific (as is also
expressed by d being passed to get_capabilities()), I think the
conditional above should actually move past the one following this
comment.

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -1167,6 +1167,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              break;
>  
>          ret = monitor_domctl(d, &op->u.monitor_op);
> +        copyback = 1;
>          break;

Conditional upon ret == 0?

> @@ -1015,7 +1019,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>  
>  struct xen_domctl_monitor_op {
>      uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> -    uint32_t event; /* XEN_DOMCTL_MONITOR_EVENT_* */
> +
> +    /*
> +     * When used with ENABLE/DISABLE this has be set to

... has to be ... ?

Jan

> +     * the requested XEN_DOMCTL_MONITOR_EVENT_* value.
> +     * With GET_CAPABILITIES this field returns a bitmap of
> +     * events supported by the platform, in the format
> +     * (1 << XEN_DOMCTL_MONITOR_EVENT_*).
> +     */
> +    uint32_t event;
>  
>      /*
>       * Further options when issuing XEN_DOMCTL_MONITOR_OP_ENABLE.

Re: [PATCH] x86/monitor: add get_capabilities to monitor_op domctl

[Lengyel, Tamas]

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

On Tue, Jul 7, 2015 at 4:22 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 07.07.15 at 02:43, <tlengyel@novetta.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -6431,6 +6431,11 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
> >      return rc;
> >  }
> >
> > +bool_t hvm_is_singlestep_supported(void)
> > +{
> > +    return cpu_has_monitor_trap_flag;
> > +}
>
> This being a VMX flag, it should be tested in VMX code, i.e. you'd
> want to add a new hook to hvm_funcs.
>

Ack.


>
> > --- a/xen/arch/x86/monitor.c
> > +++ b/xen/arch/x86/monitor.c
> > @@ -42,6 +42,22 @@ int status_check(struct xen_domctl_monitor_op *mop,
> bool_t status)
> >      return 0;
> >  }
> >
> > +static inline
> > +void get_capabilities(struct domain *d, struct xen_domctl_monitor_op
> *mop)
> > +{
> > +    mop->event = 0;
> > +
> > +    if ( !is_hvm_domain(d) || !cpu_has_vmx )
> > +        return;
> > +
> > +    mop->event = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> > +                 (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
> > +                 (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
> > +
> > +    if ( hvm_is_singlestep_supported() )
> > +        mop->event |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> > +}
>
> And with this in mind I suppose the hook then would return a bit
> mask to be or-ed in here, instead of just a boolean flag.
>

I think the boolean flag return is a lot more generic. I rather keep it as
is - otherwise I have to mask the bitfield again in the toggle patch too.


>
> > @@ -51,6 +67,12 @@ int monitor_domctl(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> >      if ( rc )
> >          return rc;
> >
> > +    if ( mop->op == XEN_DOMCTL_MONITOR_OP_GET_CAPABILITIES )
> > +    {
> > +        get_capabilities(d, mop);
> > +        return 0;
> > +    }
> > +
> >      /*
> >       * At the moment only Intel HVM domains are supported. However,
> event
> >       * delivery could be extended to AMD and PV domains.
>
> Considering that capabilities could be domain specific (as is also
> expressed by d being passed to get_capabilities()), I think the
> conditional above should actually move past the one following this
> comment.
>

Sure.


>
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -1167,6 +1167,7 @@ long
> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >              break;
> >
> >          ret = monitor_domctl(d, &op->u.monitor_op);
> > +        copyback = 1;
> >          break;
>
> Conditional upon ret == 0?
>

Yeap, wasn't sure about if we should gate the copyback. My reasoning was in
doesn't hurt to copyback for enable/disable, it's just unnecessary. But
gating it is probably better.


> > @@ -1015,7 +1019,15 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
> >
> >  struct xen_domctl_monitor_op {
> >      uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> > -    uint32_t event; /* XEN_DOMCTL_MONITOR_EVENT_* */
> > +
> > +    /*
> > +     * When used with ENABLE/DISABLE this has be set to
>
> ... has to be ... ?
>

Yeap.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 4718 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] x86/monitor: add get_capabilities to monitor_op domctl

[Jan Beulich]

>>> On 07.07.15 at 14:01, <tlengyel@novetta.com> wrote:
> On Tue, Jul 7, 2015 at 4:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 07.07.15 at 02:43, <tlengyel@novetta.com> wrote:
>> > --- a/xen/arch/x86/monitor.c
>> > +++ b/xen/arch/x86/monitor.c
>> > @@ -42,6 +42,22 @@ int status_check(struct xen_domctl_monitor_op *mop,
>> bool_t status)
>> >      return 0;
>> >  }
>> >
>> > +static inline
>> > +void get_capabilities(struct domain *d, struct xen_domctl_monitor_op
>> *mop)
>> > +{
>> > +    mop->event = 0;
>> > +
>> > +    if ( !is_hvm_domain(d) || !cpu_has_vmx )
>> > +        return;
>> > +
>> > +    mop->event = (1 << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
>> > +                 (1 << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
>> > +                 (1 << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT);
>> > +
>> > +    if ( hvm_is_singlestep_supported() )
>> > +        mop->event |= (1 << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
>> > +}
>>
>> And with this in mind I suppose the hook then would return a bit
>> mask to be or-ed in here, instead of just a boolean flag.
>>
> 
> I think the boolean flag return is a lot more generic. I rather keep it as
> is - otherwise I have to mask the bitfield again in the toggle patch too.

With the other patch in mind I agree.

Jan