diff mbox series

[v5,09/11] x86/domctl: add XEN_DOMCTL_vmtrace_op

Message ID f3ec05eb4908f774683e96553ec32d68fac0d0ac.1593974333.git.michal.leszczynski@cert.pl (mailing list archive)
State Superseded
Headers show
Series Implement support for external IPT monitoring | expand

Commit Message

Michał Leszczyński July 5, 2020, 6:55 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       | 48 +++++++++++++++++++++++++++++++++++++
 xen/include/public/domctl.h | 26 ++++++++++++++++++++
 2 files changed, 74 insertions(+)

Comments

Julien Grall July 6, 2020, 10:31 a.m. UTC | #1 
Hi Michal,

On 05/07/2020 19:55, Michał Leszczyński wrote:
> +/* 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;

AFAICT, there is a 16-bit implicit padding here and ...


> +    uint32_t vcpu;

... a 32-bit implicit padding here. I would suggest to make
the two explicit.

We possibly want to check they are also always zero.

Cheers,
Jan Beulich July 6, 2020, 10:37 a.m. UTC | #2 
On 06.07.2020 12:31, Julien Grall wrote:
> On 05/07/2020 19:55, Michał Leszczyński wrote:
>> +/* 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;
> 
> AFAICT, there is a 16-bit implicit padding here and ...
> 
> 
>> +    uint32_t vcpu;
> 
> ... a 32-bit implicit padding here. I would suggest to make
> the two explicit.
> 
> We possibly want to check they are also always zero.

Not just possibly imo - we should.

Jan
Julien Grall July 6, 2020, 10:38 a.m. UTC | #3 
On 06/07/2020 11:37, Jan Beulich wrote:
> On 06.07.2020 12:31, Julien Grall wrote:
>> On 05/07/2020 19:55, Michał Leszczyński wrote:
>>> +/* 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;
>>
>> AFAICT, there is a 16-bit implicit padding here and ...
>>
>>
>>> +    uint32_t vcpu;
>>
>> ... a 32-bit implicit padding here. I would suggest to make
>> the two explicit.
>>
>> We possibly want to check they are also always zero.
> 
> Not just possibly imo - we should.

I wasn't sure given that DOMCTL is not a stable interface.

Cheers,
Jan Beulich July 6, 2020, 11:20 a.m. UTC | #4 
On 06.07.2020 12:38, Julien Grall wrote:
> On 06/07/2020 11:37, Jan Beulich wrote:
>> On 06.07.2020 12:31, Julien Grall wrote:
>>> On 05/07/2020 19:55, Michał Leszczyński wrote:
>>>> +/* 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;
>>>
>>> AFAICT, there is a 16-bit implicit padding here and ...
>>>
>>>
>>>> +    uint32_t vcpu;
>>>
>>> ... a 32-bit implicit padding here. I would suggest to make
>>> the two explicit.
>>>
>>> We possibly want to check they are also always zero.
>>
>> Not just possibly imo - we should.
> 
> I wasn't sure given that DOMCTL is not a stable interface.

True; checking padding fields allows assigning meaning to them
without bumping the domctl interface version.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 6f2c69788d..a041b724d8 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -322,6 +322,48 @@  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 ( !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);
+        spin_lock(&d->vmtrace_lock);
+
+        rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable);
+
+        spin_unlock(&d->vmtrace_lock);
+        vcpu_unpause(v);
+        break;
+
+    case XEN_DOMCTL_vmtrace_pt_get_offset:
+        rc = vmtrace_get_pt_offset(v, &op->offset);
+        break;
+
+    default:
+        rc = -EOPNOTSUPP;
+    }
+
+    return rc;
+}
+
 #define MAX_IOPORTS 0x10000
 
 long arch_do_domctl(
@@ -337,6 +379,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 7b8289d436..f836cb5970 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1136,6 +1136,28 @@  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;
+    uint32_t vcpu;
+    uint64_aligned_t size;
+
+    /* OUT variable */
+    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 +1239,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 +1300,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;
 };