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
David Vrabel
2013-Jul-25 13:23 UTC
[PATCH 1/3] trace: include timestamp in trace records added by HVMOP_xentrace
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
David Vrabel
2013-Jul-25 13:23 UTC
[PATCH 2/3] trace: allow HVMOP_xentrace to set trace record subclass
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
David Vrabel
2013-Jul-25 13:23 UTC
[PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records
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
David Vrabel
2013-Jul-25 13:39 UTC
Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records
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
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
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
George Dunlap
2013-Aug-15 14:15 UTC
Re: [PATCH 1/3] trace: include timestamp in trace records added by HVMOP_xentrace
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
George Dunlap
2013-Aug-15 14:18 UTC
Re: [PATCH 2/3] trace: allow HVMOP_xentrace to set trace record subclass
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
George Dunlap
2013-Aug-15 14:37 UTC
Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records
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*/,
David Vrabel
2013-Aug-15 14:39 UTC
Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records
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
George Dunlap
2013-Aug-15 14:48 UTC
Re: [PATCH 3/3] libxc: add xc_tbuf_trace() to insert trace records
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