This series improves the usefulness of xentrace when tracing hypercalls: some hypercall arguments are added the trace record and the subcalls in multicalls are traced. xenalyze requires some patches to decode the new trace record format, these patches will be posted shortly. Changes since v3: - Trace arguments for a few more hypercalls (vcpu_op and sched_op). Changes since v2: - Changed all PV events to use a different subclass. - Put multicall subcalls into their own subclass (so they can be filtered out). - Use 12 bits to report which arguments are present in the PV_HYPERCALL_V2 trace record. David
David Vrabel
2012-Oct-01 17:47 UTC
[PATCH 1/3] trace: allow for different sub-classes of TRC_PV_* tracepoints
From: David Vrabel <david.vrabel@citrix.com>
We want to add additional sub-classes for TRC_PV tracepoints and to be
able to only capture these new sub-classes. This cannot currently be
done as the existing tracepoints all use a sub-class of 0xf.
So, redefine the PV events to use a new sub-class. All the current
tracepoints are tracing entry points to the hypervisor so the
sub-class is named TRC_PV_ENTRY.
This change does not affect xenalyze as that only looks at the main
class and the event number and does not use the sub-class field.
Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
---
tools/xentrace/formats | 44 ++++++++++++++++++++++----------------------
xen/include/public/trace.h | 35 +++++++++++++++++++++--------------
2 files changed, 43 insertions(+), 36 deletions(-)
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index 04e54d5..71220c0 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -82,28 +82,28 @@
0x0010f002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_unmap [ domid =
%(1)d ]
0x0010f003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_transfer [ domid =
%(1)d ]
-0x0020f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ eip = 0x%(1)08x,
eax = 0x%(2)08x ]
-0x0020f101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ rip =
0x%(2)08x%(1)08x, eax = 0x%(3)08x ]
-0x0020f003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ eip = 0x%(1)08x,
trapnr:error = 0x%(2)08x ]
-0x0020f103 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ rip =
0x%(2)08x%(1)08x, trapnr:error = 0x%(3)08x ]
-0x0020f004 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ eip = 0x%(1)08x,
addr = 0x%(2)08x, error = 0x%(3)08x ]
-0x0020f104 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ rip =
0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x, error = 0x%(5)08x ]
-0x0020f005 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ eip =
0x%(1)08x ]
-0x0020f105 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ rip =
0x%(2)08x%(1)08x ]
-0x0020f006 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ eip =
0x%(1)08x ]
-0x0020f106 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ rip =
0x%(2)08x%(1)08x ]
-0x0020f007 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ eip =
0x%(1)08x ]
-0x0020f107 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ rip =
0x%(2)08x%(1)08x ]
-0x0020f008 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore
-0x0020f108 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore
-0x0020f009 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ eip =
0x%(1)08x, addr = 0x%(2)08x ]
-0x0020f109 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ rip =
0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x ]
-0x0020f00a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ eip =
0x%(1)08x, offset = 0x%(2)08x ]
-0x0020f10a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ rip =
0x%(2)08x%(1)08x, offset = 0x%(4)08x%(3)08x ]
-0x0020f00b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr =
0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ]
-0x0020f10b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr =
0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ]
-0x0020f00c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ]
-0x0020f10c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ]
+0x00201001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ eip = 0x%(1)08x,
eax = 0x%(2)08x ]
+0x00201101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ rip =
0x%(2)08x%(1)08x, eax = 0x%(3)08x ]
+0x00201003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ eip = 0x%(1)08x,
trapnr:error = 0x%(2)08x ]
+0x00201103 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ rip =
0x%(2)08x%(1)08x, trapnr:error = 0x%(3)08x ]
+0x00201004 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ eip = 0x%(1)08x,
addr = 0x%(2)08x, error = 0x%(3)08x ]
+0x00201104 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ rip =
0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x, error = 0x%(5)08x ]
+0x00201005 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ eip =
0x%(1)08x ]
+0x00201105 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ rip =
0x%(2)08x%(1)08x ]
+0x00201006 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ eip =
0x%(1)08x ]
+0x00201106 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ rip =
0x%(2)08x%(1)08x ]
+0x00201007 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ eip =
0x%(1)08x ]
+0x00201107 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ rip =
0x%(2)08x%(1)08x ]
+0x00201008 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore
+0x00201108 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore
+0x00201009 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ eip =
0x%(1)08x, addr = 0x%(2)08x ]
+0x00201109 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ rip =
0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x ]
+0x0020100a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ eip =
0x%(1)08x, offset = 0x%(2)08x ]
+0x0020110a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ rip =
0x%(2)08x%(1)08x, offset = 0x%(4)08x%(3)08x ]
+0x0020100b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr =
0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ]
+0x0020110b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr =
0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ]
+0x0020100c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ]
+0x0020110c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ]
0x0040f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow
[ gl1e = 0x%(2)08x%(1)08x, va = 0x%(3)08x, flags = 0x%(4)08x ]
0x0040f101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow
[ gl1e = 0x%(2)08x%(1)08x, va = 0x%(4)08x%(3)08x, flags = 0x%(5)08x ]
diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index 0dfabe9..1f154bb 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -94,20 +94,19 @@
#define TRC_MEM_POD_ZERO_RECLAIM (TRC_MEM + 17)
#define TRC_MEM_POD_SUPERPAGE_SPLINTER (TRC_MEM + 18)
-
-#define TRC_PV_HYPERCALL (TRC_PV + 1)
-#define TRC_PV_TRAP (TRC_PV + 3)
-#define TRC_PV_PAGE_FAULT (TRC_PV + 4)
-#define TRC_PV_FORCED_INVALID_OP (TRC_PV + 5)
-#define TRC_PV_EMULATE_PRIVOP (TRC_PV + 6)
-#define TRC_PV_EMULATE_4GB (TRC_PV + 7)
-#define TRC_PV_MATH_STATE_RESTORE (TRC_PV + 8)
-#define TRC_PV_PAGING_FIXUP (TRC_PV + 9)
-#define TRC_PV_GDT_LDT_MAPPING_FAULT (TRC_PV + 10)
-#define TRC_PV_PTWR_EMULATION (TRC_PV + 11)
-#define TRC_PV_PTWR_EMULATION_PAE (TRC_PV + 12)
- /* Indicates that addresses in trace record are 64 bits */
-#define TRC_64_FLAG (0x100)
+#define TRC_PV_ENTRY 0x00201000 /* Hypervisor entry points for PV guests. */
+
+#define TRC_PV_HYPERCALL (TRC_PV_ENTRY + 1)
+#define TRC_PV_TRAP (TRC_PV_ENTRY + 3)
+#define TRC_PV_PAGE_FAULT (TRC_PV_ENTRY + 4)
+#define TRC_PV_FORCED_INVALID_OP (TRC_PV_ENTRY + 5)
+#define TRC_PV_EMULATE_PRIVOP (TRC_PV_ENTRY + 6)
+#define TRC_PV_EMULATE_4GB (TRC_PV_ENTRY + 7)
+#define TRC_PV_MATH_STATE_RESTORE (TRC_PV_ENTRY + 8)
+#define TRC_PV_PAGING_FIXUP (TRC_PV_ENTRY + 9)
+#define TRC_PV_GDT_LDT_MAPPING_FAULT (TRC_PV_ENTRY + 10)
+#define TRC_PV_PTWR_EMULATION (TRC_PV_ENTRY + 11)
+#define TRC_PV_PTWR_EMULATION_PAE (TRC_PV_ENTRY + 12)
#define TRC_SHADOW_NOT_SHADOW (TRC_SHADOW + 1)
#define TRC_SHADOW_FAST_PROPAGATE (TRC_SHADOW + 2)
@@ -187,6 +186,14 @@
#define TRC_HW_IRQ_UNMAPPED_VECTOR (TRC_HW_IRQ + 0x7)
#define TRC_HW_IRQ_HANDLED (TRC_HW_IRQ + 0x8)
+/*
+ * Event Flags
+ *
+ * Some events (e.g, TRC_PV_TRAP and TRC_HVM_IOMEM_READ) have multiple
+ * record formats. These event flags distinguish between the
+ * different formats.
+ */
+#define TRC_64_FLAG 0x100 /* Addresses are 64 bits (instead of 32 bits) */
/* This structure represents a single trace buffer record. */
struct t_rec {
--
1.7.2.5
David Vrabel
2012-Oct-01 17:47 UTC
[PATCH 2/3] trace: improve usefulness of hypercall trace record
From: David Vrabel <david.vrabel@citrix.com>
Trace hypercalls using a more useful trace record format.
The EIP field is removed (it was always somewhere in the hypercall
page) and include selected hypercall arguments (e.g., the number of
calls in a multicall, and the number of PTE updates in an mmu_update
etc.). 12 bits in the first extra word are used to indicate which
arguments are present in the record and what size they are (32 or
64-bit).
This is an incompatible record format so a new event ID is used so
tools can distinguish between the two formats.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
---
tools/xentrace/formats | 1 +
tools/xentrace/xentrace_format | 6 ++++
xen/arch/x86/trace.c | 35 +++++++++++---------------
xen/common/trace.c | 52 ++++++++++++++++++++++++++++++++++++++++
xen/include/public/trace.h | 30 +++++++++++++++++++++++
xen/include/xen/trace.h | 2 +
6 files changed, 106 insertions(+), 20 deletions(-)
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index 71220c0..fa935c8 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -104,6 +104,7 @@
0x0020110b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr =
0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ]
0x0020100c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ]
0x0020110c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ]
+0x0020100d CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ op = 0x%(1)08x ]
0x0040f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow
[ gl1e = 0x%(2)08x%(1)08x, va = 0x%(3)08x, flags = 0x%(4)08x ]
0x0040f101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow
[ gl1e = 0x%(2)08x%(1)08x, va = 0x%(4)08x%(3)08x, flags = 0x%(5)08x ]
diff --git a/tools/xentrace/xentrace_format b/tools/xentrace/xentrace_format
index e13b05b..bdcab09 100644
--- a/tools/xentrace/xentrace_format
+++ b/tools/xentrace/xentrace_format
@@ -111,6 +111,8 @@ D7REC = "IIIIIII"
last_tsc = [0]
TRC_TRACE_IRQ = 0x1f004
+TRC_PV_HYPERCALL_V2 = 0x20100d
+
NR_VECTORS = 256
irq_measure = [{''count'':0, ''tot_cycles'':0,
''max_cycles'':0}] * NR_VECTORS
@@ -197,6 +199,10 @@ while not interrupted:
d3 = irq_measure[d1][''tot_cycles'']
d4 = irq_measure[d1][''max_cycles'']
+ if event == TRC_PV_HYPERCALL_V2:
+ # Mask off the argument present bits.
+ d1 &= 0x000fffff
+
#tsc = (tscH<<32) | tscL
#print i, tsc
diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c
index 27fe150..f2c75bc 100644
--- a/xen/arch/x86/trace.c
+++ b/xen/arch/x86/trace.c
@@ -9,33 +9,28 @@
void trace_hypercall(void)
{
struct cpu_user_regs *regs = guest_cpu_user_regs();
+ unsigned long args[6];
if ( is_pv_32on64_vcpu(current) )
{
- struct {
- u32 eip,eax;
- } __attribute__((packed)) d;
-
- d.eip = regs->eip;
- d.eax = regs->eax;
-
- __trace_var(TRC_PV_HYPERCALL, 1, sizeof(d), &d);
+ args[0] = regs->ebx;
+ args[1] = regs->ecx;
+ args[2] = regs->edx;
+ args[3] = regs->esi;
+ args[4] = regs->edi;
+ args[5] = regs->ebp;
}
else
{
- struct {
- unsigned long eip;
- u32 eax;
- } __attribute__((packed)) d;
- u32 event;
-
- event = TRC_PV_HYPERCALL;
- event |= TRC_64_FLAG;
- d.eip = regs->eip;
- d.eax = regs->eax;
-
- __trace_var(event, 1/*tsc*/, sizeof(d), &d);
+ args[0] = regs->rdi;
+ args[1] = regs->rsi;
+ args[2] = regs->rdx;
+ args[3] = regs->r10;
+ args[4] = regs->r8;
+ args[5] = regs->r9;
}
+
+ __trace_hypercall(regs->eax, args);
}
void __trace_pv_trap(int trapnr, unsigned long eip,
diff --git a/xen/common/trace.c b/xen/common/trace.c
index cacaeb2..57a5a36 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -816,6 +816,58 @@ unlock:
tasklet_schedule(&trace_notify_dom0_tasklet);
}
+void __trace_hypercall(unsigned long op, const unsigned long *args)
+{
+ struct {
+ uint32_t op;
+ uint32_t args[6];
+ } __attribute__((packed)) d;
+ uint32_t *a = d.args;
+
+#define APPEND_ARG32(i) \
+ do { \
+ unsigned i_ = (i); \
+ *a++ = args[(i_)]; \
+ d.op |= TRC_PV_HYPERCALL_V2_ARG_32(i_); \
+ } while( 0 )
+
+ /*
+ * This shouldn''t happen as @op should be small enough but just in
+ * case, warn if the argument bits in the trace record would
+ * clobber the hypercall op.
+ */
+ WARN_ON(op & TRC_PV_HYPERCALL_V2_ARG_MASK);
+
+ d.op = op;
+
+ switch ( op )
+ {
+ case __HYPERVISOR_mmu_update:
+ APPEND_ARG32(1); /* count */
+ break;
+ case __HYPERVISOR_multicall:
+ APPEND_ARG32(1); /* count */
+ break;
+ case __HYPERVISOR_grant_table_op:
+ APPEND_ARG32(0); /* cmd */
+ APPEND_ARG32(2); /* count */
+ break;
+ case __HYPERVISOR_vcpu_op:
+ APPEND_ARG32(0); /* cmd */
+ APPEND_ARG32(1); /* vcpuid */
+ break;
+ case __HYPERVISOR_mmuext_op:
+ APPEND_ARG32(1); /* count */
+ break;
+ case __HYPERVISOR_sched_op:
+ APPEND_ARG32(0); /* cmd */
+ break;
+ }
+
+ __trace_var(TRC_PV_HYPERCALL_V2, 1,
+ sizeof(uint32_t) * (1 + (a - d.args)), &d);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index 1f154bb..ef43b23 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -107,6 +107,36 @@
#define TRC_PV_GDT_LDT_MAPPING_FAULT (TRC_PV_ENTRY + 10)
#define TRC_PV_PTWR_EMULATION (TRC_PV_ENTRY + 11)
#define TRC_PV_PTWR_EMULATION_PAE (TRC_PV_ENTRY + 12)
+#define TRC_PV_HYPERCALL_V2 (TRC_PV_ENTRY + 13)
+
+/*
+ * TRC_PV_HYPERCALL_V2 format
+ *
+ * Only some of the hypercall argument are recorded. Bit fields A0 to
+ * A5 in the first extra word are set if the argument is present and
+ * the arguments themselves are packed sequentially in the following
+ * words.
+ *
+ * The TRC_64_FLAG bit is not set for these events (even if there are
+ * 64-bit arguments in the record).
+ *
+ * Word
+ * 0 bit 31 30|29 28|27 26|25 24|23 22|21 20|19 ... 0
+ * A5 |A4 |A3 |A2 |A1 |A0 |Hypercall op
+ * 1 First 32 bit (or low word of first 64 bit) arg in record
+ * 2 Second 32 bit (or high word of first 64 bit) arg in record
+ * ...
+ *
+ * A0-A5 bitfield values:
+ *
+ * 00b Argument not present
+ * 01b 32-bit argument present
+ * 10b 64-bit argument present
+ * 11b Reserved
+ */
+#define TRC_PV_HYPERCALL_V2_ARG_32(i) (0x1 << (20 + 2*(i)))
+#define TRC_PV_HYPERCALL_V2_ARG_64(i) (0x2 << (20 + 2*(i)))
+#define TRC_PV_HYPERCALL_V2_ARG_MASK (0xfff00000)
#define TRC_SHADOW_NOT_SHADOW (TRC_SHADOW + 1)
#define TRC_SHADOW_FAST_PROPAGATE (TRC_SHADOW + 2)
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index b32f6c5..f601aeb 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -44,6 +44,8 @@ static inline void trace_var(u32 event, int cycles, int extra,
__trace_var(event, cycles, extra, extra_data);
}
+void __trace_hypercall(unsigned long call, const unsigned long *args);
+
/* Convenience macros for calling the trace function. */
#define TRACE_0D(_e) \
do { \
--
1.7.2.5
From: David Vrabel <david.vrabel@citrix.com>
Add a trace record for every hypercall inside a multicall. These use
a new event ID (with a different sub-class ) so they may be filtered
out if only the calls into hypervisor are of interest.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
---
tools/xentrace/formats | 1 +
tools/xentrace/xentrace_format | 3 ++-
xen/arch/x86/trace.c | 2 +-
xen/common/compat/multicall.c | 12 ++++++++++++
xen/common/multicall.c | 16 ++++++++++++++++
xen/common/trace.c | 6 +++---
xen/include/public/trace.h | 4 +++-
xen/include/xen/trace.h | 3 ++-
8 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index fa935c8..928e1d7 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -105,6 +105,7 @@
0x0020100c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ]
0x0020110c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr =
0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ]
0x0020100d CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ op = 0x%(1)08x ]
+0x0020200e CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ op = 0x%(1)08x ]
0x0040f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow
[ gl1e = 0x%(2)08x%(1)08x, va = 0x%(3)08x, flags = 0x%(4)08x ]
0x0040f101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow
[ gl1e = 0x%(2)08x%(1)08x, va = 0x%(4)08x%(3)08x, flags = 0x%(5)08x ]
diff --git a/tools/xentrace/xentrace_format b/tools/xentrace/xentrace_format
index bdcab09..5ff85ae 100644
--- a/tools/xentrace/xentrace_format
+++ b/tools/xentrace/xentrace_format
@@ -112,6 +112,7 @@ last_tsc = [0]
TRC_TRACE_IRQ = 0x1f004
TRC_PV_HYPERCALL_V2 = 0x20100d
+TRC_PV_HYPERCALL_SUBCALL = 0x20100e
NR_VECTORS = 256
irq_measure = [{''count'':0, ''tot_cycles'':0,
''max_cycles'':0}] * NR_VECTORS
@@ -199,7 +200,7 @@ while not interrupted:
d3 = irq_measure[d1][''tot_cycles'']
d4 = irq_measure[d1][''max_cycles'']
- if event == TRC_PV_HYPERCALL_V2:
+ if event == TRC_PV_HYPERCALL_V2 or event == TRC_PV_HYPERCALL_SUBCALL:
# Mask off the argument present bits.
d1 &= 0x000fffff
diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c
index f2c75bc..982ca88 100644
--- a/xen/arch/x86/trace.c
+++ b/xen/arch/x86/trace.c
@@ -30,7 +30,7 @@ void trace_hypercall(void)
args[5] = regs->r9;
}
- __trace_hypercall(regs->eax, args);
+ __trace_hypercall(TRC_PV_HYPERCALL_V2, regs->eax, args);
}
void __trace_pv_trap(int trapnr, unsigned long eip,
diff --git a/xen/common/compat/multicall.c b/xen/common/compat/multicall.c
index 0eb1212..e7e2a40 100644
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -5,6 +5,7 @@
#include <xen/config.h>
#include <xen/types.h>
#include <xen/multicall.h>
+#include <xen/trace.h>
#define COMPAT
typedef int ret_t;
@@ -25,6 +26,17 @@ DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t);
#define do_multicall(l, n) compat_multicall(_##l, n)
#define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
+static void __trace_multicall_call(multicall_entry_t *call)
+{
+ unsigned long args[6];
+ int i;
+
+ for ( i = 0; i < ARRAY_SIZE(args); i++ )
+ args[i] = call->args[i];
+
+ __trace_hypercall(TRC_PV_HYPERCALL_SUBCALL, call->op, args);
+}
+
#include "../multicall.c"
/*
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index 6c1a9d7..ca1839d 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -11,14 +11,28 @@
#include <xen/multicall.h>
#include <xen/guest_access.h>
#include <xen/perfc.h>
+#include <xen/trace.h>
#include <asm/current.h>
#include <asm/hardirq.h>
#ifndef COMPAT
typedef long ret_t;
#define xlat_multicall_entry(mcs)
+
+static void __trace_multicall_call(multicall_entry_t *call)
+{
+ __trace_hypercall(TRC_PV_HYPERCALL_SUBCALL, call->op, call->args);
+}
#endif
+static void trace_multicall_call(multicall_entry_t *call)
+{
+ if ( !tb_init_done )
+ return;
+
+ __trace_multicall_call(call);
+}
+
ret_t
do_multicall(
XEN_GUEST_HANDLE(multicall_entry_t) call_list, unsigned int nr_calls)
@@ -47,6 +61,8 @@ do_multicall(
break;
}
+ trace_multicall_call(&mcs->call);
+
do_multicall_call(&mcs->call);
#ifndef NDEBUG
diff --git a/xen/common/trace.c b/xen/common/trace.c
index 57a5a36..ebb60df 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -816,7 +816,8 @@ unlock:
tasklet_schedule(&trace_notify_dom0_tasklet);
}
-void __trace_hypercall(unsigned long op, const unsigned long *args)
+void __trace_hypercall(uint32_t event, unsigned long op,
+ const unsigned long *args)
{
struct {
uint32_t op;
@@ -864,8 +865,7 @@ void __trace_hypercall(unsigned long op, const unsigned long
*args)
break;
}
- __trace_var(TRC_PV_HYPERCALL_V2, 1,
- sizeof(uint32_t) * (1 + (a - d.args)), &d);
+ __trace_var(event, 1, sizeof(uint32_t) * (1 + (a - d.args)), &d);
}
/*
diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index ef43b23..3c93805 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -94,7 +94,8 @@
#define TRC_MEM_POD_ZERO_RECLAIM (TRC_MEM + 17)
#define TRC_MEM_POD_SUPERPAGE_SPLINTER (TRC_MEM + 18)
-#define TRC_PV_ENTRY 0x00201000 /* Hypervisor entry points for PV guests. */
+#define TRC_PV_ENTRY 0x00201000 /* Hypervisor entry points for PV guests. */
+#define TRC_PV_SUBCALL 0x00202000 /* Sub-call in a multicall hypercall */
#define TRC_PV_HYPERCALL (TRC_PV_ENTRY + 1)
#define TRC_PV_TRAP (TRC_PV_ENTRY + 3)
@@ -108,6 +109,7 @@
#define TRC_PV_PTWR_EMULATION (TRC_PV_ENTRY + 11)
#define TRC_PV_PTWR_EMULATION_PAE (TRC_PV_ENTRY + 12)
#define TRC_PV_HYPERCALL_V2 (TRC_PV_ENTRY + 13)
+#define TRC_PV_HYPERCALL_SUBCALL (TRC_PV_SUBCALL + 14)
/*
* TRC_PV_HYPERCALL_V2 format
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index f601aeb..3b8a7b3 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -44,7 +44,8 @@ static inline void trace_var(u32 event, int cycles, int extra,
__trace_var(event, cycles, extra, extra_data);
}
-void __trace_hypercall(unsigned long call, const unsigned long *args);
+void __trace_hypercall(uint32_t event, unsigned long op,
+ const unsigned long *args);
/* Convenience macros for calling the trace function. */
#define TRACE_0D(_e) \
--
1.7.2.5
George Dunlap
2012-Oct-02 15:56 UTC
Re: [PATCH 1/3] trace: allow for different sub-classes of TRC_PV_* tracepoints
On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@citrix.com> wrote:> From: David Vrabel <david.vrabel@citrix.com> > > We want to add additional sub-classes for TRC_PV tracepoints and to be > able to only capture these new sub-classes. This cannot currently be > done as the existing tracepoints all use a sub-class of 0xf. > > So, redefine the PV events to use a new sub-class. All the current > tracepoints are tracing entry points to the hypervisor so the > sub-class is named TRC_PV_ENTRY. > > This change does not affect xenalyze as that only looks at the main > class and the event number and does not use the sub-class field. > > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com> > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > Acked-by: George Dunlap <george.dunlap@citrix.com>Ack re-confirmed.> --- > tools/xentrace/formats | 44 ++++++++++++++++++++++---------------------- > xen/include/public/trace.h | 35 +++++++++++++++++++++-------------- > 2 files changed, 43 insertions(+), 36 deletions(-) > > diff --git a/tools/xentrace/formats b/tools/xentrace/formats > index 04e54d5..71220c0 100644 > --- a/tools/xentrace/formats > +++ b/tools/xentrace/formats > @@ -82,28 +82,28 @@ > 0x0010f002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_unmap [ domid = %(1)d ] > 0x0010f003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_transfer [ domid = %(1)d ] > > -0x0020f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ eip = 0x%(1)08x, eax = 0x%(2)08x ] > -0x0020f101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ rip = 0x%(2)08x%(1)08x, eax = 0x%(3)08x ] > -0x0020f003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ eip = 0x%(1)08x, trapnr:error = 0x%(2)08x ] > -0x0020f103 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ rip = 0x%(2)08x%(1)08x, trapnr:error = 0x%(3)08x ] > -0x0020f004 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ eip = 0x%(1)08x, addr = 0x%(2)08x, error = 0x%(3)08x ] > -0x0020f104 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ rip = 0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x, error = 0x%(5)08x ] > -0x0020f005 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ eip = 0x%(1)08x ] > -0x0020f105 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ rip = 0x%(2)08x%(1)08x ] > -0x0020f006 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ eip = 0x%(1)08x ] > -0x0020f106 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ rip = 0x%(2)08x%(1)08x ] > -0x0020f007 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ eip = 0x%(1)08x ] > -0x0020f107 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ rip = 0x%(2)08x%(1)08x ] > -0x0020f008 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore > -0x0020f108 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore > -0x0020f009 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ eip = 0x%(1)08x, addr = 0x%(2)08x ] > -0x0020f109 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ rip = 0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x ] > -0x0020f00a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ eip = 0x%(1)08x, offset = 0x%(2)08x ] > -0x0020f10a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ rip = 0x%(2)08x%(1)08x, offset = 0x%(4)08x%(3)08x ] > -0x0020f00b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr = 0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ] > -0x0020f10b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr = 0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ] > -0x0020f00c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr = 0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ] > -0x0020f10c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr = 0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ] > +0x00201001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ eip = 0x%(1)08x, eax = 0x%(2)08x ] > +0x00201101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) hypercall [ rip = 0x%(2)08x%(1)08x, eax = 0x%(3)08x ] > +0x00201003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ eip = 0x%(1)08x, trapnr:error = 0x%(2)08x ] > +0x00201103 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) trap [ rip = 0x%(2)08x%(1)08x, trapnr:error = 0x%(3)08x ] > +0x00201004 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ eip = 0x%(1)08x, addr = 0x%(2)08x, error = 0x%(3)08x ] > +0x00201104 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_fault [ rip = 0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x, error = 0x%(5)08x ] > +0x00201005 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ eip = 0x%(1)08x ] > +0x00201105 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) forced_invalid_op [ rip = 0x%(2)08x%(1)08x ] > +0x00201006 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ eip = 0x%(1)08x ] > +0x00201106 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_privop [ rip = 0x%(2)08x%(1)08x ] > +0x00201007 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ eip = 0x%(1)08x ] > +0x00201107 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) emulate_4G [ rip = 0x%(2)08x%(1)08x ] > +0x00201008 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore > +0x00201108 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) math_state_restore > +0x00201009 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ eip = 0x%(1)08x, addr = 0x%(2)08x ] > +0x00201109 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) paging_fixup [ rip = 0x%(2)08x%(1)08x, addr = 0x%(4)08x%(3)08x ] > +0x0020100a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ eip = 0x%(1)08x, offset = 0x%(2)08x ] > +0x0020110a CPU%(cpu)d %(tsc)d (+%(reltsc)8d) gdt_ldt_mapping_fault [ rip = 0x%(2)08x%(1)08x, offset = 0x%(4)08x%(3)08x ] > +0x0020100b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr = 0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ] > +0x0020110b CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation [ addr = 0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ] > +0x0020100c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr = 0x%(3)08x, eip = 0x%(4)08x, npte = 0x%(2)08x%(1)08x ] > +0x0020110c CPU%(cpu)d %(tsc)d (+%(reltsc)8d) ptwr_emulation_pae [ addr = 0x%(4)08x%(3)08x, rip = 0x%(6)08x%(5)08x, npte = 0x%(2)08x%(1)08x ] > > 0x0040f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow [ gl1e = 0x%(2)08x%(1)08x, va = 0x%(3)08x, flags = 0x%(4)08x ] > 0x0040f101 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) shadow_not_shadow [ gl1e = 0x%(2)08x%(1)08x, va = 0x%(4)08x%(3)08x, flags = 0x%(5)08x ] > diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h > index 0dfabe9..1f154bb 100644 > --- a/xen/include/public/trace.h > +++ b/xen/include/public/trace.h > @@ -94,20 +94,19 @@ > #define TRC_MEM_POD_ZERO_RECLAIM (TRC_MEM + 17) > #define TRC_MEM_POD_SUPERPAGE_SPLINTER (TRC_MEM + 18) > > - > -#define TRC_PV_HYPERCALL (TRC_PV + 1) > -#define TRC_PV_TRAP (TRC_PV + 3) > -#define TRC_PV_PAGE_FAULT (TRC_PV + 4) > -#define TRC_PV_FORCED_INVALID_OP (TRC_PV + 5) > -#define TRC_PV_EMULATE_PRIVOP (TRC_PV + 6) > -#define TRC_PV_EMULATE_4GB (TRC_PV + 7) > -#define TRC_PV_MATH_STATE_RESTORE (TRC_PV + 8) > -#define TRC_PV_PAGING_FIXUP (TRC_PV + 9) > -#define TRC_PV_GDT_LDT_MAPPING_FAULT (TRC_PV + 10) > -#define TRC_PV_PTWR_EMULATION (TRC_PV + 11) > -#define TRC_PV_PTWR_EMULATION_PAE (TRC_PV + 12) > - /* Indicates that addresses in trace record are 64 bits */ > -#define TRC_64_FLAG (0x100) > +#define TRC_PV_ENTRY 0x00201000 /* Hypervisor entry points for PV guests. */ > + > +#define TRC_PV_HYPERCALL (TRC_PV_ENTRY + 1) > +#define TRC_PV_TRAP (TRC_PV_ENTRY + 3) > +#define TRC_PV_PAGE_FAULT (TRC_PV_ENTRY + 4) > +#define TRC_PV_FORCED_INVALID_OP (TRC_PV_ENTRY + 5) > +#define TRC_PV_EMULATE_PRIVOP (TRC_PV_ENTRY + 6) > +#define TRC_PV_EMULATE_4GB (TRC_PV_ENTRY + 7) > +#define TRC_PV_MATH_STATE_RESTORE (TRC_PV_ENTRY + 8) > +#define TRC_PV_PAGING_FIXUP (TRC_PV_ENTRY + 9) > +#define TRC_PV_GDT_LDT_MAPPING_FAULT (TRC_PV_ENTRY + 10) > +#define TRC_PV_PTWR_EMULATION (TRC_PV_ENTRY + 11) > +#define TRC_PV_PTWR_EMULATION_PAE (TRC_PV_ENTRY + 12) > > #define TRC_SHADOW_NOT_SHADOW (TRC_SHADOW + 1) > #define TRC_SHADOW_FAST_PROPAGATE (TRC_SHADOW + 2) > @@ -187,6 +186,14 @@ > #define TRC_HW_IRQ_UNMAPPED_VECTOR (TRC_HW_IRQ + 0x7) > #define TRC_HW_IRQ_HANDLED (TRC_HW_IRQ + 0x8) > > +/* > + * Event Flags > + * > + * Some events (e.g, TRC_PV_TRAP and TRC_HVM_IOMEM_READ) have multiple > + * record formats. These event flags distinguish between the > + * different formats. > + */ > +#define TRC_64_FLAG 0x100 /* Addresses are 64 bits (instead of 32 bits) */ > > /* This structure represents a single trace buffer record. */ > struct t_rec { > -- > 1.7.2.5 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
George Dunlap
2012-Oct-02 16:09 UTC
Re: [PATCH 2/3] trace: improve usefulness of hypercall trace record
On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@citrix.com> wrote:> From: David Vrabel <david.vrabel@citrix.com> > > Trace hypercalls using a more useful trace record format. > > The EIP field is removed (it was always somewhere in the hypercall > page) and include selected hypercall arguments (e.g., the number of > calls in a multicall, and the number of PTE updates in an mmu_update > etc.). 12 bits in the first extra word are used to indicate which > arguments are present in the record and what size they are (32 or > 64-bit). > > This is an incompatible record format so a new event ID is used so > tools can distinguish between the two formats. > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > Acked-by: George Dunlap <george.dunlap@citrix.com>Looking at it again, I do have one comment (see below) -- so I guess that would be technically withdrawing the Ack until we sort it out.> diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c > index 27fe150..f2c75bc 100644 > --- a/xen/arch/x86/trace.c > +++ b/xen/arch/x86/trace.c > @@ -9,33 +9,28 @@ > void trace_hypercall(void)In most places, "trace_*" means "Call unconditionally; inside it will check tb_init_done", while "__trace_*" means, "I have already checked that tb_init_done is set, don''t bother checking it." It seems not to be the case here -- entry.S checks tb_init_done before calling "trace_hypercall". Could you either: * Come up with a naming scheme consistent with the rest of the calls (e.g., __trace_hypercall_x86 and __trace_hypercall), or * Put a comment at the top of trace_hypercall() noting that it''s an exception to the general rule?> + switch ( op ) > + { > + case __HYPERVISOR_mmu_update: > + APPEND_ARG32(1); /* count */ > + break; > + case __HYPERVISOR_multicall: > + APPEND_ARG32(1); /* count */ > + break; > + case __HYPERVISOR_grant_table_op: > + APPEND_ARG32(0); /* cmd */ > + APPEND_ARG32(2); /* count */ > + break; > + case __HYPERVISOR_vcpu_op: > + APPEND_ARG32(0); /* cmd */ > + APPEND_ARG32(1); /* vcpuid */ > + break; > + case __HYPERVISOR_mmuext_op: > + APPEND_ARG32(1); /* count */ > + break; > + case __HYPERVISOR_sched_op: > + APPEND_ARG32(0); /* cmd */ > + break; > + }I may have commented on this before -- I wonder if doing some kind of array might be better than a big switch statement. I think with only a few hypercalls, a switch statement is probably acceptable; but as we add more, the code is going to get bloated. So I guess, I''ll accept it this time, but as you add more, Real Soon I''ll probably ask for a more efficient implementation. :-) Thanks, -George
George Dunlap
2012-Oct-02 16:15 UTC
Re: [PATCH 3/3] trace: trace hypercalls inside a multicall
On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@citrix.com> wrote:> From: David Vrabel <david.vrabel@citrix.com> > > Add a trace record for every hypercall inside a multicall. These use > a new event ID (with a different sub-class ) so they may be filtered > out if only the calls into hypervisor are of interest. > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> > Acked-by: George Dunlap <george.dunlap@citrix.com>Re-confirming the Ack. -George
David Vrabel
2012-Oct-02 17:06 UTC
Re: [PATCH 2/3] trace: improve usefulness of hypercall trace record
On 02/10/12 17:09, George Dunlap wrote:> On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@citrix.com> wrote: >> From: David Vrabel <david.vrabel@citrix.com> >> >> Trace hypercalls using a more useful trace record format. >> >> The EIP field is removed (it was always somewhere in the hypercall >> page) and include selected hypercall arguments (e.g., the number of >> calls in a multicall, and the number of PTE updates in an mmu_update >> etc.). 12 bits in the first extra word are used to indicate which >> arguments are present in the record and what size they are (32 or >> 64-bit). >> >> This is an incompatible record format so a new event ID is used so >> tools can distinguish between the two formats. >> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com> >> Acked-by: George Dunlap <george.dunlap@citrix.com> > > Looking at it again, I do have one comment (see below) -- so I guess > that would be technically withdrawing the Ack until we sort it out. > >> diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c >> index 27fe150..f2c75bc 100644 >> --- a/xen/arch/x86/trace.c >> +++ b/xen/arch/x86/trace.c >> @@ -9,33 +9,28 @@ >> void trace_hypercall(void) > > In most places, "trace_*" means "Call unconditionally; inside it will > check tb_init_done", while "__trace_*" means, "I have already checked > that tb_init_done is set, don''t bother checking it."This isn''t something that this patch changes but I''ll add a new patch to change this since it''s a trivial change.>> + switch ( op ) >> + { >> + case __HYPERVISOR_mmu_update: >> + APPEND_ARG32(1); /* count */ >> + break; >> + case __HYPERVISOR_multicall: >> + APPEND_ARG32(1); /* count */ >> + break; >> + case __HYPERVISOR_grant_table_op: >> + APPEND_ARG32(0); /* cmd */ >> + APPEND_ARG32(2); /* count */ >> + break; >> + case __HYPERVISOR_vcpu_op: >> + APPEND_ARG32(0); /* cmd */ >> + APPEND_ARG32(1); /* vcpuid */ >> + break; >> + case __HYPERVISOR_mmuext_op: >> + APPEND_ARG32(1); /* count */ >> + break; >> + case __HYPERVISOR_sched_op: >> + APPEND_ARG32(0); /* cmd */ >> + break; >> + } > > I may have commented on this before -- I wonder if doing some kind of > array might be better than a big switch statement. I think with only > a few hypercalls, a switch statement is probably acceptable; but as we > add more, the code is going to get bloated.I''ll see what I can come up with. David
George Dunlap
2012-Oct-03 10:01 UTC
Re: [PATCH 2/3] trace: improve usefulness of hypercall trace record
On Tue, Oct 2, 2012 at 6:06 PM, David Vrabel <david.vrabel@citrix.com> wrote:> On 02/10/12 17:09, George Dunlap wrote: >> On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@citrix.com> wrote: >>> From: David Vrabel <david.vrabel@citrix.com> >>> >>> Trace hypercalls using a more useful trace record format. >>> >>> The EIP field is removed (it was always somewhere in the hypercall >>> page) and include selected hypercall arguments (e.g., the number of >>> calls in a multicall, and the number of PTE updates in an mmu_update >>> etc.). 12 bits in the first extra word are used to indicate which >>> arguments are present in the record and what size they are (32 or >>> 64-bit). >>> >>> This is an incompatible record format so a new event ID is used so >>> tools can distinguish between the two formats. >>> >>> Signed-off-by: David Vrabel <david.vrabel@citrix.com> >>> Acked-by: George Dunlap <george.dunlap@citrix.com> >> >> Looking at it again, I do have one comment (see below) -- so I guess >> that would be technically withdrawing the Ack until we sort it out. >> >>> diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c >>> index 27fe150..f2c75bc 100644 >>> --- a/xen/arch/x86/trace.c >>> +++ b/xen/arch/x86/trace.c >>> @@ -9,33 +9,28 @@ >>> void trace_hypercall(void) >> >> In most places, "trace_*" means "Call unconditionally; inside it will >> check tb_init_done", while "__trace_*" means, "I have already checked >> that tb_init_done is set, don''t bother checking it." > > This isn''t something that this patch changes but I''ll add a new patch to > change this since it''s a trivial change.Great, thanks. -G> >>> + switch ( op ) >>> + { >>> + case __HYPERVISOR_mmu_update: >>> + APPEND_ARG32(1); /* count */ >>> + break; >>> + case __HYPERVISOR_multicall: >>> + APPEND_ARG32(1); /* count */ >>> + break; >>> + case __HYPERVISOR_grant_table_op: >>> + APPEND_ARG32(0); /* cmd */ >>> + APPEND_ARG32(2); /* count */ >>> + break; >>> + case __HYPERVISOR_vcpu_op: >>> + APPEND_ARG32(0); /* cmd */ >>> + APPEND_ARG32(1); /* vcpuid */ >>> + break; >>> + case __HYPERVISOR_mmuext_op: >>> + APPEND_ARG32(1); /* count */ >>> + break; >>> + case __HYPERVISOR_sched_op: >>> + APPEND_ARG32(0); /* cmd */ >>> + break; >>> + } >> >> I may have commented on this before -- I wonder if doing some kind of >> array might be better than a big switch statement. I think with only >> a few hypercalls, a switch statement is probably acceptable; but as we >> add more, the code is going to get bloated. > > I''ll see what I can come up with. > > David