Re: [Qemu-devel] [PATCH] ehci: avoid string arguments in trace events

2011-09-05 Thread Gerd Hoffmann

On 09/03/11 17:22, Stefan Hajnoczi wrote:

String arguments are not supported by all trace backends.  This patch
replaces existing string arguments in hw/usb-ehci.c either with
individual trace events that remain human-friendly or by printing raw
addresses when there is no alternative or downside to that.


Printing raw addresses *is* a downside.


States and usbsts bits remain human-friendly since it is hard to
remember all of them.  MMIO addresses are printed raw because they would
create many individual trace events and the addresses are usually easy
to remember when debugging.


I find it hard to rememeber them.  There is a reason why the code to 
print the names for the mmio addresses is there in the first place. I 
don't want to loose that.


Can't we just fix the backends instead?  Replacing debug fprintf with 
trace points isn't going to work if tracing can't handle strings.


cheers,
  Gerd




Re: [Qemu-devel] [PATCH] ehci: avoid string arguments in trace events

2011-09-05 Thread Stefan Hajnoczi
On Mon, Sep 5, 2011 at 9:38 AM, Gerd Hoffmann kra...@redhat.com wrote:
 On 09/03/11 17:22, Stefan Hajnoczi wrote:

 String arguments are not supported by all trace backends.  This patch
 replaces existing string arguments in hw/usb-ehci.c either with
 individual trace events that remain human-friendly or by printing raw
 addresses when there is no alternative or downside to that.

 Printing raw addresses *is* a downside.

 States and usbsts bits remain human-friendly since it is hard to
 remember all of them.  MMIO addresses are printed raw because they would
 create many individual trace events and the addresses are usually easy
 to remember when debugging.

 I find it hard to rememeber them.  There is a reason why the code to print
 the names for the mmio addresses is there in the first place. I don't want
 to loose that.

 Can't we just fix the backends instead?  Replacing debug fprintf with trace
 points isn't going to work if tracing can't handle strings.

I looked at the capabilities of the backends again and I think we
*can* allow strings.  The simple trace backend does not support them
but it's possible to add that.  I thought SystemTap doesn't either but
it turns out there is a userspace string copy function available.
Both stderr and ust are fine with strings.

Let's drop this patch.  I will update the tracing documentation.

Stefan



Re: [Qemu-devel] [PATCH] ehci: avoid string arguments in trace events

2011-09-05 Thread Gerd Hoffmann

  Hi,


Let's drop this patch.  I will update the tracing documentation.


Great.

thanks,
  Gerd



[Qemu-devel] [PATCH] ehci: avoid string arguments in trace events

2011-09-03 Thread Stefan Hajnoczi
String arguments are not supported by all trace backends.  This patch
replaces existing string arguments in hw/usb-ehci.c either with
individual trace events that remain human-friendly or by printing raw
addresses when there is no alternative or downside to that.

States and usbsts bits remain human-friendly since it is hard to
remember all of them.  MMIO addresses are printed raw because they would
create many individual trace events and the addresses are usually easy
to remember when debugging.  Queue actions remain human-friendly while
device attach only prints the USBDevice pointer.

Signed-off-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
---
 hw/usb-ehci.c |  138 +++-
 trace-events  |   40 ++---
 2 files changed, 100 insertions(+), 78 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 47a7fb9..e101fc7 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -435,93 +435,89 @@ struct EHCIState {
 *data = val; \
 } while(0)
 
-static const char *ehci_state_names[] = {
-[ EST_INACTIVE ] = INACTIVE,
-[ EST_ACTIVE ]   = ACTIVE,
-[ EST_EXECUTING ]= EXECUTING,
-[ EST_SLEEPING ] = SLEEPING,
-[ EST_WAITLISTHEAD ] = WAITLISTHEAD,
-[ EST_FETCHENTRY ]   = FETCH ENTRY,
-[ EST_FETCHQH ]  = FETCH QH,
-[ EST_FETCHITD ] = FETCH ITD,
-[ EST_ADVANCEQUEUE ] = ADVANCEQUEUE,
-[ EST_FETCHQTD ] = FETCH QTD,
-[ EST_EXECUTE ]  = EXECUTE,
-[ EST_WRITEBACK ]= WRITEBACK,
-[ EST_HORIZONTALQH ] = HORIZONTALQH,
-};
-
-static const char *ehci_mmio_names[] = {
-[ CAPLENGTH ]= CAPLENGTH,
-[ HCIVERSION ]   = HCIVERSION,
-[ HCSPARAMS ]= HCSPARAMS,
-[ HCCPARAMS ]= HCCPARAMS,
-[ USBCMD ]   = USBCMD,
-[ USBSTS ]   = USBSTS,
-[ USBINTR ]  = USBINTR,
-[ FRINDEX ]  = FRINDEX,
-[ PERIODICLISTBASE ] = P-LIST BASE,
-[ ASYNCLISTADDR ]= A-LIST ADDR,
-[ PORTSC_BEGIN ] = PORTSC #0,
-[ PORTSC_BEGIN + 4]  = PORTSC #1,
-[ PORTSC_BEGIN + 8]  = PORTSC #2,
-[ PORTSC_BEGIN + 12] = PORTSC #3,
-[ CONFIGFLAG ]   = CONFIGFLAG,
-};
-
-static const char *nr2str(const char **n, size_t len, uint32_t nr)
+/* Individual human-friendly trace events for states */
+static void ehci_trace_set_state(int async, uint32_t state)
 {
-if (nr  len  n[nr] != NULL) {
-return n[nr];
-} else {
-return unknown;
+switch (state) {
+case EST_INACTIVE:
+trace_usb_ehci_state_inactive(async);
+break;
+case EST_ACTIVE:
+trace_usb_ehci_state_active(async);
+break;
+case EST_EXECUTING:
+trace_usb_ehci_state_executing(async);
+break;
+case EST_SLEEPING:
+trace_usb_ehci_state_sleeping(async);
+break;
+case EST_WAITLISTHEAD:
+trace_usb_ehci_state_waitlisthead(async);
+break;
+case EST_FETCHENTRY:
+trace_usb_ehci_state_fetchentry(async);
+break;
+case EST_FETCHQH:
+trace_usb_ehci_state_fetchqh(async);
+break;
+case EST_FETCHITD:
+trace_usb_ehci_state_fetchitd(async);
+break;
+case EST_ADVANCEQUEUE:
+trace_usb_ehci_state_advancequeue(async);
+break;
+case EST_FETCHQTD:
+trace_usb_ehci_state_fetchqtd(async);
+break;
+case EST_EXECUTE:
+trace_usb_ehci_state_execute(async);
+break;
+case EST_WRITEBACK:
+trace_usb_ehci_state_writeback(async);
+break;
+case EST_HORIZONTALQH:
+trace_usb_ehci_state_horizontalqh(async);
+break;
+default:
+trace_usb_ehci_state_unknown(async, state);
+break;
 }
 }
 
-static const char *state2str(uint32_t state)
-{
-return nr2str(ehci_state_names, ARRAY_SIZE(ehci_state_names), state);
-}
-
-static const char *addr2str(target_phys_addr_t addr)
-{
-return nr2str(ehci_mmio_names, ARRAY_SIZE(ehci_mmio_names), addr);
-}
-
 static void ehci_trace_usbsts(uint32_t mask, int state)
 {
 /* interrupts */
 if (mask  USBSTS_INT) {
-trace_usb_ehci_usbsts(INT, state);
+trace_usb_ehci_usbsts_int(state);
 }
 if (mask  USBSTS_ERRINT) {
-trace_usb_ehci_usbsts(ERRINT, state);
+trace_usb_ehci_usbsts_errint(state);
 }
 if (mask  USBSTS_PCD) {
-trace_usb_ehci_usbsts(PCD, state);
+trace_usb_ehci_usbsts_pcd(state);
 }
 if (mask  USBSTS_FLR) {
-trace_usb_ehci_usbsts(FLR, state);
+trace_usb_ehci_usbsts_flr(state);
 }
 if (mask  USBSTS_HSE) {
-trace_usb_ehci_usbsts(HSE, state);
+trace_usb_ehci_usbsts_hse(state);
 }
 if (mask  USBSTS_IAA) {
-trace_usb_ehci_usbsts(IAA, state);
+trace_usb_ehci_usbsts_iaa(state);
 }
 
 /* status */
 if (mask  USBSTS_HALT) {
-trace_usb_ehci_usbsts(HALT, state);
+