qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] Upstreaming Course Debugging Changes
@ 2024-03-29 15:04 Don Porter
  2024-03-29 15:04 ` [PATCH 1/1] e1000: Get debug flags from an environment variable Don Porter
  0 siblings, 1 reply; 8+ messages in thread
From: Don Porter @ 2024-03-29 15:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Don Porter

Hi all,

I am a CS professor (and first time contributor) and have been using
qemu in my courses for over a decade, especially a course that asks
students to write major pieces of an OS kernel from starter code.

I have some patches, originally from Austin Clements at MIT, that I
have found useful over the years and that may be useful to others.  It
would also be nice not to have to build a custom qemu each semester.  I
have cleared upstreaming these with Austin, the original author.

In order to learn the process and community, I thought I would start
with one patch.  As the description says, it enables e1000 debugging
without recompilation.  One project in the course is to write an e1000
driver, and these logs are quite helpful to students.

Thank you in advance for your time,
Don Porter

Austin Clements (1):
  e1000: Get debug flags from an environment variable

 hw/net/e1000.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 6 deletions(-)

--
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/1] e1000: Get debug flags from an environment variable
  2024-03-29 15:04 [PATCH 0/1] Upstreaming Course Debugging Changes Don Porter
@ 2024-03-29 15:04 ` Don Porter
  2024-03-29 17:46   ` Richard Henderson
  0 siblings, 1 reply; 8+ messages in thread
From: Don Porter @ 2024-03-29 15:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Austin Clements, Geoffrey Thomas, Don Porter

From: Austin Clements <aclements@csail.mit.edu>

The E1000 debug messages are very useful for developing drivers, so
this introduces an E1000_DEBUG environment variable that lets the
debug flags be set without recompiling QEMU.

Signed-off-by: Austin Clements <aclements@csail.mit.edu>
[geofft@ldpreload.com: Rebased on top of 2.9.0]
Signed-off-by: Geoffrey Thomas <geofft@ldpreload.com>
Signed-off-by: Don Porter <porter@cs.unc.edu>
---
 hw/net/e1000.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 6 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 43f3a4a701..8d46225944 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -30,11 +30,14 @@
 #include "hw/pci/pci_device.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "monitor/monitor.h"
 #include "net/eth.h"
 #include "net/net.h"
 #include "net/checksum.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/dma.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/error-report.h"
 #include "qemu/iov.h"
 #include "qemu/module.h"
 #include "qemu/range.h"
@@ -44,15 +47,19 @@
 #include "trace.h"
 #include "qom/object.h"
 
-/* #define E1000_DEBUG */
-
-#ifdef E1000_DEBUG
 enum {
     DEBUG_GENERAL,      DEBUG_IO,       DEBUG_MMIO,     DEBUG_INTERRUPT,
     DEBUG_RX,           DEBUG_TX,       DEBUG_MDIC,     DEBUG_EEPROM,
     DEBUG_UNKNOWN,      DEBUG_TXSUM,    DEBUG_TXERR,    DEBUG_RXERR,
     DEBUG_RXFILTER,     DEBUG_PHY,      DEBUG_NOTYET,
 };
+
+static const char *debugnames[] = {
+    "GENERAL",      "IO",       "MMIO",     "INTERRUPT",
+    "RX",           "TX",       "MDIC",     "EEPROM",
+    "UNKNOWN",      "TXSUM",    "TXERR",    "RXERR",
+    "RXFILTER",     "PHY",      "NOTYET",   NULL
+};
 #define DBGBIT(x)    (1<<DEBUG_##x)
 static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
 
@@ -60,9 +67,6 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
     if (debugflags & DBGBIT(what)) \
         fprintf(stderr, "e1000: " fmt, ## __VA_ARGS__); \
     } while (0)
-#else
-#define DBGOUT(what, fmt, ...) do {} while (0)
-#endif
 
 #define IOPORT_SIZE       0x40
 #define PNPMMIO_SIZE      0x20000
@@ -1779,3 +1783,52 @@ static void e1000_register_types(void)
 }
 
 type_init(e1000_register_types)
+
+static void e1000_init_debug(void)
+{
+    const char *e1000_debug;
+    const char *p, *p1;
+    const char **debugname;
+    int i;
+
+    e1000_debug = getenv("E1000_DEBUG");
+    if (!e1000_debug || !*e1000_debug) {
+        return;
+    }
+
+    if (strcmp(e1000_debug, "?") == 0) {
+        error_printf("E1000_DEBUG flags:\n");
+        for (debugname = debugnames; *debugname; debugname++) {
+            error_printf("%s\n", *debugname);
+        }
+        exit(0);
+    }
+
+    p = e1000_debug;
+    debugflags = 0;
+    for (p = e1000_debug; ; p = p1 + 1) {
+        p1 = strchr(p, ',');
+        if (!p1) {
+            p1 = p + strlen(p);
+        }
+        for (i = 0, debugname = debugnames; *debugname; i++, debugname++) {
+            if (strlen(*debugname) == p1 - p &&
+                strncasecmp(p, *debugname, p1 - p) == 0) {
+                debugflags |= 1 << i;
+                break;
+            }
+        }
+        if (!*debugname) {
+            error_report(QERR_INVALID_PARAMETER_VALUE, "E1000_DEBUG",
+                         "a comma-separated list of E1000 debug flags");
+            error_printf_unless_qmp(
+                "Try with argument '?' for a list.\n");
+            exit(1);
+        }
+        if (*p1 != ',') {
+            break;
+        }
+    }
+}
+
+type_init(e1000_init_debug)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] e1000: Get debug flags from an environment variable
  2024-03-29 15:04 ` [PATCH 1/1] e1000: Get debug flags from an environment variable Don Porter
@ 2024-03-29 17:46   ` Richard Henderson
  2024-04-03 13:45     ` [PATCH v2] e1000: Convert debug macros into tracepoints Don Porter
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Henderson @ 2024-03-29 17:46 UTC (permalink / raw)
  To: Don Porter, qemu-devel; +Cc: Austin Clements, Geoffrey Thomas

On 3/29/24 05:04, Don Porter wrote:
> From: Austin Clements <aclements@csail.mit.edu>
> 
> The E1000 debug messages are very useful for developing drivers, so
> this introduces an E1000_DEBUG environment variable that lets the
> debug flags be set without recompiling QEMU.
> 
> Signed-off-by: Austin Clements <aclements@csail.mit.edu>
> [geofft@ldpreload.com: Rebased on top of 2.9.0]
> Signed-off-by: Geoffrey Thomas <geofft@ldpreload.com>
> Signed-off-by: Don Porter <porter@cs.unc.edu>
...
> -/* #define E1000_DEBUG */
> -
> -#ifdef E1000_DEBUG
>   enum {
>       DEBUG_GENERAL,      DEBUG_IO,       DEBUG_MMIO,     DEBUG_INTERRUPT,
>       DEBUG_RX,           DEBUG_TX,       DEBUG_MDIC,     DEBUG_EEPROM,
>       DEBUG_UNKNOWN,      DEBUG_TXSUM,    DEBUG_TXERR,    DEBUG_RXERR,
>       DEBUG_RXFILTER,     DEBUG_PHY,      DEBUG_NOTYET,
>   };
> +
> +static const char *debugnames[] = {
> +    "GENERAL",      "IO",       "MMIO",     "INTERRUPT",
> +    "RX",           "TX",       "MDIC",     "EEPROM",
> +    "UNKNOWN",      "TXSUM",    "TXERR",    "RXERR",
> +    "RXFILTER",     "PHY",      "NOTYET",   NULL
> +};
>   #define DBGBIT(x)    (1<<DEBUG_##x)
>   static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);

These should be converted to tracepoints.
See docs/devel/tracing.rst.


r~



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] e1000: Convert debug macros into tracepoints.
  2024-03-29 17:46   ` Richard Henderson
@ 2024-04-03 13:45     ` Don Porter
  2024-04-03 17:51       ` Richard Henderson
  2024-04-03 18:44       ` Austin Clements
  0 siblings, 2 replies; 8+ messages in thread
From: Don Porter @ 2024-04-03 13:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: jasonwang, richard.henderson, Austin Clements, Geoffrey Thomas,
	Don Porter

From: Austin Clements <aclements@csail.mit.edu>

The E1000 debug messages are very useful for developing drivers.
Make these available to users without recompiling QEMU.

Signed-off-by: Austin Clements <aclements@csail.mit.edu>
[geofft@ldpreload.com: Rebased on top of 2.9.0]
Signed-off-by: Geoffrey Thomas <geofft@ldpreload.com>
Signed-off-by: Don Porter <porter@cs.unc.edu>
---
 hw/net/e1000.c      | 90 +++++++++++++++------------------------------
 hw/net/trace-events | 25 ++++++++++++-
 2 files changed, 54 insertions(+), 61 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 43f3a4a701..24475636a3 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -44,26 +44,6 @@
 #include "trace.h"
 #include "qom/object.h"
 
-/* #define E1000_DEBUG */
-
-#ifdef E1000_DEBUG
-enum {
-    DEBUG_GENERAL,      DEBUG_IO,       DEBUG_MMIO,     DEBUG_INTERRUPT,
-    DEBUG_RX,           DEBUG_TX,       DEBUG_MDIC,     DEBUG_EEPROM,
-    DEBUG_UNKNOWN,      DEBUG_TXSUM,    DEBUG_TXERR,    DEBUG_RXERR,
-    DEBUG_RXFILTER,     DEBUG_PHY,      DEBUG_NOTYET,
-};
-#define DBGBIT(x)    (1<<DEBUG_##x)
-static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
-
-#define DBGOUT(what, fmt, ...) do { \
-    if (debugflags & DBGBIT(what)) \
-        fprintf(stderr, "e1000: " fmt, ## __VA_ARGS__); \
-    } while (0)
-#else
-#define DBGOUT(what, fmt, ...) do {} while (0)
-#endif
-
 #define IOPORT_SIZE       0x40
 #define PNPMMIO_SIZE      0x20000
 
@@ -351,8 +331,7 @@ e1000_mit_timer(void *opaque)
 static void
 set_ics(E1000State *s, int index, uint32_t val)
 {
-    DBGOUT(INTERRUPT, "set_ics %x, ICR %x, IMR %x\n", val, s->mac_reg[ICR],
-        s->mac_reg[IMS]);
+    trace_e1000_set_ics(val, s->mac_reg[ICR], s->mac_reg[IMS]);
     set_interrupt_cause(s, 0, val | s->mac_reg[ICR]);
 }
 
@@ -425,8 +404,7 @@ set_rx_control(E1000State *s, int index, uint32_t val)
     s->mac_reg[RCTL] = val;
     s->rxbuf_size = e1000x_rxbufsize(val);
     s->rxbuf_min_shift = ((val / E1000_RCTL_RDMTS_QUAT) & 3) + 1;
-    DBGOUT(RX, "RCTL: %d, mac_reg[RCTL] = 0x%x\n", s->mac_reg[RDT],
-           s->mac_reg[RCTL]);
+    trace_e1000_set_rx_control(s->mac_reg[RDT], s->mac_reg[RCTL]);
     timer_mod(s->flush_queue_timer,
               qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
 }
@@ -440,16 +418,16 @@ set_mdic(E1000State *s, int index, uint32_t val)
     if ((val & E1000_MDIC_PHY_MASK) >> E1000_MDIC_PHY_SHIFT != 1) // phy #
         val = s->mac_reg[MDIC] | E1000_MDIC_ERROR;
     else if (val & E1000_MDIC_OP_READ) {
-        DBGOUT(MDIC, "MDIC read reg 0x%x\n", addr);
+        trace_e1000_mdic_read_register(addr);
         if (!(phy_regcap[addr] & PHY_R)) {
-            DBGOUT(MDIC, "MDIC read reg %x unhandled\n", addr);
+            trace_e1000_mdic_read_register_unhandled(addr);
             val |= E1000_MDIC_ERROR;
         } else
             val = (val ^ data) | s->phy_reg[addr];
     } else if (val & E1000_MDIC_OP_WRITE) {
-        DBGOUT(MDIC, "MDIC write reg 0x%x, value 0x%x\n", addr, data);
+        trace_e1000_mdic_write_register(addr, data);
         if (!(phy_regcap[addr] & PHY_W)) {
-            DBGOUT(MDIC, "MDIC write reg %x unhandled\n", addr);
+            trace_e1000_mdic_write_register_unhandled(addr);
             val |= E1000_MDIC_ERROR;
         } else {
             if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
@@ -471,8 +449,8 @@ get_eecd(E1000State *s, int index)
 {
     uint32_t ret = E1000_EECD_PRES|E1000_EECD_GNT | s->eecd_state.old_eecd;
 
-    DBGOUT(EEPROM, "reading eeprom bit %d (reading %d)\n",
-           s->eecd_state.bitnum_out, s->eecd_state.reading);
+    trace_e1000_get_eecd(s->eecd_state.bitnum_out, s->eecd_state.reading);
+
     if (!s->eecd_state.reading ||
         ((s->eeprom_data[(s->eecd_state.bitnum_out >> 4) & 0x3f] >>
           ((s->eecd_state.bitnum_out & 0xf) ^ 0xf))) & 1)
@@ -511,9 +489,8 @@ set_eecd(E1000State *s, int index, uint32_t val)
         s->eecd_state.reading = (((s->eecd_state.val_in >> 6) & 7) ==
             EEPROM_READ_OPCODE_MICROWIRE);
     }
-    DBGOUT(EEPROM, "eeprom bitnum in %d out %d, reading %d\n",
-           s->eecd_state.bitnum_in, s->eecd_state.bitnum_out,
-           s->eecd_state.reading);
+    trace_e1000_set_eecd(s->eecd_state.bitnum_in, s->eecd_state.bitnum_out,
+                         s->eecd_state.reading);
 }
 
 static uint32_t
@@ -580,8 +557,7 @@ xmit_seg(E1000State *s)
 
     if (tp->cptse) {
         css = props->ipcss;
-        DBGOUT(TXSUM, "frames %d size %d ipcss %d\n",
-               frames, tp->size, css);
+        trace_e1000_xmit_seg1(frames, tp->size, css);
         if (props->ip) {    /* IPv4 */
             stw_be_p(tp->data+css+2, tp->size - css);
             stw_be_p(tp->data+css+4,
@@ -591,7 +567,7 @@ xmit_seg(E1000State *s)
         }
         css = props->tucss;
         len = tp->size - css;
-        DBGOUT(TXSUM, "tcp %d tucss %d len %d\n", props->tcp, css, len);
+        trace_e1000_xmit_seg2(props->tcp, css, len);
         if (props->tcp) {
             sofar = frames * props->mss;
             stl_be_p(tp->data+css+4, ldl_be_p(tp->data+css+4)+sofar); /* seq */
@@ -759,7 +735,7 @@ start_xmit(E1000State *s)
     uint32_t tdh_start = s->mac_reg[TDH], cause = E1000_ICS_TXQE;
 
     if (!(s->mac_reg[TCTL] & E1000_TCTL_EN)) {
-        DBGOUT(TX, "tx disabled\n");
+        trace_e1000_start_xmit_fail1();
         return;
     }
 
@@ -773,9 +749,9 @@ start_xmit(E1000State *s)
                sizeof(struct e1000_tx_desc) * s->mac_reg[TDH];
         pci_dma_read(d, base, &desc, sizeof(desc));
 
-        DBGOUT(TX, "index %d: %p : %x %x\n", s->mac_reg[TDH],
-               (void *)(intptr_t)desc.buffer_addr, desc.lower.data,
-               desc.upper.data);
+        trace_e1000_transmit(s->mac_reg[TDH],
+                             (void *)(intptr_t)desc.buffer_addr,
+                             desc.lower.data, desc.upper.data);
 
         process_tx_desc(s, &desc);
         cause |= txdesc_writeback(s, base, &desc);
@@ -789,8 +765,8 @@ start_xmit(E1000State *s)
          */
         if (s->mac_reg[TDH] == tdh_start ||
             tdh_start >= s->mac_reg[TDLEN] / sizeof(desc)) {
-            DBGOUT(TXERR, "TDH wraparound @%x, TDT %x, TDLEN %x\n",
-                   tdh_start, s->mac_reg[TDT], s->mac_reg[TDLEN]);
+            trace_e1000_start_xmit_fail2(tdh_start, s->mac_reg[TDT],
+                                         s->mac_reg[TDLEN]);
             break;
         }
     }
@@ -978,7 +954,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
                 desc.status &= ~E1000_RXD_STAT_EOP;
             }
         } else { // as per intel docs; skip descriptors with null buf addr
-            DBGOUT(RX, "Null RX descriptor!!\n");
+            trace_e1000_null_rx();
         }
         pci_dma_write(d, base, &desc, sizeof(desc));
         desc.status |= (vlan_status | E1000_RXD_STAT_DD);
@@ -990,8 +966,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
         /* see comment in start_xmit; same here */
         if (s->mac_reg[RDH] == rdh_start ||
             rdh_start >= s->mac_reg[RDLEN] / sizeof(desc)) {
-            DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n",
-                   rdh_start, s->mac_reg[RDT], s->mac_reg[RDLEN]);
+            trace_e1000_rdh_wraparound(rdh_start, s->mac_reg[RDT],
+                                       s->mac_reg[RDLEN]);
             e1000_receiver_overrun(s, total_size);
             return -1;
         }
@@ -1033,7 +1009,7 @@ mac_icr_read(E1000State *s, int index)
 {
     uint32_t ret = s->mac_reg[ICR];
 
-    DBGOUT(INTERRUPT, "ICR read: %x\n", ret);
+    trace_e1000_mac_icr_read(ret);
     set_interrupt_cause(s, 0, 0);
     return ret;
 }
@@ -1109,7 +1085,7 @@ set_tctl(E1000State *s, int index, uint32_t val)
 static void
 set_icr(E1000State *s, int index, uint32_t val)
 {
-    DBGOUT(INTERRUPT, "set_icr %x\n", val);
+    trace_e1000_set_icr(val);
     set_interrupt_cause(s, 0, s->mac_reg[ICR] & ~val);
 }
 
@@ -1271,20 +1247,16 @@ e1000_mmio_write(void *opaque, hwaddr addr, uint64_t val,
         if (!(mac_reg_access[index] & MAC_ACCESS_FLAG_NEEDED)
             || (s->compat_flags & (mac_reg_access[index] >> 2))) {
             if (mac_reg_access[index] & MAC_ACCESS_PARTIAL) {
-                DBGOUT(GENERAL, "Writing to register at offset: 0x%08x. "
-                       "It is not fully implemented.\n", index<<2);
+                trace_e1000_mmio_write_fail1(index << 2);
             }
             macreg_writeops[index](s, index, val);
         } else {    /* "flag needed" bit is set, but the flag is not active */
-            DBGOUT(MMIO, "MMIO write attempt to disabled reg. addr=0x%08x\n",
-                   index<<2);
+            trace_e1000_mmio_write_fail2(index << 2);
         }
     } else if (index < NREADOPS && macreg_readops[index]) {
-        DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04"PRIx64"\n",
-               index<<2, val);
+        trace_e1000_mmio_write_fail3(index << 2, val);
     } else {
-        DBGOUT(UNKNOWN, "MMIO unknown write addr=0x%08x,val=0x%08"PRIx64"\n",
-               index<<2, val);
+        trace_e1000_mmio_write_fail4(index << 2, val);
     }
 }
 
@@ -1298,16 +1270,14 @@ e1000_mmio_read(void *opaque, hwaddr addr, unsigned size)
         if (!(mac_reg_access[index] & MAC_ACCESS_FLAG_NEEDED)
             || (s->compat_flags & (mac_reg_access[index] >> 2))) {
             if (mac_reg_access[index] & MAC_ACCESS_PARTIAL) {
-                DBGOUT(GENERAL, "Reading register at offset: 0x%08x. "
-                       "It is not fully implemented.\n", index<<2);
+                trace_e1000_mmio_read_fail1(index << 2);
             }
             return macreg_readops[index](s, index);
         } else {    /* "flag needed" bit is set, but the flag is not active */
-            DBGOUT(MMIO, "MMIO read attempt of disabled reg. addr=0x%08x\n",
-                   index<<2);
+            trace_e1000_mmio_read_fail2(index << 2);
         }
     } else {
-        DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
+        trace_e1000_mmio_read_fail3(index << 2);
     }
     return 0;
 }
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 78efa2ec2c..f426f79a0c 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -101,7 +101,31 @@ net_rx_pkt_rss_hash(size_t rss_length, uint32_t rss_hash) "RSS hash for %zu byte
 net_rx_pkt_rss_add_chunk(void* ptr, size_t size, size_t input_offset) "Add RSS chunk %p, %zu bytes, RSS input offset %zu bytes"
 
 # e1000.c
+e1000_set_ics(uint32_t val, uint32_t ICR, uint32_t IMR) "set_ics 0x%x, ICR 0x%x, IMR 0x%x"
+e1000_set_rx_control(uint32_t RDT, uint32_t RCTL) "RCTL: %d, mac_reg[RCTL] = 0x%x"
+e1000_mdic_read_register(uint32_t addr) "MDIC read reg 0x%x"
+e1000_mdic_read_register_unhandled(uint32_t addr) "MDIC read reg 0x%x unhandled"
+e1000_mdic_write_register(uint32_t addr, uint32_t val) "MDIC write reg 0x%x, value 0x%x"
+e1000_mdic_write_register_unhandled(uint32_t addr) "MDIC write reg 0x%x unhandled"
+e1000_transmit(uint32_t tdh, void *addr, uint32_t data_low, uint32_t data_high) "index %d: %p : 0x%x 0x%x"
+e1000_get_eecd(uint16_t bitnum_out, uint16_t reading) "reading eeprom bit %d (reading %d)"
+e1000_set_eecd(uint16_t bitnum_in, uint16_t bitnum_out, uint16_t reading) "eeprom bitnum in %d out %d, reading %d"
+e1000_xmit_seg1(unsigned int frames, uint16_t size, unsigned int css) "frames %d size %d ipcss %d"
+e1000_xmit_seg2(int8_t tcp, uint16_t css, unsigned int len) "tcp %d tucss %d len %d"
+e1000_start_xmit_fail1(void) "tx disabled"
+e1000_start_xmit_fail2(uint32_t tdh_start, uint32_t TDT, uint32_t TDLEN) "TDH wraparound @0x%x, TDT 0x%x, TDLEN 0x%x"
 e1000_receiver_overrun(size_t s, uint32_t rdh, uint32_t rdt) "Receiver overrun: dropped packet of %zu bytes, RDH=%u, RDT=%u"
+e1000_null_rx(void) "Null RX descriptor!!"
+e1000_rdh_wraparound(uint32_t rdh_start, uint32_t RDT, uint32_t RDLEN) "RDH wraparound @0x%x, RDT 0x%x, RDLEN 0x%x"
+e1000_mac_icr_read(uint32_t ret) "ICR read: 0x%x"
+e1000_set_icr(uint32_t val) "set_icr 0x%x"
+e1000_mmio_write_fail1(unsigned int index) "Writing to register at offset: 0x%08x. It is not fully implemented."
+e1000_mmio_write_fail2(unsigned int index) "MMIO write attempt to disabled reg. addr=0x%08x"
+e1000_mmio_write_fail3(unsigned int index, uint64_t val) "e1000_mmio_writel RO 0x%x: 0x%04"PRIx64""
+e1000_mmio_write_fail4(unsigned int index, uint64_t val) "MMIO unknown write addr=0x%08x,val=0x%08"PRIx64""
+e1000_mmio_read_fail1(unsigned int index) "Reading register at offset: 0x%08x. It is not fully implemented."
+e1000_mmio_read_fail2(unsigned int index) "MMIO read attempt of disabled reg. addr=0x%08x"
+e1000_mmio_read_fail3(unsigned int index) "MMIO unknown read addr=0x%08x"
 
 # e1000x_common.c
 e1000x_rx_can_recv_disabled(bool link_up, bool rx_enabled, bool pci_master) "link_up: %d, rx_enabled %d, pci_master %d"
@@ -146,7 +170,6 @@ e1000e_wrn_no_snap_support(void) "WARNING: Guest requested TX SNAP header update
 e1000e_wrn_iscsi_filtering_not_supported(void) "WARNING: Guest requested iSCSI filtering  which is not supported"
 e1000e_wrn_nfsw_filtering_not_supported(void) "WARNING: Guest requested NFS write filtering  which is not supported"
 e1000e_wrn_nfsr_filtering_not_supported(void) "WARNING: Guest requested NFS read filtering  which is not supported"
-
 e1000e_tx_disabled(void) "TX Disabled"
 e1000e_tx_descr(void *addr, uint32_t lower, uint32_t upper) "%p : %x %x"
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] e1000: Convert debug macros into tracepoints.
  2024-04-03 13:45     ` [PATCH v2] e1000: Convert debug macros into tracepoints Don Porter
@ 2024-04-03 17:51       ` Richard Henderson
  2024-04-03 18:44       ` Austin Clements
  1 sibling, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2024-04-03 17:51 UTC (permalink / raw)
  To: Don Porter, qemu-devel; +Cc: jasonwang, Austin Clements, Geoffrey Thomas

On 4/3/24 03:45, Don Porter wrote:
> From: Austin Clements<aclements@csail.mit.edu>
> 
> The E1000 debug messages are very useful for developing drivers.
> Make these available to users without recompiling QEMU.
> 
> Signed-off-by: Austin Clements<aclements@csail.mit.edu>
> [geofft@ldpreload.com: Rebased on top of 2.9.0]
> Signed-off-by: Geoffrey Thomas<geofft@ldpreload.com>
> Signed-off-by: Don Porter<porter@cs.unc.edu>
> ---
>   hw/net/e1000.c      | 90 +++++++++++++++------------------------------
>   hw/net/trace-events | 25 ++++++++++++-
>   2 files changed, 54 insertions(+), 61 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] e1000: Convert debug macros into tracepoints.
  2024-04-03 13:45     ` [PATCH v2] e1000: Convert debug macros into tracepoints Don Porter
  2024-04-03 17:51       ` Richard Henderson
@ 2024-04-03 18:44       ` Austin Clements
  2024-04-08 21:11         ` Don Porter
  2024-04-08 21:15         ` [PATCH v3] " Don Porter
  1 sibling, 2 replies; 8+ messages in thread
From: Austin Clements @ 2024-04-03 18:44 UTC (permalink / raw)
  To: Don Porter; +Cc: qemu-devel, jasonwang, richard.henderson, Geoffrey Thomas

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

At this point there's not much of my original code left. :D Don, you're
welcome to take the credit in the commit.

On Wed, Apr 3, 2024, 9:46 AM Don Porter <porter@cs.unc.edu> wrote:

> From: Austin Clements <aclements@csail.mit.edu>
>
> The E1000 debug messages are very useful for developing drivers.
> Make these available to users without recompiling QEMU.
>
> Signed-off-by: Austin Clements <aclements@csail.mit.edu>
> [geofft@ldpreload.com: Rebased on top of 2.9.0]
> Signed-off-by: Geoffrey Thomas <geofft@ldpreload.com>
> Signed-off-by: Don Porter <porter@cs.unc.edu>
> ---
>  hw/net/e1000.c      | 90 +++++++++++++++------------------------------
>  hw/net/trace-events | 25 ++++++++++++-
>  2 files changed, 54 insertions(+), 61 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 43f3a4a701..24475636a3 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -44,26 +44,6 @@
>  #include "trace.h"
>  #include "qom/object.h"
>
> -/* #define E1000_DEBUG */
> -
> -#ifdef E1000_DEBUG
> -enum {
> -    DEBUG_GENERAL,      DEBUG_IO,       DEBUG_MMIO,     DEBUG_INTERRUPT,
> -    DEBUG_RX,           DEBUG_TX,       DEBUG_MDIC,     DEBUG_EEPROM,
> -    DEBUG_UNKNOWN,      DEBUG_TXSUM,    DEBUG_TXERR,    DEBUG_RXERR,
> -    DEBUG_RXFILTER,     DEBUG_PHY,      DEBUG_NOTYET,
> -};
> -#define DBGBIT(x)    (1<<DEBUG_##x)
> -static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
> -
> -#define DBGOUT(what, fmt, ...) do { \
> -    if (debugflags & DBGBIT(what)) \
> -        fprintf(stderr, "e1000: " fmt, ## __VA_ARGS__); \
> -    } while (0)
> -#else
> -#define DBGOUT(what, fmt, ...) do {} while (0)
> -#endif
> -
>  #define IOPORT_SIZE       0x40
>  #define PNPMMIO_SIZE      0x20000
>
> @@ -351,8 +331,7 @@ e1000_mit_timer(void *opaque)
>  static void
>  set_ics(E1000State *s, int index, uint32_t val)
>  {
> -    DBGOUT(INTERRUPT, "set_ics %x, ICR %x, IMR %x\n", val,
> s->mac_reg[ICR],
> -        s->mac_reg[IMS]);
> +    trace_e1000_set_ics(val, s->mac_reg[ICR], s->mac_reg[IMS]);
>      set_interrupt_cause(s, 0, val | s->mac_reg[ICR]);
>  }
>
> @@ -425,8 +404,7 @@ set_rx_control(E1000State *s, int index, uint32_t val)
>      s->mac_reg[RCTL] = val;
>      s->rxbuf_size = e1000x_rxbufsize(val);
>      s->rxbuf_min_shift = ((val / E1000_RCTL_RDMTS_QUAT) & 3) + 1;
> -    DBGOUT(RX, "RCTL: %d, mac_reg[RCTL] = 0x%x\n", s->mac_reg[RDT],
> -           s->mac_reg[RCTL]);
> +    trace_e1000_set_rx_control(s->mac_reg[RDT], s->mac_reg[RCTL]);
>      timer_mod(s->flush_queue_timer,
>                qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
>  }
> @@ -440,16 +418,16 @@ set_mdic(E1000State *s, int index, uint32_t val)
>      if ((val & E1000_MDIC_PHY_MASK) >> E1000_MDIC_PHY_SHIFT != 1) // phy #
>          val = s->mac_reg[MDIC] | E1000_MDIC_ERROR;
>      else if (val & E1000_MDIC_OP_READ) {
> -        DBGOUT(MDIC, "MDIC read reg 0x%x\n", addr);
> +        trace_e1000_mdic_read_register(addr);
>          if (!(phy_regcap[addr] & PHY_R)) {
> -            DBGOUT(MDIC, "MDIC read reg %x unhandled\n", addr);
> +            trace_e1000_mdic_read_register_unhandled(addr);
>              val |= E1000_MDIC_ERROR;
>          } else
>              val = (val ^ data) | s->phy_reg[addr];
>      } else if (val & E1000_MDIC_OP_WRITE) {
> -        DBGOUT(MDIC, "MDIC write reg 0x%x, value 0x%x\n", addr, data);
> +        trace_e1000_mdic_write_register(addr, data);
>          if (!(phy_regcap[addr] & PHY_W)) {
> -            DBGOUT(MDIC, "MDIC write reg %x unhandled\n", addr);
> +            trace_e1000_mdic_write_register_unhandled(addr);
>              val |= E1000_MDIC_ERROR;
>          } else {
>              if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
> @@ -471,8 +449,8 @@ get_eecd(E1000State *s, int index)
>  {
>      uint32_t ret = E1000_EECD_PRES|E1000_EECD_GNT |
> s->eecd_state.old_eecd;
>
> -    DBGOUT(EEPROM, "reading eeprom bit %d (reading %d)\n",
> -           s->eecd_state.bitnum_out, s->eecd_state.reading);
> +    trace_e1000_get_eecd(s->eecd_state.bitnum_out, s->eecd_state.reading);
> +
>      if (!s->eecd_state.reading ||
>          ((s->eeprom_data[(s->eecd_state.bitnum_out >> 4) & 0x3f] >>
>            ((s->eecd_state.bitnum_out & 0xf) ^ 0xf))) & 1)
> @@ -511,9 +489,8 @@ set_eecd(E1000State *s, int index, uint32_t val)
>          s->eecd_state.reading = (((s->eecd_state.val_in >> 6) & 7) ==
>              EEPROM_READ_OPCODE_MICROWIRE);
>      }
> -    DBGOUT(EEPROM, "eeprom bitnum in %d out %d, reading %d\n",
> -           s->eecd_state.bitnum_in, s->eecd_state.bitnum_out,
> -           s->eecd_state.reading);
> +    trace_e1000_set_eecd(s->eecd_state.bitnum_in,
> s->eecd_state.bitnum_out,
> +                         s->eecd_state.reading);
>  }
>
>  static uint32_t
> @@ -580,8 +557,7 @@ xmit_seg(E1000State *s)
>
>      if (tp->cptse) {
>          css = props->ipcss;
> -        DBGOUT(TXSUM, "frames %d size %d ipcss %d\n",
> -               frames, tp->size, css);
> +        trace_e1000_xmit_seg1(frames, tp->size, css);
>          if (props->ip) {    /* IPv4 */
>              stw_be_p(tp->data+css+2, tp->size - css);
>              stw_be_p(tp->data+css+4,
> @@ -591,7 +567,7 @@ xmit_seg(E1000State *s)
>          }
>          css = props->tucss;
>          len = tp->size - css;
> -        DBGOUT(TXSUM, "tcp %d tucss %d len %d\n", props->tcp, css, len);
> +        trace_e1000_xmit_seg2(props->tcp, css, len);
>          if (props->tcp) {
>              sofar = frames * props->mss;
>              stl_be_p(tp->data+css+4, ldl_be_p(tp->data+css+4)+sofar); /*
> seq */
> @@ -759,7 +735,7 @@ start_xmit(E1000State *s)
>      uint32_t tdh_start = s->mac_reg[TDH], cause = E1000_ICS_TXQE;
>
>      if (!(s->mac_reg[TCTL] & E1000_TCTL_EN)) {
> -        DBGOUT(TX, "tx disabled\n");
> +        trace_e1000_start_xmit_fail1();
>          return;
>      }
>
> @@ -773,9 +749,9 @@ start_xmit(E1000State *s)
>                 sizeof(struct e1000_tx_desc) * s->mac_reg[TDH];
>          pci_dma_read(d, base, &desc, sizeof(desc));
>
> -        DBGOUT(TX, "index %d: %p : %x %x\n", s->mac_reg[TDH],
> -               (void *)(intptr_t)desc.buffer_addr, desc.lower.data,
> -               desc.upper.data);
> +        trace_e1000_transmit(s->mac_reg[TDH],
> +                             (void *)(intptr_t)desc.buffer_addr,
> +                             desc.lower.data, desc.upper.data);
>
>          process_tx_desc(s, &desc);
>          cause |= txdesc_writeback(s, base, &desc);
> @@ -789,8 +765,8 @@ start_xmit(E1000State *s)
>           */
>          if (s->mac_reg[TDH] == tdh_start ||
>              tdh_start >= s->mac_reg[TDLEN] / sizeof(desc)) {
> -            DBGOUT(TXERR, "TDH wraparound @%x, TDT %x, TDLEN %x\n",
> -                   tdh_start, s->mac_reg[TDT], s->mac_reg[TDLEN]);
> +            trace_e1000_start_xmit_fail2(tdh_start, s->mac_reg[TDT],
> +                                         s->mac_reg[TDLEN]);
>              break;
>          }
>      }
> @@ -978,7 +954,7 @@ e1000_receive_iov(NetClientState *nc, const struct
> iovec *iov, int iovcnt)
>                  desc.status &= ~E1000_RXD_STAT_EOP;
>              }
>          } else { // as per intel docs; skip descriptors with null buf addr
> -            DBGOUT(RX, "Null RX descriptor!!\n");
> +            trace_e1000_null_rx();
>          }
>          pci_dma_write(d, base, &desc, sizeof(desc));
>          desc.status |= (vlan_status | E1000_RXD_STAT_DD);
> @@ -990,8 +966,8 @@ e1000_receive_iov(NetClientState *nc, const struct
> iovec *iov, int iovcnt)
>          /* see comment in start_xmit; same here */
>          if (s->mac_reg[RDH] == rdh_start ||
>              rdh_start >= s->mac_reg[RDLEN] / sizeof(desc)) {
> -            DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n",
> -                   rdh_start, s->mac_reg[RDT], s->mac_reg[RDLEN]);
> +            trace_e1000_rdh_wraparound(rdh_start, s->mac_reg[RDT],
> +                                       s->mac_reg[RDLEN]);
>              e1000_receiver_overrun(s, total_size);
>              return -1;
>          }
> @@ -1033,7 +1009,7 @@ mac_icr_read(E1000State *s, int index)
>  {
>      uint32_t ret = s->mac_reg[ICR];
>
> -    DBGOUT(INTERRUPT, "ICR read: %x\n", ret);
> +    trace_e1000_mac_icr_read(ret);
>      set_interrupt_cause(s, 0, 0);
>      return ret;
>  }
> @@ -1109,7 +1085,7 @@ set_tctl(E1000State *s, int index, uint32_t val)
>  static void
>  set_icr(E1000State *s, int index, uint32_t val)
>  {
> -    DBGOUT(INTERRUPT, "set_icr %x\n", val);
> +    trace_e1000_set_icr(val);
>      set_interrupt_cause(s, 0, s->mac_reg[ICR] & ~val);
>  }
>
> @@ -1271,20 +1247,16 @@ e1000_mmio_write(void *opaque, hwaddr addr,
> uint64_t val,
>          if (!(mac_reg_access[index] & MAC_ACCESS_FLAG_NEEDED)
>              || (s->compat_flags & (mac_reg_access[index] >> 2))) {
>              if (mac_reg_access[index] & MAC_ACCESS_PARTIAL) {
> -                DBGOUT(GENERAL, "Writing to register at offset: 0x%08x. "
> -                       "It is not fully implemented.\n", index<<2);
> +                trace_e1000_mmio_write_fail1(index << 2);
>              }
>              macreg_writeops[index](s, index, val);
>          } else {    /* "flag needed" bit is set, but the flag is not
> active */
> -            DBGOUT(MMIO, "MMIO write attempt to disabled reg.
> addr=0x%08x\n",
> -                   index<<2);
> +            trace_e1000_mmio_write_fail2(index << 2);
>          }
>      } else if (index < NREADOPS && macreg_readops[index]) {
> -        DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04"PRIx64"\n",
> -               index<<2, val);
> +        trace_e1000_mmio_write_fail3(index << 2, val);
>      } else {
> -        DBGOUT(UNKNOWN, "MMIO unknown write
> addr=0x%08x,val=0x%08"PRIx64"\n",
> -               index<<2, val);
> +        trace_e1000_mmio_write_fail4(index << 2, val);
>      }
>  }
>
> @@ -1298,16 +1270,14 @@ e1000_mmio_read(void *opaque, hwaddr addr,
> unsigned size)
>          if (!(mac_reg_access[index] & MAC_ACCESS_FLAG_NEEDED)
>              || (s->compat_flags & (mac_reg_access[index] >> 2))) {
>              if (mac_reg_access[index] & MAC_ACCESS_PARTIAL) {
> -                DBGOUT(GENERAL, "Reading register at offset: 0x%08x. "
> -                       "It is not fully implemented.\n", index<<2);
> +                trace_e1000_mmio_read_fail1(index << 2);
>              }
>              return macreg_readops[index](s, index);
>          } else {    /* "flag needed" bit is set, but the flag is not
> active */
> -            DBGOUT(MMIO, "MMIO read attempt of disabled reg.
> addr=0x%08x\n",
> -                   index<<2);
> +            trace_e1000_mmio_read_fail2(index << 2);
>          }
>      } else {
> -        DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
> +        trace_e1000_mmio_read_fail3(index << 2);
>      }
>      return 0;
>  }
> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index 78efa2ec2c..f426f79a0c 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -101,7 +101,31 @@ net_rx_pkt_rss_hash(size_t rss_length, uint32_t
> rss_hash) "RSS hash for %zu byte
>  net_rx_pkt_rss_add_chunk(void* ptr, size_t size, size_t input_offset)
> "Add RSS chunk %p, %zu bytes, RSS input offset %zu bytes"
>
>  # e1000.c
> +e1000_set_ics(uint32_t val, uint32_t ICR, uint32_t IMR) "set_ics 0x%x,
> ICR 0x%x, IMR 0x%x"
> +e1000_set_rx_control(uint32_t RDT, uint32_t RCTL) "RCTL: %d,
> mac_reg[RCTL] = 0x%x"
> +e1000_mdic_read_register(uint32_t addr) "MDIC read reg 0x%x"
> +e1000_mdic_read_register_unhandled(uint32_t addr) "MDIC read reg 0x%x
> unhandled"
> +e1000_mdic_write_register(uint32_t addr, uint32_t val) "MDIC write reg
> 0x%x, value 0x%x"
> +e1000_mdic_write_register_unhandled(uint32_t addr) "MDIC write reg 0x%x
> unhandled"
> +e1000_transmit(uint32_t tdh, void *addr, uint32_t data_low, uint32_t
> data_high) "index %d: %p : 0x%x 0x%x"
> +e1000_get_eecd(uint16_t bitnum_out, uint16_t reading) "reading eeprom bit
> %d (reading %d)"
> +e1000_set_eecd(uint16_t bitnum_in, uint16_t bitnum_out, uint16_t reading)
> "eeprom bitnum in %d out %d, reading %d"
> +e1000_xmit_seg1(unsigned int frames, uint16_t size, unsigned int css)
> "frames %d size %d ipcss %d"
> +e1000_xmit_seg2(int8_t tcp, uint16_t css, unsigned int len) "tcp %d tucss
> %d len %d"
> +e1000_start_xmit_fail1(void) "tx disabled"
> +e1000_start_xmit_fail2(uint32_t tdh_start, uint32_t TDT, uint32_t TDLEN)
> "TDH wraparound @0x%x, TDT 0x%x, TDLEN 0x%x"
>  e1000_receiver_overrun(size_t s, uint32_t rdh, uint32_t rdt) "Receiver
> overrun: dropped packet of %zu bytes, RDH=%u, RDT=%u"
> +e1000_null_rx(void) "Null RX descriptor!!"
> +e1000_rdh_wraparound(uint32_t rdh_start, uint32_t RDT, uint32_t RDLEN)
> "RDH wraparound @0x%x, RDT 0x%x, RDLEN 0x%x"
> +e1000_mac_icr_read(uint32_t ret) "ICR read: 0x%x"
> +e1000_set_icr(uint32_t val) "set_icr 0x%x"
> +e1000_mmio_write_fail1(unsigned int index) "Writing to register at
> offset: 0x%08x. It is not fully implemented."
> +e1000_mmio_write_fail2(unsigned int index) "MMIO write attempt to
> disabled reg. addr=0x%08x"
> +e1000_mmio_write_fail3(unsigned int index, uint64_t val)
> "e1000_mmio_writel RO 0x%x: 0x%04"PRIx64""
> +e1000_mmio_write_fail4(unsigned int index, uint64_t val) "MMIO unknown
> write addr=0x%08x,val=0x%08"PRIx64""
> +e1000_mmio_read_fail1(unsigned int index) "Reading register at offset:
> 0x%08x. It is not fully implemented."
> +e1000_mmio_read_fail2(unsigned int index) "MMIO read attempt of disabled
> reg. addr=0x%08x"
> +e1000_mmio_read_fail3(unsigned int index) "MMIO unknown read addr=0x%08x"
>
>  # e1000x_common.c
>  e1000x_rx_can_recv_disabled(bool link_up, bool rx_enabled, bool
> pci_master) "link_up: %d, rx_enabled %d, pci_master %d"
> @@ -146,7 +170,6 @@ e1000e_wrn_no_snap_support(void) "WARNING: Guest
> requested TX SNAP header update
>  e1000e_wrn_iscsi_filtering_not_supported(void) "WARNING: Guest requested
> iSCSI filtering  which is not supported"
>  e1000e_wrn_nfsw_filtering_not_supported(void) "WARNING: Guest requested
> NFS write filtering  which is not supported"
>  e1000e_wrn_nfsr_filtering_not_supported(void) "WARNING: Guest requested
> NFS read filtering  which is not supported"
> -
>  e1000e_tx_disabled(void) "TX Disabled"
>  e1000e_tx_descr(void *addr, uint32_t lower, uint32_t upper) "%p : %x %x"
>
> --
> 2.25.1
>
>

[-- Attachment #2: Type: text/html, Size: 17957 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] e1000: Convert debug macros into tracepoints.
  2024-04-03 18:44       ` Austin Clements
@ 2024-04-08 21:11         ` Don Porter
  2024-04-08 21:15         ` [PATCH v3] " Don Porter
  1 sibling, 0 replies; 8+ messages in thread
From: Don Porter @ 2024-04-08 21:11 UTC (permalink / raw)
  To: Austin Clements; +Cc: qemu-devel, jasonwang, richard.henderson, Geoffrey Thomas

On 4/3/24 2:44 PM, Austin Clements wrote:
> At this point there's not much of my original code left. :D Don, 
> you're welcome to take the credit in the commit.

Thanks Austin.  I'll send v3 with this change :)

BTW, my attempt to include the appropriate maintainer from 
scripts/get_maintainer.pl (jasonwang@redhat.com) bounced.  Any pointers 
on who else should be cc-ed are appreciated.

-dp



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3] e1000: Convert debug macros into tracepoints.
  2024-04-03 18:44       ` Austin Clements
  2024-04-08 21:11         ` Don Porter
@ 2024-04-08 21:15         ` Don Porter
  1 sibling, 0 replies; 8+ messages in thread
From: Don Porter @ 2024-04-08 21:15 UTC (permalink / raw)
  To: qemu-devel
  Cc: richard.henderson, Don Porter, Austin Clements, Geoffrey Thomas

The E1000 debug messages are very useful for developing drivers.
Make these available to users without recompiling QEMU.

Signed-off-by: Austin Clements <aclements@csail.mit.edu>
[geofft@ldpreload.com: Rebased on top of 2.9.0]
Signed-off-by: Geoffrey Thomas <geofft@ldpreload.com>
Signed-off-by: Don Porter <porter@cs.unc.edu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/net/e1000.c      | 90 +++++++++++++++------------------------------
 hw/net/trace-events | 25 ++++++++++++-
 2 files changed, 54 insertions(+), 61 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 43f3a4a701..24475636a3 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -44,26 +44,6 @@
 #include "trace.h"
 #include "qom/object.h"
 
-/* #define E1000_DEBUG */
-
-#ifdef E1000_DEBUG
-enum {
-    DEBUG_GENERAL,      DEBUG_IO,       DEBUG_MMIO,     DEBUG_INTERRUPT,
-    DEBUG_RX,           DEBUG_TX,       DEBUG_MDIC,     DEBUG_EEPROM,
-    DEBUG_UNKNOWN,      DEBUG_TXSUM,    DEBUG_TXERR,    DEBUG_RXERR,
-    DEBUG_RXFILTER,     DEBUG_PHY,      DEBUG_NOTYET,
-};
-#define DBGBIT(x)    (1<<DEBUG_##x)
-static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
-
-#define DBGOUT(what, fmt, ...) do { \
-    if (debugflags & DBGBIT(what)) \
-        fprintf(stderr, "e1000: " fmt, ## __VA_ARGS__); \
-    } while (0)
-#else
-#define DBGOUT(what, fmt, ...) do {} while (0)
-#endif
-
 #define IOPORT_SIZE       0x40
 #define PNPMMIO_SIZE      0x20000
 
@@ -351,8 +331,7 @@ e1000_mit_timer(void *opaque)
 static void
 set_ics(E1000State *s, int index, uint32_t val)
 {
-    DBGOUT(INTERRUPT, "set_ics %x, ICR %x, IMR %x\n", val, s->mac_reg[ICR],
-        s->mac_reg[IMS]);
+    trace_e1000_set_ics(val, s->mac_reg[ICR], s->mac_reg[IMS]);
     set_interrupt_cause(s, 0, val | s->mac_reg[ICR]);
 }
 
@@ -425,8 +404,7 @@ set_rx_control(E1000State *s, int index, uint32_t val)
     s->mac_reg[RCTL] = val;
     s->rxbuf_size = e1000x_rxbufsize(val);
     s->rxbuf_min_shift = ((val / E1000_RCTL_RDMTS_QUAT) & 3) + 1;
-    DBGOUT(RX, "RCTL: %d, mac_reg[RCTL] = 0x%x\n", s->mac_reg[RDT],
-           s->mac_reg[RCTL]);
+    trace_e1000_set_rx_control(s->mac_reg[RDT], s->mac_reg[RCTL]);
     timer_mod(s->flush_queue_timer,
               qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000);
 }
@@ -440,16 +418,16 @@ set_mdic(E1000State *s, int index, uint32_t val)
     if ((val & E1000_MDIC_PHY_MASK) >> E1000_MDIC_PHY_SHIFT != 1) // phy #
         val = s->mac_reg[MDIC] | E1000_MDIC_ERROR;
     else if (val & E1000_MDIC_OP_READ) {
-        DBGOUT(MDIC, "MDIC read reg 0x%x\n", addr);
+        trace_e1000_mdic_read_register(addr);
         if (!(phy_regcap[addr] & PHY_R)) {
-            DBGOUT(MDIC, "MDIC read reg %x unhandled\n", addr);
+            trace_e1000_mdic_read_register_unhandled(addr);
             val |= E1000_MDIC_ERROR;
         } else
             val = (val ^ data) | s->phy_reg[addr];
     } else if (val & E1000_MDIC_OP_WRITE) {
-        DBGOUT(MDIC, "MDIC write reg 0x%x, value 0x%x\n", addr, data);
+        trace_e1000_mdic_write_register(addr, data);
         if (!(phy_regcap[addr] & PHY_W)) {
-            DBGOUT(MDIC, "MDIC write reg %x unhandled\n", addr);
+            trace_e1000_mdic_write_register_unhandled(addr);
             val |= E1000_MDIC_ERROR;
         } else {
             if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
@@ -471,8 +449,8 @@ get_eecd(E1000State *s, int index)
 {
     uint32_t ret = E1000_EECD_PRES|E1000_EECD_GNT | s->eecd_state.old_eecd;
 
-    DBGOUT(EEPROM, "reading eeprom bit %d (reading %d)\n",
-           s->eecd_state.bitnum_out, s->eecd_state.reading);
+    trace_e1000_get_eecd(s->eecd_state.bitnum_out, s->eecd_state.reading);
+
     if (!s->eecd_state.reading ||
         ((s->eeprom_data[(s->eecd_state.bitnum_out >> 4) & 0x3f] >>
           ((s->eecd_state.bitnum_out & 0xf) ^ 0xf))) & 1)
@@ -511,9 +489,8 @@ set_eecd(E1000State *s, int index, uint32_t val)
         s->eecd_state.reading = (((s->eecd_state.val_in >> 6) & 7) ==
             EEPROM_READ_OPCODE_MICROWIRE);
     }
-    DBGOUT(EEPROM, "eeprom bitnum in %d out %d, reading %d\n",
-           s->eecd_state.bitnum_in, s->eecd_state.bitnum_out,
-           s->eecd_state.reading);
+    trace_e1000_set_eecd(s->eecd_state.bitnum_in, s->eecd_state.bitnum_out,
+                         s->eecd_state.reading);
 }
 
 static uint32_t
@@ -580,8 +557,7 @@ xmit_seg(E1000State *s)
 
     if (tp->cptse) {
         css = props->ipcss;
-        DBGOUT(TXSUM, "frames %d size %d ipcss %d\n",
-               frames, tp->size, css);
+        trace_e1000_xmit_seg1(frames, tp->size, css);
         if (props->ip) {    /* IPv4 */
             stw_be_p(tp->data+css+2, tp->size - css);
             stw_be_p(tp->data+css+4,
@@ -591,7 +567,7 @@ xmit_seg(E1000State *s)
         }
         css = props->tucss;
         len = tp->size - css;
-        DBGOUT(TXSUM, "tcp %d tucss %d len %d\n", props->tcp, css, len);
+        trace_e1000_xmit_seg2(props->tcp, css, len);
         if (props->tcp) {
             sofar = frames * props->mss;
             stl_be_p(tp->data+css+4, ldl_be_p(tp->data+css+4)+sofar); /* seq */
@@ -759,7 +735,7 @@ start_xmit(E1000State *s)
     uint32_t tdh_start = s->mac_reg[TDH], cause = E1000_ICS_TXQE;
 
     if (!(s->mac_reg[TCTL] & E1000_TCTL_EN)) {
-        DBGOUT(TX, "tx disabled\n");
+        trace_e1000_start_xmit_fail1();
         return;
     }
 
@@ -773,9 +749,9 @@ start_xmit(E1000State *s)
                sizeof(struct e1000_tx_desc) * s->mac_reg[TDH];
         pci_dma_read(d, base, &desc, sizeof(desc));
 
-        DBGOUT(TX, "index %d: %p : %x %x\n", s->mac_reg[TDH],
-               (void *)(intptr_t)desc.buffer_addr, desc.lower.data,
-               desc.upper.data);
+        trace_e1000_transmit(s->mac_reg[TDH],
+                             (void *)(intptr_t)desc.buffer_addr,
+                             desc.lower.data, desc.upper.data);
 
         process_tx_desc(s, &desc);
         cause |= txdesc_writeback(s, base, &desc);
@@ -789,8 +765,8 @@ start_xmit(E1000State *s)
          */
         if (s->mac_reg[TDH] == tdh_start ||
             tdh_start >= s->mac_reg[TDLEN] / sizeof(desc)) {
-            DBGOUT(TXERR, "TDH wraparound @%x, TDT %x, TDLEN %x\n",
-                   tdh_start, s->mac_reg[TDT], s->mac_reg[TDLEN]);
+            trace_e1000_start_xmit_fail2(tdh_start, s->mac_reg[TDT],
+                                         s->mac_reg[TDLEN]);
             break;
         }
     }
@@ -978,7 +954,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
                 desc.status &= ~E1000_RXD_STAT_EOP;
             }
         } else { // as per intel docs; skip descriptors with null buf addr
-            DBGOUT(RX, "Null RX descriptor!!\n");
+            trace_e1000_null_rx();
         }
         pci_dma_write(d, base, &desc, sizeof(desc));
         desc.status |= (vlan_status | E1000_RXD_STAT_DD);
@@ -990,8 +966,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
         /* see comment in start_xmit; same here */
         if (s->mac_reg[RDH] == rdh_start ||
             rdh_start >= s->mac_reg[RDLEN] / sizeof(desc)) {
-            DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n",
-                   rdh_start, s->mac_reg[RDT], s->mac_reg[RDLEN]);
+            trace_e1000_rdh_wraparound(rdh_start, s->mac_reg[RDT],
+                                       s->mac_reg[RDLEN]);
             e1000_receiver_overrun(s, total_size);
             return -1;
         }
@@ -1033,7 +1009,7 @@ mac_icr_read(E1000State *s, int index)
 {
     uint32_t ret = s->mac_reg[ICR];
 
-    DBGOUT(INTERRUPT, "ICR read: %x\n", ret);
+    trace_e1000_mac_icr_read(ret);
     set_interrupt_cause(s, 0, 0);
     return ret;
 }
@@ -1109,7 +1085,7 @@ set_tctl(E1000State *s, int index, uint32_t val)
 static void
 set_icr(E1000State *s, int index, uint32_t val)
 {
-    DBGOUT(INTERRUPT, "set_icr %x\n", val);
+    trace_e1000_set_icr(val);
     set_interrupt_cause(s, 0, s->mac_reg[ICR] & ~val);
 }
 
@@ -1271,20 +1247,16 @@ e1000_mmio_write(void *opaque, hwaddr addr, uint64_t val,
         if (!(mac_reg_access[index] & MAC_ACCESS_FLAG_NEEDED)
             || (s->compat_flags & (mac_reg_access[index] >> 2))) {
             if (mac_reg_access[index] & MAC_ACCESS_PARTIAL) {
-                DBGOUT(GENERAL, "Writing to register at offset: 0x%08x. "
-                       "It is not fully implemented.\n", index<<2);
+                trace_e1000_mmio_write_fail1(index << 2);
             }
             macreg_writeops[index](s, index, val);
         } else {    /* "flag needed" bit is set, but the flag is not active */
-            DBGOUT(MMIO, "MMIO write attempt to disabled reg. addr=0x%08x\n",
-                   index<<2);
+            trace_e1000_mmio_write_fail2(index << 2);
         }
     } else if (index < NREADOPS && macreg_readops[index]) {
-        DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04"PRIx64"\n",
-               index<<2, val);
+        trace_e1000_mmio_write_fail3(index << 2, val);
     } else {
-        DBGOUT(UNKNOWN, "MMIO unknown write addr=0x%08x,val=0x%08"PRIx64"\n",
-               index<<2, val);
+        trace_e1000_mmio_write_fail4(index << 2, val);
     }
 }
 
@@ -1298,16 +1270,14 @@ e1000_mmio_read(void *opaque, hwaddr addr, unsigned size)
         if (!(mac_reg_access[index] & MAC_ACCESS_FLAG_NEEDED)
             || (s->compat_flags & (mac_reg_access[index] >> 2))) {
             if (mac_reg_access[index] & MAC_ACCESS_PARTIAL) {
-                DBGOUT(GENERAL, "Reading register at offset: 0x%08x. "
-                       "It is not fully implemented.\n", index<<2);
+                trace_e1000_mmio_read_fail1(index << 2);
             }
             return macreg_readops[index](s, index);
         } else {    /* "flag needed" bit is set, but the flag is not active */
-            DBGOUT(MMIO, "MMIO read attempt of disabled reg. addr=0x%08x\n",
-                   index<<2);
+            trace_e1000_mmio_read_fail2(index << 2);
         }
     } else {
-        DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
+        trace_e1000_mmio_read_fail3(index << 2);
     }
     return 0;
 }
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 78efa2ec2c..f426f79a0c 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -101,7 +101,31 @@ net_rx_pkt_rss_hash(size_t rss_length, uint32_t rss_hash) "RSS hash for %zu byte
 net_rx_pkt_rss_add_chunk(void* ptr, size_t size, size_t input_offset) "Add RSS chunk %p, %zu bytes, RSS input offset %zu bytes"
 
 # e1000.c
+e1000_set_ics(uint32_t val, uint32_t ICR, uint32_t IMR) "set_ics 0x%x, ICR 0x%x, IMR 0x%x"
+e1000_set_rx_control(uint32_t RDT, uint32_t RCTL) "RCTL: %d, mac_reg[RCTL] = 0x%x"
+e1000_mdic_read_register(uint32_t addr) "MDIC read reg 0x%x"
+e1000_mdic_read_register_unhandled(uint32_t addr) "MDIC read reg 0x%x unhandled"
+e1000_mdic_write_register(uint32_t addr, uint32_t val) "MDIC write reg 0x%x, value 0x%x"
+e1000_mdic_write_register_unhandled(uint32_t addr) "MDIC write reg 0x%x unhandled"
+e1000_transmit(uint32_t tdh, void *addr, uint32_t data_low, uint32_t data_high) "index %d: %p : 0x%x 0x%x"
+e1000_get_eecd(uint16_t bitnum_out, uint16_t reading) "reading eeprom bit %d (reading %d)"
+e1000_set_eecd(uint16_t bitnum_in, uint16_t bitnum_out, uint16_t reading) "eeprom bitnum in %d out %d, reading %d"
+e1000_xmit_seg1(unsigned int frames, uint16_t size, unsigned int css) "frames %d size %d ipcss %d"
+e1000_xmit_seg2(int8_t tcp, uint16_t css, unsigned int len) "tcp %d tucss %d len %d"
+e1000_start_xmit_fail1(void) "tx disabled"
+e1000_start_xmit_fail2(uint32_t tdh_start, uint32_t TDT, uint32_t TDLEN) "TDH wraparound @0x%x, TDT 0x%x, TDLEN 0x%x"
 e1000_receiver_overrun(size_t s, uint32_t rdh, uint32_t rdt) "Receiver overrun: dropped packet of %zu bytes, RDH=%u, RDT=%u"
+e1000_null_rx(void) "Null RX descriptor!!"
+e1000_rdh_wraparound(uint32_t rdh_start, uint32_t RDT, uint32_t RDLEN) "RDH wraparound @0x%x, RDT 0x%x, RDLEN 0x%x"
+e1000_mac_icr_read(uint32_t ret) "ICR read: 0x%x"
+e1000_set_icr(uint32_t val) "set_icr 0x%x"
+e1000_mmio_write_fail1(unsigned int index) "Writing to register at offset: 0x%08x. It is not fully implemented."
+e1000_mmio_write_fail2(unsigned int index) "MMIO write attempt to disabled reg. addr=0x%08x"
+e1000_mmio_write_fail3(unsigned int index, uint64_t val) "e1000_mmio_writel RO 0x%x: 0x%04"PRIx64""
+e1000_mmio_write_fail4(unsigned int index, uint64_t val) "MMIO unknown write addr=0x%08x,val=0x%08"PRIx64""
+e1000_mmio_read_fail1(unsigned int index) "Reading register at offset: 0x%08x. It is not fully implemented."
+e1000_mmio_read_fail2(unsigned int index) "MMIO read attempt of disabled reg. addr=0x%08x"
+e1000_mmio_read_fail3(unsigned int index) "MMIO unknown read addr=0x%08x"
 
 # e1000x_common.c
 e1000x_rx_can_recv_disabled(bool link_up, bool rx_enabled, bool pci_master) "link_up: %d, rx_enabled %d, pci_master %d"
@@ -146,7 +170,6 @@ e1000e_wrn_no_snap_support(void) "WARNING: Guest requested TX SNAP header update
 e1000e_wrn_iscsi_filtering_not_supported(void) "WARNING: Guest requested iSCSI filtering  which is not supported"
 e1000e_wrn_nfsw_filtering_not_supported(void) "WARNING: Guest requested NFS write filtering  which is not supported"
 e1000e_wrn_nfsr_filtering_not_supported(void) "WARNING: Guest requested NFS read filtering  which is not supported"
-
 e1000e_tx_disabled(void) "TX Disabled"
 e1000e_tx_descr(void *addr, uint32_t lower, uint32_t upper) "%p : %x %x"
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-04-08 21:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-29 15:04 [PATCH 0/1] Upstreaming Course Debugging Changes Don Porter
2024-03-29 15:04 ` [PATCH 1/1] e1000: Get debug flags from an environment variable Don Porter
2024-03-29 17:46   ` Richard Henderson
2024-04-03 13:45     ` [PATCH v2] e1000: Convert debug macros into tracepoints Don Porter
2024-04-03 17:51       ` Richard Henderson
2024-04-03 18:44       ` Austin Clements
2024-04-08 21:11         ` Don Porter
2024-04-08 21:15         ` [PATCH v3] " Don Porter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).