[PATCH 0/3] xen: guest tracing improvements

[PATCH 0/3] xen: guest tracing improvements

[David Vrabel]

There exists an existing hypercall sub-op (HVMOP_xentrace) for adding
trace records.  This series makes some minor improvements and adds a
libxc function to call it.

David

[PATCH 1/3] trace: include timestamp in trace records added by HVMOP_xentrace

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

TRC_GUEST trace records added by the HVMOP_xentrace op are not
timestamped.  Timestamping these records is useful if only TRC_GUEST
records are enabled.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 1fcaed0..e2701b6 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4391,8 +4391,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
              || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
             return -EINVAL;
 
-        /* Cycles will be taken at the vmexit and vmenter */
-        trace_var(tr.event | TRC_GUEST, 0 /*!cycles*/,
+        trace_var(tr.event | TRC_GUEST, 1 /*cycles*/,
                   tr.extra_bytes, tr.extra);
         break;
     }
-- 
1.7.2.5

Re: [PATCH 1/3] trace: include timestamp in trace records added by HVMOP_xentrace

[George Dunlap]

On 25/07/13 14:23, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> TRC_GUEST trace records added by the HVMOP_xentrace op are not
> timestamped.  Timestamping these records is useful if only TRC_GUEST
> records are enabled.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Sorry for the delay in reviewing.

  -George

[PATCH 2/3] trace: allow HVMOP_xentrace to set trace record subclass

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Allow guests adding trace records with HVMOP_xentrace to set the
sub-class.  This allows different guest generated traces to be
filtered by xentrace.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/hvm/hvm.c     |    4 ++--
 xen/include/public/trace.h |    3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e2701b6..b0d8094 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4388,10 +4388,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
 
         if ( tr.extra_bytes > sizeof(tr.extra)
-             || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
+             || (tr.event & ~((1u<<TRC_CLS_SHIFT)-1)) )
             return -EINVAL;
 
-        trace_var(tr.event | TRC_GUEST, 1 /*cycles*/,
+        trace_var(TRC_GUEST_EVENT(tr.event), 1 /*cycles*/,
                   tr.extra_bytes, tr.extra);
         break;
     }
diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index e2f60a6..bbbd38b 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -244,6 +244,9 @@
 #define TRC_HW_IRQ_UNMAPPED_VECTOR    (TRC_HW_IRQ + 0x7)
 #define TRC_HW_IRQ_HANDLED            (TRC_HW_IRQ + 0x8)
 
+/* Guest event with guest-specified sub-class and number. */
+#define TRC_GUEST_EVENT(e) (0x08000000 | (e & 0xffff))
+
 /*
  * Event Flags
  *
-- 
1.7.2.5

Re: [PATCH 2/3] trace: allow HVMOP_xentrace to set trace record subclass

[George Dunlap]

On 25/07/13 14:23, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> Allow guests adding trace records with HVMOP_xentrace to set the
> sub-class.  This allows different guest generated traces to be
> filtered by xentrace.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   xen/arch/x86/hvm/hvm.c     |    4 ++--
>   xen/include/public/trace.h |    3 +++
>   2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e2701b6..b0d8094 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4388,10 +4388,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               return -EFAULT;
>   
>           if ( tr.extra_bytes > sizeof(tr.extra)
> -             || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
> +             || (tr.event & ~((1u<<TRC_CLS_SHIFT)-1)) )
>               return -EINVAL;
>   
> -        trace_var(tr.event | TRC_GUEST, 1 /*cycles*/,
> +        trace_var(TRC_GUEST_EVENT(tr.event), 1 /*cycles*/,
>                     tr.extra_bytes, tr.extra);
>           break;
>       }
> diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
> index e2f60a6..bbbd38b 100644
> --- a/xen/include/public/trace.h
> +++ b/xen/include/public/trace.h
> @@ -244,6 +244,9 @@
>   #define TRC_HW_IRQ_UNMAPPED_VECTOR    (TRC_HW_IRQ + 0x7)
>   #define TRC_HW_IRQ_HANDLED            (TRC_HW_IRQ + 0x8)
>   
> +/* Guest event with guest-specified sub-class and number. */
> +#define TRC_GUEST_EVENT(e) (0x08000000 | (e & 0xffff))

Minor nit -- should this be ((1u<<TRC_CLS_SHIFT)-1)?

Alternately, would it make sense to add the following, and use it both 
places in this patch?

#define TRC_CLS_MASK ((1u<<TRC_CLS_SHIFT)-1)

  -George

[PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

Add xc_tbuf_trace() to allow trace records to be added to the trace
buffer.  The subclass and event number and up to 7 uin32_t arguments
may be specified.

The hypercall sub-op used is HVMOP_xentrace which (despite the name)
may be used by PV guests.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 tools/libxc/xc_tbuf.c  |   39 +++++++++++++++++++++++++++++++++++++++
 tools/libxc/xenctrl.h  |   11 +++++++++++
 xen/arch/x86/hvm/hvm.c |    2 +-
 3 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index 4fb7bb1..0644952 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -156,3 +156,42 @@ int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask)
     return do_sysctl(xch, &sysctl);
 }
 
+int xc_tbuf_trace(xc_interface *xch, uint16_t event, unsigned nr_args, ...)
+{
+    DECLARE_HYPERCALL;
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_xentrace_t, trace);
+    unsigned i;
+    va_list args;
+    int ret = -1;
+
+    if ( nr_args > 7 ) {
+        errno = -EINVAL;
+        goto out;
+    }
+
+    trace = xc_hypercall_buffer_alloc(xch, trace, sizeof(*trace));
+    if ( trace == NULL )
+    {
+        PERROR("Count not alloc bounce buffer for trace hypercall");
+        goto out;
+    }
+
+    trace->event = event;
+    trace->extra_bytes = nr_args * sizeof(uint32_t);
+
+    va_start(args, nr_args);
+    for (i = 0; i < nr_args; i++)
+        ((uint32_t *)trace->extra)[i] = va_arg(args, uint32_t);
+    va_end(args);
+
+    hypercall.op = __HYPERVISOR_hvm_op;
+    hypercall.arg[0] = HVMOP_xentrace;
+    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(trace);
+
+    ret = do_xen_hypercall(xch, &hypercall);
+
+out:
+    xc_hypercall_buffer_free(xch, trace);
+
+    return ret;
+}
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 388a9c3..927b198 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1387,6 +1387,17 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask);
 
 int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
 
+/**
+ * Insert a trace record into the trace buffer.
+ *
+ * The trace records use the TRC_GUEST class and 'event' sets the
+ * sub-class and the event number.  There are no predefined values for
+ * these.
+ *
+ * Up to 7 uint32_t arguments may be included in the trace record.
+ */
+int xc_tbuf_trace(xc_interface *xch, uint16_t event, unsigned nr_args, ...);
+
 int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
 int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index b0d8094..b24a37d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4388,7 +4388,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
 
         if ( tr.extra_bytes > sizeof(tr.extra)
-             || (tr.event & ~((1u<<TRC_CLS_SHIFT)-1)) )
+             || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
             return -EINVAL;
 
         trace_var(TRC_GUEST_EVENT(tr.event), 1 /*cycles*/,
-- 
1.7.2.5

Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records

[David Vrabel]

On 25/07/13 14:23, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Add xc_tbuf_trace() to allow trace records to be added to the trace
> buffer.  The subclass and event number and up to 7 uin32_t arguments
> may be specified.
> 
> The hypercall sub-op used is HVMOP_xentrace which (despite the name)
> may be used by PV guests.
> 
[...]
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4388,7 +4388,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>              return -EFAULT;
>  
>          if ( tr.extra_bytes > sizeof(tr.extra)
> -             || (tr.event & ~((1u<<TRC_CLS_SHIFT)-1)) )
> +             || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>              return -EINVAL;
>  
>          trace_var(TRC_GUEST_EVENT(tr.event), 1 /*cycles*/,

This hunk shouldn't be here. I made a mistake when refactoring the series.

David

Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records

[George Dunlap]

On 25/07/13 14:23, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> Add xc_tbuf_trace() to allow trace records to be added to the trace
> buffer.  The subclass and event number and up to 7 uin32_t arguments
> may be specified.
>
> The hypercall sub-op used is HVMOP_xentrace which (despite the name)
> may be used by PV guests.

Would it make sense to make this interface more like the hypervisor's 
trace_var() -- that is, taking a sizeof() and single pointer, so that 
callers can easily pass in packed structs?

I suppose there's no reason we couldn't add a function with that 
interface at some point in the future if people want it.  Any thoughts 
from the toolstack maintainers?

  -George

>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   tools/libxc/xc_tbuf.c  |   39 +++++++++++++++++++++++++++++++++++++++
>   tools/libxc/xenctrl.h  |   11 +++++++++++
>   xen/arch/x86/hvm/hvm.c |    2 +-
>   3 files changed, 51 insertions(+), 1 deletions(-)
>
> diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> index 4fb7bb1..0644952 100644
> --- a/tools/libxc/xc_tbuf.c
> +++ b/tools/libxc/xc_tbuf.c
> @@ -156,3 +156,42 @@ int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask)
>       return do_sysctl(xch, &sysctl);
>   }
>   
> +int xc_tbuf_trace(xc_interface *xch, uint16_t event, unsigned nr_args, ...)
> +{
> +    DECLARE_HYPERCALL;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_xentrace_t, trace);
> +    unsigned i;
> +    va_list args;
> +    int ret = -1;
> +
> +    if ( nr_args > 7 ) {
> +        errno = -EINVAL;
> +        goto out;
> +    }
> +
> +    trace = xc_hypercall_buffer_alloc(xch, trace, sizeof(*trace));
> +    if ( trace == NULL )
> +    {
> +        PERROR("Count not alloc bounce buffer for trace hypercall");
> +        goto out;
> +    }
> +
> +    trace->event = event;
> +    trace->extra_bytes = nr_args * sizeof(uint32_t);
> +
> +    va_start(args, nr_args);
> +    for (i = 0; i < nr_args; i++)
> +        ((uint32_t *)trace->extra)[i] = va_arg(args, uint32_t);
> +    va_end(args);
> +
> +    hypercall.op = __HYPERVISOR_hvm_op;
> +    hypercall.arg[0] = HVMOP_xentrace;
> +    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(trace);
> +
> +    ret = do_xen_hypercall(xch, &hypercall);
> +
> +out:
> +    xc_hypercall_buffer_free(xch, trace);
> +
> +    return ret;
> +}
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 388a9c3..927b198 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1387,6 +1387,17 @@ int xc_tbuf_set_cpu_mask(xc_interface *xch, uint32_t mask);
>   
>   int xc_tbuf_set_evt_mask(xc_interface *xch, uint32_t mask);
>   
> +/**
> + * Insert a trace record into the trace buffer.
> + *
> + * The trace records use the TRC_GUEST class and 'event' sets the
> + * sub-class and the event number.  There are no predefined values for
> + * these.
> + *
> + * Up to 7 uint32_t arguments may be included in the trace record.
> + */
> +int xc_tbuf_trace(xc_interface *xch, uint16_t event, unsigned nr_args, ...);
> +
>   int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
>   int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
>   
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index b0d8094..b24a37d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4388,7 +4388,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               return -EFAULT;
>   
>           if ( tr.extra_bytes > sizeof(tr.extra)
> -             || (tr.event & ~((1u<<TRC_CLS_SHIFT)-1)) )
> +             || (tr.event & ~((1u<<TRC_SUBCLS_SHIFT)-1)) )
>               return -EINVAL;
>   
>           trace_var(TRC_GUEST_EVENT(tr.event), 1 /*cycles*/,

Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records

[David Vrabel]

On 15/08/13 15:37, George Dunlap wrote:
> On 25/07/13 14:23, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> Add xc_tbuf_trace() to allow trace records to be added to the trace
>> buffer.  The subclass and event number and up to 7 uin32_t arguments
>> may be specified.
>>
>> The hypercall sub-op used is HVMOP_xentrace which (despite the name)
>> may be used by PV guests.
> 
> Would it make sense to make this interface more like the hypervisor's
> trace_var() -- that is, taking a sizeof() and single pointer, so that
> callers can easily pass in packed structs?

I did it like this so it would only take a single line to get a useful
trace point.

David

Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records

[George Dunlap]

On 15/08/13 15:39, David Vrabel wrote:
> On 15/08/13 15:37, George Dunlap wrote:
>> On 25/07/13 14:23, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> Add xc_tbuf_trace() to allow trace records to be added to the trace
>>> buffer.  The subclass and event number and up to 7 uin32_t arguments
>>> may be specified.
>>>
>>> The hypercall sub-op used is HVMOP_xentrace which (despite the name)
>>> may be used by PV guests.
>> Would it make sense to make this interface more like the hypervisor's
>> trace_var() -- that is, taking a sizeof() and single pointer, so that
>> callers can easily pass in packed structs?
> I did it like this so it would only take a single line to get a useful
> trace point.

Yes, I figured that: you're basically using the function call generating 
code to make an array of unsigned from the arguments. But if at some 
point in the future you decide that you need to pack a record tighter 
(to make records smaller or to fit more in a single record), then won't 
you want a different interface?

On the other hand -- could you just abuse the va_args interface to DTRT 
anyway?  e.g. consider the following:

uint16_t a, b;
uint32_t c;
xc_tbuf_trace(xc, evt, 2, a, b, c);
-----

struct foo st;
xc_tbuf_trace(xc, evt, ROUND_UP(sizeof(st), sizeof(unsigned)), st);

----

1. Would these work?
2. Is this an acceptable thing to do?

If so, it certainly makes the code a lot cleaner...

  -George

Re: [PATCH 0/3] xen: guest tracing improvements

[David Vrabel]

On 25/07/13 14:22, David Vrabel wrote:
> There exists an existing hypercall sub-op (HVMOP_xentrace) for adding
> trace records.  This series makes some minor improvements and adds a
> libxc function to call it.

George, I think this is your area.  Could you review please?

David

Re: [PATCH 0/3] xen: guest tracing improvements

[George Dunlap]

On 06/08/13 15:43, David Vrabel wrote:
> On 25/07/13 14:22, David Vrabel wrote:
>> There exists an existing hypercall sub-op (HVMOP_xentrace) for adding
>> trace records.  This series makes some minor improvements and adds a
>> libxc function to call it.
> George, I think this is your area.  Could you review please?

It's on my short list.

  -George