[v6,09/11] x86/domctl: add XEN_DOMCTL_vmtrace_op
diff mbox series

Message ID a9899858dba4a7e22a0256cff734399bff348adb.1594150543.git.michal.leszczynski@cert.pl
State New
Headers show
Series
  • Implement support for external IPT monitoring
Related show

Commit Message

Michał Leszczyński July 7, 2020, 7:39 p.m. UTC
From: Michal Leszczynski <michal.leszczynski@cert.pl>

Implement domctl to manage the runtime state of
processor trace feature.

Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
---
 xen/arch/x86/domctl.c       | 50 +++++++++++++++++++++++++++++++++++++
 xen/include/public/domctl.h | 28 +++++++++++++++++++++
 2 files changed, 78 insertions(+)

Comments

Roger Pau Monné July 15, 2020, 5:32 p.m. UTC | #1
On Tue, Jul 07, 2020 at 09:39:48PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@cert.pl>
> 
> Implement domctl to manage the runtime state of
> processor trace feature.
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
> ---
>  xen/arch/x86/domctl.c       | 50 +++++++++++++++++++++++++++++++++++++
>  xen/include/public/domctl.h | 28 +++++++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 6f2c69788d..6132499db4 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -322,6 +322,50 @@ void arch_get_domain_info(const struct domain *d,
>      info->arch_config.emulation_flags = d->arch.emulation_flags;
>  }
>  
> +static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
> +                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    int rc;
> +    struct vcpu *v;
> +
> +    if ( op->pad1 || op->pad2 )
> +        return -EINVAL;
> +
> +    if ( !vmtrace_supported )
> +        return -EOPNOTSUPP;
> +
> +    if ( !is_hvm_domain(d) )
> +        return -EOPNOTSUPP;

You can join both if into a single one if they both return
-EOPNOTSUPP.

> +
> +    if ( op->vcpu >= d->max_vcpus )
> +        return -EINVAL;

You can remove this check and just use the return value of domain_vcpu
instead, if it's NULL the vCPU ID is wrong.

> +
> +    v = domain_vcpu(d, op->vcpu);
> +    rc = 0;

No need to init rc to 0, as it would be unconditionally initialized in
the switch below due to the default case.

> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_vmtrace_pt_enable:
> +    case XEN_DOMCTL_vmtrace_pt_disable:
> +        vcpu_pause(v);
> +        rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable);
> +        vcpu_unpause(v);
> +        break;
> +
> +    case XEN_DOMCTL_vmtrace_pt_get_offset:
> +        rc = vmtrace_get_pt_offset(v, &op->offset, &op->size);

In order to get consistent values here the vCPU should be paused, or
else you just get stale values from the last vmexit?

Maybe that's fine for your use case?

> +
> +        if ( !rc && d->is_dying )
> +            rc = ENODATA;

This check here seems weird, if this is really required shouldn't it
be done before attempting to fetch the data?

> +        break;
> +
> +    default:
> +        rc = -EOPNOTSUPP;
> +    }
> +
> +    return rc;
> +}
> +
>  #define MAX_IOPORTS 0x10000
>  
>  long arch_do_domctl(
> @@ -337,6 +381,12 @@ long arch_do_domctl(
>      switch ( domctl->cmd )
>      {
>  
> +    case XEN_DOMCTL_vmtrace_op:
> +        ret = do_vmtrace_op(d, &domctl->u.vmtrace_op, u_domctl);
> +        if ( !ret )
> +            copyback = true;
> +        break;

Nit: new additions would usually got at the end of the switch.

> +
>      case XEN_DOMCTL_shadow_op:
>          ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
>          if ( ret == -ERESTART )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 7681675a94..73c7ccbd16 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1136,6 +1136,30 @@ struct xen_domctl_vuart_op {
>                                   */
>  };
>  
> +/* XEN_DOMCTL_vmtrace_op: Perform VM tracing related operation */
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +struct xen_domctl_vmtrace_op {
> +    /* IN variable */
> +    uint32_t cmd;
> +/* Enable/disable external vmtrace for given domain */
> +#define XEN_DOMCTL_vmtrace_pt_enable      1
> +#define XEN_DOMCTL_vmtrace_pt_disable     2
> +#define XEN_DOMCTL_vmtrace_pt_get_offset  3
> +    domid_t domain;
> +    uint16_t pad1;
> +    uint32_t vcpu;
> +    uint16_t pad2;

pad2 should be a uint32_t? Or else there's a padding field added by
the compiler? (maybe I'm missing something, I haven't checked with
pahole).

> +
> +    /* OUT variable */
> +    uint64_aligned_t size;
> +    uint64_aligned_t offset;
> +};
> +typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
> +
> +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> +
>  struct xen_domctl {
>      uint32_t cmd;
>  #define XEN_DOMCTL_createdomain                   1
> @@ -1217,6 +1241,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_vuart_op                      81
>  #define XEN_DOMCTL_get_cpu_policy                82
>  #define XEN_DOMCTL_set_cpu_policy                83
> +#define XEN_DOMCTL_vmtrace_op                    84
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -1277,6 +1302,9 @@ struct xen_domctl {
>          struct xen_domctl_monitor_op        monitor_op;
>          struct xen_domctl_psr_alloc         psr_alloc;
>          struct xen_domctl_vuart_op          vuart_op;
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +        struct xen_domctl_vmtrace_op        vmtrace_op;
> +#endif

No need for the preprocessor conditionals here, the whole domctl.h is
only to be used by the tools or Xen, and is already properly guarded
(see the #error at the top of the file).

Thanks.

Patch
diff mbox series

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 6f2c69788d..6132499db4 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -322,6 +322,50 @@  void arch_get_domain_info(const struct domain *d,
     info->arch_config.emulation_flags = d->arch.emulation_flags;
 }
 
+static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op,
+                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+    int rc;
+    struct vcpu *v;
+
+    if ( op->pad1 || op->pad2 )
+        return -EINVAL;
+
+    if ( !vmtrace_supported )
+        return -EOPNOTSUPP;
+
+    if ( !is_hvm_domain(d) )
+        return -EOPNOTSUPP;
+
+    if ( op->vcpu >= d->max_vcpus )
+        return -EINVAL;
+
+    v = domain_vcpu(d, op->vcpu);
+    rc = 0;
+
+    switch ( op->cmd )
+    {
+    case XEN_DOMCTL_vmtrace_pt_enable:
+    case XEN_DOMCTL_vmtrace_pt_disable:
+        vcpu_pause(v);
+        rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable);
+        vcpu_unpause(v);
+        break;
+
+    case XEN_DOMCTL_vmtrace_pt_get_offset:
+        rc = vmtrace_get_pt_offset(v, &op->offset, &op->size);
+
+        if ( !rc && d->is_dying )
+            rc = ENODATA;
+        break;
+
+    default:
+        rc = -EOPNOTSUPP;
+    }
+
+    return rc;
+}
+
 #define MAX_IOPORTS 0x10000
 
 long arch_do_domctl(
@@ -337,6 +381,12 @@  long arch_do_domctl(
     switch ( domctl->cmd )
     {
 
+    case XEN_DOMCTL_vmtrace_op:
+        ret = do_vmtrace_op(d, &domctl->u.vmtrace_op, u_domctl);
+        if ( !ret )
+            copyback = true;
+        break;
+
     case XEN_DOMCTL_shadow_op:
         ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
         if ( ret == -ERESTART )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 7681675a94..73c7ccbd16 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1136,6 +1136,30 @@  struct xen_domctl_vuart_op {
                                  */
 };
 
+/* XEN_DOMCTL_vmtrace_op: Perform VM tracing related operation */
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+
+struct xen_domctl_vmtrace_op {
+    /* IN variable */
+    uint32_t cmd;
+/* Enable/disable external vmtrace for given domain */
+#define XEN_DOMCTL_vmtrace_pt_enable      1
+#define XEN_DOMCTL_vmtrace_pt_disable     2
+#define XEN_DOMCTL_vmtrace_pt_get_offset  3
+    domid_t domain;
+    uint16_t pad1;
+    uint32_t vcpu;
+    uint16_t pad2;
+
+    /* OUT variable */
+    uint64_aligned_t size;
+    uint64_aligned_t offset;
+};
+typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
+
+#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -1217,6 +1241,7 @@  struct xen_domctl {
 #define XEN_DOMCTL_vuart_op                      81
 #define XEN_DOMCTL_get_cpu_policy                82
 #define XEN_DOMCTL_set_cpu_policy                83
+#define XEN_DOMCTL_vmtrace_op                    84
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1277,6 +1302,9 @@  struct xen_domctl {
         struct xen_domctl_monitor_op        monitor_op;
         struct xen_domctl_psr_alloc         psr_alloc;
         struct xen_domctl_vuart_op          vuart_op;
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+        struct xen_domctl_vmtrace_op        vmtrace_op;
+#endif
         uint8_t                             pad[128];
     } u;
 };