Re: [PATCH v2] e1000: Convert debug macros into tracepoints.

2024-04-08 Thread Don Porter

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 (jasonw...@redhat.com) bounced.  Any pointers 
on who else should be cc-ed are appreciated.


-dp




Re: [PATCH v2] e1000: Convert debug macros into tracepoints.

2024-04-03 Thread Austin Clements
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  wrote:

> From: Austin Clements 
>
> The E1000 debug messages are very useful for developing drivers.
> Make these available to users without recompiling QEMU.
>
> Signed-off-by: Austin Clements 
> [geo...@ldpreload.com: Rebased on top of 2.9.0]
> Signed-off-by: Geoffrey Thomas 
> Signed-off-by: Don Porter 
> ---
>  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< -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  0x2
>
> @@ -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);
>   

Re: [PATCH v2] e1000: Convert debug macros into tracepoints.

2024-04-03 Thread Richard Henderson

On 4/3/24 03:45, Don Porter wrote:

From: Austin Clements

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

Signed-off-by: Austin Clements
[geo...@ldpreload.com: Rebased on top of 2.9.0]
Signed-off-by: Geoffrey Thomas
Signed-off-by: Don Porter
---
  hw/net/e1000.c  | 90 +++--
  hw/net/trace-events | 25 -
  2 files changed, 54 insertions(+), 61 deletions(-)


Reviewed-by: Richard Henderson 


r~



[PATCH v2] e1000: Convert debug macros into tracepoints.

2024-04-03 Thread Don Porter
From: Austin Clements 

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

Signed-off-by: Austin Clements 
[geo...@ldpreload.com: Rebased on top of 2.9.0]
Signed-off-by: Geoffrey Thomas 
Signed-off-by: Don Porter 
---
 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)(1mac_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, , sizeof(desc));
 
-DBGOUT(TX, "index %d: %p : %x %x\n", s->mac_reg[TDH],