[Qemu-devel] [PATCH] exec: Rename and fix trace events for tracing I/O port access.

[Qemu-devel] [PATCH] exec: Rename and fix trace events for tracing I/O port access.

[Richard W.M. Jones]

Back in the day you used to be able to set DEBUG_IOPORT in ioport.c
and get qemu to dump what (x86) I/O ports were being accessed by the
guest.  This was rather useful for finding out what closed source
device drivers were up to.

Now you're supposed to use cpu_in/cpu_out tracepoints instead.
However for the majority of guests these tracepoints will never be
called.

So this patch tries to rationalize all of that.  It:

 - replaces cpu_in/cpu_out with ioport_in/ioport_out tracepoints

 - moves them down in the stack, so they actually get called

 - fixes various details like address size

It turns out this is still not particularly useful for debugging
because (a) it creates massive amounts of log messages and (b) there's
no way to select a range of addresses or a device of interest.  For
example, if you have a serial port, everything else gets swamped by
I/O access to the serial port.  Maybe using a different tracing
backend (eg. stap) would help?

Anyway, it still seems to me to be an improvement over the current
situation.

Rich.

[Qemu-devel] [PATCH] exec: Rename and fix trace events for tracing I/O port access.

[Richard W.M. Jones]

cpu_in and cpu_out replaced the old DEBUG_IOPORT mechanism.  However
they weren't very useful - for most guests neither tracepoint would
ever fire (even when using TCG).

This commit moves the tracepoints down the stack into exec.c, and
renames them as ioport_in/ioport_out so it's more obvious what they
do.

Also:
 - they now work for 64 bit accesses
 - print the read/written value in hexadecimal

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 exec.c       | 10 +++++++++-
 ioport.c     |  7 -------
 trace-events |  6 +++---
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/exec.c b/exec.c
index f398d21..7477eea 100644
--- a/exec.c
+++ b/exec.c
@@ -42,8 +42,8 @@
 #include <qemu.h>
 #else /* !CONFIG_USER_ONLY */
 #include "sysemu/xen-mapcache.h"
+#endif
 #include "trace.h"
-#endif
 #include "exec/cpu-all.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/main-loop.h"
@@ -2592,24 +2592,28 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
             case 8:
                 /* 64 bit write access */
                 val = ldq_p(buf);
+                trace_ioport_out(addr, 'q', val);
                 result |= memory_region_dispatch_write(mr, addr1, val, 8,
                                                        attrs);
                 break;
             case 4:
                 /* 32 bit write access */
                 val = ldl_p(buf);
+                trace_ioport_out(addr, 'l', val & 0xffffffff);
                 result |= memory_region_dispatch_write(mr, addr1, val, 4,
                                                        attrs);
                 break;
             case 2:
                 /* 16 bit write access */
                 val = lduw_p(buf);
+                trace_ioport_out(addr, 'w', val & 0xffff);
                 result |= memory_region_dispatch_write(mr, addr1, val, 2,
                                                        attrs);
                 break;
             case 1:
                 /* 8 bit write access */
                 val = ldub_p(buf);
+                trace_ioport_out(addr, 'b', val & 0xff);
                 result |= memory_region_dispatch_write(mr, addr1, val, 1,
                                                        attrs);
                 break;
@@ -2685,24 +2689,28 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
                 /* 64 bit read access */
                 result |= memory_region_dispatch_read(mr, addr1, &val, 8,
                                                       attrs);
+                trace_ioport_in(addr, 'q', val);
                 stq_p(buf, val);
                 break;
             case 4:
                 /* 32 bit read access */
                 result |= memory_region_dispatch_read(mr, addr1, &val, 4,
                                                       attrs);
+                trace_ioport_in(addr, 'l', val & 0xffffffff);
                 stl_p(buf, val);
                 break;
             case 2:
                 /* 16 bit read access */
                 result |= memory_region_dispatch_read(mr, addr1, &val, 2,
                                                       attrs);
+                trace_ioport_in(addr, 'w', val & 0xffff);
                 stw_p(buf, val);
                 break;
             case 1:
                 /* 8 bit read access */
                 result |= memory_region_dispatch_read(mr, addr1, &val, 1,
                                                       attrs);
+                trace_ioport_in(addr, 'b', val & 0xff);
                 stb_p(buf, val);
                 break;
             default:
diff --git a/ioport.c b/ioport.c
index 7a84d54..2f8ee9d 100644
--- a/ioport.c
+++ b/ioport.c
@@ -27,7 +27,6 @@
 
 #include "qemu/osdep.h"
 #include "exec/ioport.h"
-#include "trace.h"
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
 
@@ -55,7 +54,6 @@ const MemoryRegionOps unassigned_io_ops = {
 
 void cpu_outb(pio_addr_t addr, uint8_t val)
 {
-    trace_cpu_out(addr, 'b', val);
     address_space_write(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
                         &val, 1);
 }
@@ -64,7 +62,6 @@ void cpu_outw(pio_addr_t addr, uint16_t val)
 {
     uint8_t buf[2];
 
-    trace_cpu_out(addr, 'w', val);
     stw_p(buf, val);
     address_space_write(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
                         buf, 2);
@@ -74,7 +71,6 @@ void cpu_outl(pio_addr_t addr, uint32_t val)
 {
     uint8_t buf[4];
 
-    trace_cpu_out(addr, 'l', val);
     stl_p(buf, val);
     address_space_write(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
                         buf, 4);
@@ -86,7 +82,6 @@ uint8_t cpu_inb(pio_addr_t addr)
 
     address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
                        &val, 1);
-    trace_cpu_in(addr, 'b', val);
     return val;
 }
 
@@ -97,7 +92,6 @@ uint16_t cpu_inw(pio_addr_t addr)
 
     address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED, buf, 2);
     val = lduw_p(buf);
-    trace_cpu_in(addr, 'w', val);
     return val;
 }
 
@@ -108,7 +102,6 @@ uint32_t cpu_inl(pio_addr_t addr)
 
     address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED, buf, 4);
     val = ldl_p(buf);
-    trace_cpu_in(addr, 'l', val);
     return val;
 }
 
diff --git a/trace-events b/trace-events
index d494de1..2759412 100644
--- a/trace-events
+++ b/trace-events
@@ -136,9 +136,9 @@ thread_pool_cancel(void *req, void *opaque) "req %p opaque %p"
 paio_submit_co(int64_t sector_num, int nb_sectors, int type) "sector_num %"PRId64" nb_sectors %d type %d"
 paio_submit(void *acb, void *opaque, int64_t sector_num, int nb_sectors, int type) "acb %p opaque %p sector_num %"PRId64" nb_sectors %d type %d"
 
-# ioport.c
-cpu_in(unsigned int addr, char size, unsigned int val) "addr %#x(%c) value %u"
-cpu_out(unsigned int addr, char size, unsigned int val) "addr %#x(%c) value %u"
+# exec.c
+ioport_in(uint64_t addr, char size, uint64_t val) "addr %#"PRIx64"(%c) value %#"PRIx64
+ioport_out(uint64_t addr, char size, uint64_t val) "addr %#"PRIx64"(%c) value %#"PRIx64
 
 # balloon.c
 # Since requests are raised via monitor, not many tracepoints are needed.
-- 
2.7.4

Re: [Qemu-devel] [PATCH] exec: Rename and fix trace events for tracing I/O port access.

[Daniel P. Berrange]

On Tue, Mar 29, 2016 at 04:02:24PM +0100, Richard W.M. Jones wrote:
> Back in the day you used to be able to set DEBUG_IOPORT in ioport.c
> and get qemu to dump what (x86) I/O ports were being accessed by the
> guest.  This was rather useful for finding out what closed source
> device drivers were up to.
> 
> Now you're supposed to use cpu_in/cpu_out tracepoints instead.
> However for the majority of guests these tracepoints will never be
> called.
> 
> So this patch tries to rationalize all of that.  It:
> 
>  - replaces cpu_in/cpu_out with ioport_in/ioport_out tracepoints
> 
>  - moves them down in the stack, so they actually get called
> 
>  - fixes various details like address size
> 
> It turns out this is still not particularly useful for debugging
> because (a) it creates massive amounts of log messages and (b) there's
> no way to select a range of addresses or a device of interest.  For
> example, if you have a serial port, everything else gets swamped by
> I/O access to the serial port.  Maybe using a different tracing
> backend (eg. stap) would help?

With ftrace the granularity is simply on/off printf on a per-tracepoint
basis. With dtrace/systemtap you provide a hook that does whatever it
wants per-tracepoint. So assuming the arguments passed with the tracepoint
have the info, you can filter so it only prints on the ones you care about.

eg you've defined this new probe

 ioport_in(uint64_t addr, char size, uint64_t val) "addr %#"PRIx64"(%c) value %#"PRIx64
 ioport_out(uint64_t addr, char size, uint64_t val) "addr %#"PRIx64"(%c) value %#"PRIx64


so with systemtap you would  do

  probe qemu.ioport_in {
      printf("addr=%p size=%d val=%llu\n", addr, size, val)
  }

to get the same output as ftrace, but to filter it you would add
a conditional


  probe qemu.ioport_in {
      if (addr == 0xdeadbeef) {
        printf("addr=%p size=%d val=%llu\n", addr, size, val);
      }
  }

...assuming you have an easy way to figure out the correct value
of addr you want.

Since you have global state with systemtap you could record parameters
you want in one probe and reference them in later probes. eg you could
stick a probe on the serial port emulation which registers the i/O
port address to record the address associated with the serial port,
then use this address in the ioport_in probe.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH] exec: Rename and fix trace events for tracing I/O port access.

[Paolo Bonzini]

On 29/03/2016 17:02, Richard W.M. Jones wrote:
> Back in the day you used to be able to set DEBUG_IOPORT in ioport.c
> and get qemu to dump what (x86) I/O ports were being accessed by the
> guest.  This was rather useful for finding out what closed source
> device drivers were up to.
> 
> Now you're supposed to use cpu_in/cpu_out tracepoints instead.
> However for the majority of guests these tracepoints will never be
> called.

For what it's worth, the breakage dated to way before DEBUG_IOPORT was
converted to tracepoints.  (This is why putting tracing code behind #if
0 is bad).

It happened when I/O ports stopped being special snowflakes, and started
being just like any other MMIO target (except they live in another
address space).

> It turns out this is still not particularly useful for debugging
> because (a) it creates massive amounts of log messages and (b) there's
> no way to select a range of addresses or a device of interest.  For
> example, if you have a serial port, everything else gets swamped by
> I/O access to the serial port.

Am I wrong that this is not any better/worse than "-d ioport" used to be?

> Maybe using a different tracing backend (eg. stap) would help?

You are actually logging every memory write (except stuff that it's
DMA'd from/to block devices directly by passing a guest memory pointer
to preadv/pwritev).  That's way more than just I/O ports.

However I understand that it's useful to treat them as special
snowflakes for the purpose of tracing.  To fix the problem with your
patch, I suggest to:

- consolidate the six tracepoints in 2 (cpu_in and cpu_out) that take
the size as an extra argument

- keep the existing calls to the tracepoints, and add more calls in
kvm_handle_io and in the helpers in target-i386/misc_helper.c.

Paolo

Re: [Qemu-devel] [PATCH] exec: Rename and fix trace events for tracing I/O port access.

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Tue, Mar 29, 2016 at 04:02:24PM +0100, Richard W.M. Jones wrote:
> Back in the day you used to be able to set DEBUG_IOPORT in ioport.c
> and get qemu to dump what (x86) I/O ports were being accessed by the
> guest.  This was rather useful for finding out what closed source
> device drivers were up to.
> 
> Now you're supposed to use cpu_in/cpu_out tracepoints instead.
> However for the majority of guests these tracepoints will never be
> called.
> 
> So this patch tries to rationalize all of that.  It:
> 
>  - replaces cpu_in/cpu_out with ioport_in/ioport_out tracepoints
> 
>  - moves them down in the stack, so they actually get called
> 
>  - fixes various details like address size
> 
> It turns out this is still not particularly useful for debugging
> because (a) it creates massive amounts of log messages and (b) there's
> no way to select a range of addresses or a device of interest.  For
> example, if you have a serial port, everything else gets swamped by
> I/O access to the serial port.  Maybe using a different tracing
> backend (eg. stap) would help?
> 
> Anyway, it still seems to me to be an improvement over the current
> situation.

I use "perf -e kvm:kvm_pio" but it's worth fixing QEMU's own trace
events.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]