[PATCH 0/4] Several fixes of AXI-ethernet/DMA

[PATCH 0/4] Several fixes of AXI-ethernet/DMA

[Jim Shu]

Fix txlen value in the descriptor status field, DMA error handling,
and ethernet/DMA reset flow.

*** BLURB HERE ***

Jim Shu (4):
  hw/dma: xilinx_axidma: Correct the txlen value in the descriptor
  hw/dma: xilinx_axidma: Send DMA error IRQ if any memory access is
    failed
  hw/dma: xilinx_axidma: Reset qemu_irq when DMA/Stream is reset
  hw/net: xilinx_axienet: Fix DMA RX IRQ if ethernet disable RX

 hw/dma/trace-events     |  1 +
 hw/dma/xilinx_axidma.c  | 97 +++++++++++++++++++++++++++--------------
 hw/net/xilinx_axienet.c | 71 ++++++++++++++++++------------
 3 files changed, 107 insertions(+), 62 deletions(-)

-- 
2.17.1

[PATCH 1/4] hw/dma: xilinx_axidma: Correct the txlen value in the descriptor

[Jim Shu]

Currently, txlen is always decremented to 0 before filling to the
descriptor. Keep the origin txlen value to have the correct value of
descriptor status field.

It will fix the 'tx_bytes' statistic value in linux axi-ethernet driver.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/dma/xilinx_axidma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index c9cfc3169b..6aa8c9272c 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -291,7 +291,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
                                  StreamSink *tx_control_dev)
 {
     uint32_t prev_d;
-    uint32_t txlen;
+    uint32_t txlen, origin_txlen;
     uint64_t addr;
     bool eop;
 
@@ -314,6 +314,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
         }
 
         txlen = s->desc.control & SDESC_CTRL_LEN_MASK;
+        origin_txlen = txlen;
 
         eop = stream_desc_eof(&s->desc);
         addr = s->desc.buffer_address;
@@ -334,7 +335,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
         }
 
         /* Update the descriptor.  */
-        s->desc.status = txlen | SDESC_STATUS_COMPLETE;
+        s->desc.status = origin_txlen | SDESC_STATUS_COMPLETE;
         stream_desc_store(s, s->regs[R_CURDESC]);
 
         /* Advance.  */
-- 
2.17.1

[PATCH 2/4] hw/dma: xilinx_axidma: Send DMA error IRQ if any memory access is failed

[Jim Shu]

The memory transactions from DMA could have bus-error in some cases. If
it is failed, DMA device should send error IRQs.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/dma/trace-events    |  1 +
 hw/dma/xilinx_axidma.c | 69 ++++++++++++++++++++++++++++++------------
 2 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/hw/dma/trace-events b/hw/dma/trace-events
index 4c09790f9a..7db38e0e93 100644
--- a/hw/dma/trace-events
+++ b/hw/dma/trace-events
@@ -47,3 +47,4 @@ pl330_iomem_read(uint32_t addr, uint32_t data) "addr: 0x%08"PRIx32" data: 0x%08"
 
 # xilinx_axidma.c
 xilinx_axidma_loading_desc_fail(uint32_t res) "error:%u"
+xilinx_axidma_storing_desc_fail(uint32_t res) "error:%u"
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 6aa8c9272c..728246f925 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -194,6 +194,20 @@ static inline int streamid_from_addr(hwaddr addr)
     return sid;
 }
 
+/* When DMA is error, fill in the register of this Stream. */
+static void stream_dma_error(struct Stream *s, MemTxResult result)
+{
+    if (result == MEMTX_DECODE_ERROR) {
+        s->regs[R_DMASR] |= DMASR_DECERR;
+    } else {
+        s->regs[R_DMASR] |= DMASR_SLVERR;
+    }
+
+    s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
+    s->regs[R_DMASR] |= DMASR_HALTED;
+    s->regs[R_DMASR] |= DMASR_ERR_IRQ;
+}
+
 static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
 {
     struct SDesc *d = &s->desc;
@@ -203,16 +217,7 @@ static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
                                             d, sizeof *d);
     if (result != MEMTX_OK) {
         trace_xilinx_axidma_loading_desc_fail(result);
-
-        if (result == MEMTX_DECODE_ERROR) {
-            s->regs[R_DMASR] |= DMASR_DECERR;
-        } else {
-            s->regs[R_DMASR] |= DMASR_SLVERR;
-        }
-
-        s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
-        s->regs[R_DMASR] |= DMASR_HALTED;
-        s->regs[R_DMASR] |= DMASR_ERR_IRQ;
+        stream_dma_error(s, result);
         return result;
     }
 
@@ -224,17 +229,24 @@ static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
     return result;
 }
 
-static void stream_desc_store(struct Stream *s, hwaddr addr)
+static MemTxResult stream_desc_store(struct Stream *s, hwaddr addr)
 {
     struct SDesc *d = &s->desc;
+    MemTxResult result;
 
     /* Convert from host endianness into LE.  */
     d->buffer_address = cpu_to_le64(d->buffer_address);
     d->nxtdesc = cpu_to_le64(d->nxtdesc);
     d->control = cpu_to_le32(d->control);
     d->status = cpu_to_le32(d->status);
-    address_space_write(&s->dma->as, addr, MEMTXATTRS_UNSPECIFIED,
-                        d, sizeof *d);
+    result = address_space_write(&s->dma->as, addr, MEMTXATTRS_UNSPECIFIED,
+                                 d, sizeof *d);
+
+    if (result != MEMTX_OK) {
+        trace_xilinx_axidma_storing_desc_fail(result);
+        stream_dma_error(s, result);
+    }
+    return result;
 }
 
 static void stream_update_irq(struct Stream *s)
@@ -294,6 +306,7 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
     uint32_t txlen, origin_txlen;
     uint64_t addr;
     bool eop;
+    MemTxResult result;
 
     if (!stream_running(s) || stream_idle(s) || stream_halted(s)) {
         return;
@@ -322,9 +335,14 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
             unsigned int len;
 
             len = txlen > sizeof s->txbuf ? sizeof s->txbuf : txlen;
-            address_space_read(&s->dma->as, addr,
-                               MEMTXATTRS_UNSPECIFIED,
-                               s->txbuf, len);
+            result = address_space_read(&s->dma->as, addr,
+                                        MEMTXATTRS_UNSPECIFIED,
+                                        s->txbuf, len);
+            if (result != MEMTX_OK) {
+                stream_dma_error(s, result);
+                return;
+            }
+
             stream_push(tx_data_dev, s->txbuf, len, eop && len == txlen);
             txlen -= len;
             addr += len;
@@ -336,7 +354,9 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
 
         /* Update the descriptor.  */
         s->desc.status = origin_txlen | SDESC_STATUS_COMPLETE;
-        stream_desc_store(s, s->regs[R_CURDESC]);
+        if (stream_desc_store(s, s->regs[R_CURDESC]) != MEMTX_OK) {
+            break;
+        }
 
         /* Advance.  */
         prev_d = s->regs[R_CURDESC];
@@ -354,6 +374,7 @@ static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
     uint32_t prev_d;
     unsigned int rxlen;
     size_t pos = 0;
+    MemTxResult result;
 
     if (!stream_running(s) || stream_idle(s) || stream_halted(s)) {
         return 0;
@@ -375,8 +396,13 @@ static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
             rxlen = len;
         }
 
-        address_space_write(&s->dma->as, s->desc.buffer_address,
-                            MEMTXATTRS_UNSPECIFIED, buf + pos, rxlen);
+        result = address_space_write(&s->dma->as, s->desc.buffer_address,
+                                     MEMTXATTRS_UNSPECIFIED, buf + pos, rxlen);
+        if (result != MEMTX_OK) {
+            stream_dma_error(s, result);
+            break;
+        }
+
         len -= rxlen;
         pos += rxlen;
 
@@ -389,7 +415,10 @@ static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
 
         s->desc.status |= s->sof << SDESC_STATUS_SOF_BIT;
         s->desc.status |= SDESC_STATUS_COMPLETE;
-        stream_desc_store(s, s->regs[R_CURDESC]);
+        result = stream_desc_store(s, s->regs[R_CURDESC]);
+        if (result != MEMTX_OK) {
+            break;
+        }
         s->sof = eop;
 
         /* Advance.  */
-- 
2.17.1

[PATCH 3/4] hw/dma: xilinx_axidma: Reset qemu_irq when DMA/Stream is reset

[Jim Shu]

Current DMA/Stream reset will clear interrupt pending bit of DMA device.
The qemu_irq of device should be updated at the same time.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/dma/xilinx_axidma.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 728246f925..beb3fbf1d5 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -177,11 +177,24 @@ static inline int stream_halted(struct Stream *s)
     return !!(s->regs[R_DMASR] & DMASR_HALTED);
 }
 
+static void stream_update_irq(struct Stream *s)
+{
+    unsigned int pending, mask, irq;
+
+    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
+    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
+
+    irq = pending & mask;
+
+    qemu_set_irq(s->irq, !!irq);
+}
+
 static void stream_reset(struct Stream *s)
 {
     s->regs[R_DMASR] = DMASR_HALTED;  /* starts up halted.  */
     s->regs[R_DMACR] = 1 << 16; /* Starts with one in compl threshold.  */
     s->sof = true;
+    stream_update_irq(s); /* Clear interrupt */
 }
 
 /* Map an offset addr into a channel index.  */
@@ -249,18 +262,6 @@ static MemTxResult stream_desc_store(struct Stream *s, hwaddr addr)
     return result;
 }
 
-static void stream_update_irq(struct Stream *s)
-{
-    unsigned int pending, mask, irq;
-
-    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
-    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
-
-    irq = pending & mask;
-
-    qemu_set_irq(s->irq, !!irq);
-}
-
 static void stream_reload_complete_cnt(struct Stream *s)
 {
     unsigned int comp_th;
-- 
2.17.1

Re: [PATCH 3/4] hw/dma: xilinx_axidma: Reset qemu_irq when DMA/Stream is reset

[Peter Maydell]

On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote:
>
> Current DMA/Stream reset will clear interrupt pending bit of DMA device.
> The qemu_irq of device should be updated at the same time.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>  hw/dma/xilinx_axidma.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 728246f925..beb3fbf1d5 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -177,11 +177,24 @@ static inline int stream_halted(struct Stream *s)
>      return !!(s->regs[R_DMASR] & DMASR_HALTED);
>  }
>
> +static void stream_update_irq(struct Stream *s)
> +{
> +    unsigned int pending, mask, irq;
> +
> +    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
> +    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
> +
> +    irq = pending & mask;
> +
> +    qemu_set_irq(s->irq, !!irq);
> +}
> +
>  static void stream_reset(struct Stream *s)
>  {
>      s->regs[R_DMASR] = DMASR_HALTED;  /* starts up halted.  */
>      s->regs[R_DMACR] = 1 << 16; /* Starts with one in compl threshold.  */
>      s->sof = true;
> +    stream_update_irq(s); /* Clear interrupt */
>  }

The general rule of thumb in QEMU is not to call
qemu_set_irq() from a DeviceState::reset function,
and xilinx_axidma_reset calls stream_reset. I think
probably the best thing is to separate out whether
this is a DeviceState::reset or a software-triggered
reset, and only call qemu_set_irq() in the latter case.

thanks
-- PMM

Re: [PATCH 3/4] hw/dma: xilinx_axidma: Reset qemu_irq when DMA/Stream is reset

[Jim Shu]

Hi Peter,

Except DeviceState::reset(), stream_reset() is only used in
axidma_write() and axidma_write() has qemu_set_irq() at the end of
function.
I think this commit could be dropped. I will remove it in the v2 patchset.


Thanks,
Jim Shu



On Mon, Jul 29, 2024 at 11:23 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote:
> >
> > Current DMA/Stream reset will clear interrupt pending bit of DMA device.
> > The qemu_irq of device should be updated at the same time.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >  hw/dma/xilinx_axidma.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> > index 728246f925..beb3fbf1d5 100644
> > --- a/hw/dma/xilinx_axidma.c
> > +++ b/hw/dma/xilinx_axidma.c
> > @@ -177,11 +177,24 @@ static inline int stream_halted(struct Stream *s)
> >      return !!(s->regs[R_DMASR] & DMASR_HALTED);
> >  }
> >
> > +static void stream_update_irq(struct Stream *s)
> > +{
> > +    unsigned int pending, mask, irq;
> > +
> > +    pending = s->regs[R_DMASR] & DMASR_IRQ_MASK;
> > +    mask = s->regs[R_DMACR] & DMASR_IRQ_MASK;
> > +
> > +    irq = pending & mask;
> > +
> > +    qemu_set_irq(s->irq, !!irq);
> > +}
> > +
> >  static void stream_reset(struct Stream *s)
> >  {
> >      s->regs[R_DMASR] = DMASR_HALTED;  /* starts up halted.  */
> >      s->regs[R_DMACR] = 1 << 16; /* Starts with one in compl threshold.  */
> >      s->sof = true;
> > +    stream_update_irq(s); /* Clear interrupt */
> >  }
>
> The general rule of thumb in QEMU is not to call
> qemu_set_irq() from a DeviceState::reset function,
> and xilinx_axidma_reset calls stream_reset. I think
> probably the best thing is to separate out whether
> this is a DeviceState::reset or a software-triggered
> reset, and only call qemu_set_irq() in the latter case.
>
> thanks
> -- PMM

[PATCH 4/4] hw/net: xilinx_axienet: Fix DMA RX IRQ if ethernet disable RX

[Jim Shu]

When AXI ethernet RX is disabled, it shouldn't send packets to AXI DMA,
which may let AXI DMA to send RX full IRQs. It is aligned with real AXI
DMA/ethernet IP behavior in the FPGA.

Also, now ethernet RX blocks the RX packets when it is disabled. It
should send and clear the remaining RX packets when enabling it.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/net/xilinx_axienet.c | 71 ++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 05d41bd548..8428f10946 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -530,6 +530,40 @@ static uint64_t enet_read(void *opaque, hwaddr addr, unsigned size)
     return r;
 }
 
+static void axienet_eth_rx_notify(void *opaque)
+{
+    XilinxAXIEnet *s = XILINX_AXI_ENET(opaque);
+
+    /* If RX is disabled, don't trigger DMA to update RX desc and send IRQ */
+    if (!axienet_rx_enabled(s)) {
+        return;
+    }
+
+    while (s->rxappsize && stream_can_push(s->tx_control_dev,
+                                           axienet_eth_rx_notify, s)) {
+        size_t ret = stream_push(s->tx_control_dev,
+                                 (void *)s->rxapp + CONTROL_PAYLOAD_SIZE
+                                 - s->rxappsize, s->rxappsize, true);
+        s->rxappsize -= ret;
+    }
+
+    while (s->rxsize && stream_can_push(s->tx_data_dev,
+                                        axienet_eth_rx_notify, s)) {
+        size_t ret = stream_push(s->tx_data_dev, (void *)s->rxmem + s->rxpos,
+                                 s->rxsize, true);
+        s->rxsize -= ret;
+        s->rxpos += ret;
+        if (!s->rxsize) {
+            s->regs[R_IS] |= IS_RX_COMPLETE;
+            if (s->need_flush) {
+                s->need_flush = false;
+                qemu_flush_queued_packets(qemu_get_queue(s->nic));
+            }
+        }
+    }
+    enet_update_irq(s);
+}
+
 static void enet_write(void *opaque, hwaddr addr,
                        uint64_t value, unsigned size)
 {
@@ -546,6 +580,14 @@ static void enet_write(void *opaque, hwaddr addr,
             } else {
                 qemu_flush_queued_packets(qemu_get_queue(s->nic));
             }
+
+            /*
+             * When RX is enabled, check if any remaining data in rxmem
+             * and send them.
+             */
+            if ((addr & 1) && s->rcw[addr & 1] & RCW1_RX) {
+                axienet_eth_rx_notify(s);
+            }
             break;
 
         case R_TC:
@@ -666,35 +708,6 @@ static int enet_match_addr(const uint8_t *buf, uint32_t f0, uint32_t f1)
     return match;
 }
 
-static void axienet_eth_rx_notify(void *opaque)
-{
-    XilinxAXIEnet *s = XILINX_AXI_ENET(opaque);
-
-    while (s->rxappsize && stream_can_push(s->tx_control_dev,
-                                           axienet_eth_rx_notify, s)) {
-        size_t ret = stream_push(s->tx_control_dev,
-                                 (void *)s->rxapp + CONTROL_PAYLOAD_SIZE
-                                 - s->rxappsize, s->rxappsize, true);
-        s->rxappsize -= ret;
-    }
-
-    while (s->rxsize && stream_can_push(s->tx_data_dev,
-                                        axienet_eth_rx_notify, s)) {
-        size_t ret = stream_push(s->tx_data_dev, (void *)s->rxmem + s->rxpos,
-                                 s->rxsize, true);
-        s->rxsize -= ret;
-        s->rxpos += ret;
-        if (!s->rxsize) {
-            s->regs[R_IS] |= IS_RX_COMPLETE;
-            if (s->need_flush) {
-                s->need_flush = false;
-                qemu_flush_queued_packets(qemu_get_queue(s->nic));
-            }
-        }
-    }
-    enet_update_irq(s);
-}
-
 static ssize_t eth_rx(NetClientState *nc, const uint8_t *buf, size_t size)
 {
     XilinxAXIEnet *s = qemu_get_nic_opaque(nc);
-- 
2.17.1

Re: [PATCH 4/4] hw/net: xilinx_axienet: Fix DMA RX IRQ if ethernet disable RX

[Peter Maydell]

On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote:
>
> When AXI ethernet RX is disabled, it shouldn't send packets to AXI DMA,
> which may let AXI DMA to send RX full IRQs. It is aligned with real AXI
> DMA/ethernet IP behavior in the FPGA.
>
> Also, now ethernet RX blocks the RX packets when it is disabled. It
> should send and clear the remaining RX packets when enabling it.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>  hw/net/xilinx_axienet.c | 71 ++++++++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index 05d41bd548..8428f10946 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -530,6 +530,40 @@ static uint64_t enet_read(void *opaque, hwaddr addr, unsigned size)
>      return r;
>  }
>
> +static void axienet_eth_rx_notify(void *opaque)
> +{
> +    XilinxAXIEnet *s = XILINX_AXI_ENET(opaque);
> +
> +    /* If RX is disabled, don't trigger DMA to update RX desc and send IRQ */
> +    if (!axienet_rx_enabled(s)) {
> +        return;
> +    }

This checks s->rcw[1] & RCW1_RX, and does nothing if it's not set...

>  static void enet_write(void *opaque, hwaddr addr,
>                         uint64_t value, unsigned size)
>  {
> @@ -546,6 +580,14 @@ static void enet_write(void *opaque, hwaddr addr,
>              } else {
>                  qemu_flush_queued_packets(qemu_get_queue(s->nic));
>              }
> +
> +            /*
> +             * When RX is enabled, check if any remaining data in rxmem
> +             * and send them.
> +             */
> +            if ((addr & 1) && s->rcw[addr & 1] & RCW1_RX) {
> +                axienet_eth_rx_notify(s);
> +            }

...but at this callsite we open-code a check on RCW1_RX and
skip the call if it's not set. We don't need to check twice.

>              break;
>
>          case R_TC:

thanks
-- PMM

Re: [PATCH 4/4] hw/net: xilinx_axienet: Fix DMA RX IRQ if ethernet disable RX

[Jim Shu]

Hi Peter,

Thanks for the suggestion.

axienet_eth_rx_notify() is also called by axidma_write() as a notify()
callback, so we need to check RCW1_RX in the function.
I think I could remove RCW1_RX checking in the enet_write() to avoid
double checking.
I will fix it in the v2 patchset.


Thanks,
JIm Shu.

On Mon, Jul 29, 2024 at 11:31 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 Jul 2024 at 06:59, Jim Shu <jim.shu@sifive.com> wrote:
> >
> > When AXI ethernet RX is disabled, it shouldn't send packets to AXI DMA,
> > which may let AXI DMA to send RX full IRQs. It is aligned with real AXI
> > DMA/ethernet IP behavior in the FPGA.
> >
> > Also, now ethernet RX blocks the RX packets when it is disabled. It
> > should send and clear the remaining RX packets when enabling it.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >  hw/net/xilinx_axienet.c | 71 ++++++++++++++++++++++++-----------------
> >  1 file changed, 42 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> > index 05d41bd548..8428f10946 100644
> > --- a/hw/net/xilinx_axienet.c
> > +++ b/hw/net/xilinx_axienet.c
> > @@ -530,6 +530,40 @@ static uint64_t enet_read(void *opaque, hwaddr addr, unsigned size)
> >      return r;
> >  }
> >
> > +static void axienet_eth_rx_notify(void *opaque)
> > +{
> > +    XilinxAXIEnet *s = XILINX_AXI_ENET(opaque);
> > +
> > +    /* If RX is disabled, don't trigger DMA to update RX desc and send IRQ */
> > +    if (!axienet_rx_enabled(s)) {
> > +        return;
> > +    }
>
> This checks s->rcw[1] & RCW1_RX, and does nothing if it's not set...
>
> >  static void enet_write(void *opaque, hwaddr addr,
> >                         uint64_t value, unsigned size)
> >  {
> > @@ -546,6 +580,14 @@ static void enet_write(void *opaque, hwaddr addr,
> >              } else {
> >                  qemu_flush_queued_packets(qemu_get_queue(s->nic));
> >              }
> > +
> > +            /*
> > +             * When RX is enabled, check if any remaining data in rxmem
> > +             * and send them.
> > +             */
> > +            if ((addr & 1) && s->rcw[addr & 1] & RCW1_RX) {
> > +                axienet_eth_rx_notify(s);
> > +            }
>
> ...but at this callsite we open-code a check on RCW1_RX and
> skip the call if it's not set. We don't need to check twice.
>
> >              break;
> >
> >          case R_TC:
>
> thanks
> -- PMM