[PATCH 0/4] hw/dma: Add error handling for loading descriptions failing

[PATCH 0/4] hw/dma: Add error handling for loading descriptions failing

[Fea.Wang]

The original code assumes that memory transmission is always successful,
but in some cases, it gets bus-error.

Add error handling for DMA reading description failures. Do some
reasonable settings, and return the corrected transmission size.
Finally, return the error status.

base-commit: 1806da76cb81088ea026ca3441551782b850e393

Fea.Wang (4):
  hw/dma: Enhance error handling in loading description
  hw/dma: Break the loop when loading descriptions fails
  hw/dma: Add a trace log for a description loading failure
  hw/net: Fix the transmission return size

 hw/dma/trace-events     |  3 +++
 hw/dma/xilinx_axidma.c  | 27 +++++++++++++++++++++++----
 hw/net/xilinx_axienet.c |  2 +-
 3 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/4] hw/dma: Enhance error handling in loading description

[Fea.Wang]

Loading a description from memory may cause a bus-error. In this
case, the DMA should stop working, set the error flag, and return
the error value.

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
---
 hw/dma/xilinx_axidma.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 0ae056ed06..4b475e5484 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -71,8 +71,10 @@ enum {
 enum {
     DMASR_HALTED = 1,
     DMASR_IDLE  = 2,
+    DMASR_SLVERR = 1 << 5,
     DMASR_IOC_IRQ  = 1 << 12,
     DMASR_DLY_IRQ  = 1 << 13,
+    DMASR_ERR_IRQ  = 1 << 14,
 
     DMASR_IRQ_MASK = 7 << 12
 };
@@ -190,17 +192,27 @@ static inline int streamid_from_addr(hwaddr addr)
     return sid;
 }
 
-static void stream_desc_load(struct Stream *s, hwaddr addr)
+static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
 {
     struct SDesc *d = &s->desc;
 
-    address_space_read(&s->dma->as, addr, MEMTXATTRS_UNSPECIFIED, d, sizeof *d);
+    MemTxResult result = address_space_read(&s->dma->as,
+                                            addr, MEMTXATTRS_UNSPECIFIED,
+                                            d, sizeof *d);
+    if (result != MEMTX_OK) {
+        s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
+        s->regs[R_DMASR] |= DMASR_HALTED;
+        s->regs[R_DMASR] |= DMASR_SLVERR;
+        s->regs[R_DMASR] |= DMASR_ERR_IRQ;
+        return result;
+    }
 
     /* Convert from LE into host endianness.  */
     d->buffer_address = le64_to_cpu(d->buffer_address);
     d->nxtdesc = le64_to_cpu(d->nxtdesc);
     d->control = le32_to_cpu(d->control);
     d->status = le32_to_cpu(d->status);
+    return result;
 }
 
 static void stream_desc_store(struct Stream *s, hwaddr addr)
-- 
2.34.1

[PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

[Fea.Wang]

When calling the loading a description function, it should be noticed
that the function may return a failure value. Breaking the loop is one
of the possible ways to handle it.

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
---
 hw/dma/xilinx_axidma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 4b475e5484..b8ab5a423d 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
     }
 
     while (1) {
-        stream_desc_load(s, s->regs[R_CURDESC]);
+        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
+            break;
+        }
 
         if (s->desc.status & SDESC_STATUS_COMPLETE) {
             s->regs[R_DMASR] |= DMASR_HALTED;
@@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
     }
 
     while (len) {
-        stream_desc_load(s, s->regs[R_CURDESC]);
+        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
+            break;
+        }
 
         if (s->desc.status & SDESC_STATUS_COMPLETE) {
             s->regs[R_DMASR] |= DMASR_HALTED;
-- 
2.34.1

[PATCH 3/4] hw/dma: Add a trace log for a description loading failure

[Fea.Wang]

Due to a description loading failure, adding a trace log makes observing
the DMA behavior easy.

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
---
 hw/dma/trace-events    | 3 +++
 hw/dma/xilinx_axidma.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/hw/dma/trace-events b/hw/dma/trace-events
index 3c47df54e4..95db083be4 100644
--- a/hw/dma/trace-events
+++ b/hw/dma/trace-events
@@ -44,3 +44,6 @@ pl330_debug_exec_stall(void) "stall of debug instruction not implemented"
 pl330_iomem_write(uint32_t offset, uint32_t value) "addr: 0x%08"PRIx32" data: 0x%08"PRIx32
 pl330_iomem_write_clr(int i) "event interrupt lowered %d"
 pl330_iomem_read(uint32_t addr, uint32_t data) "addr: 0x%08"PRIx32" data: 0x%08"PRIx32
+
+# xilinx_axidma.c
+xilinx_axidma_loading_desc_fail(uint32_t res) "error:%d"
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index b8ab5a423d..1bbb9d6c4c 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -36,6 +36,7 @@
 #include "sysemu/dma.h"
 #include "hw/stream.h"
 #include "qom/object.h"
+#include "trace.h"
 
 #define D(x)
 
@@ -200,6 +201,8 @@ static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
                                             addr, MEMTXATTRS_UNSPECIFIED,
                                             d, sizeof *d);
     if (result != MEMTX_OK) {
+        trace_xilinx_axidma_loading_desc_fail(result);
+
         s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
         s->regs[R_DMASR] |= DMASR_HALTED;
         s->regs[R_DMASR] |= DMASR_SLVERR;
-- 
2.34.1

[PATCH 4/4] hw/net: Fix the transmission return size

[Fea.Wang]

Fix the transmission return size because not all bytes could be
transmitted successfully. So, return a successful length instead of a
constant value.

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
---
 hw/net/xilinx_axienet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 7d1fd37b4a..05d41bd548 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -847,7 +847,7 @@ static ssize_t eth_rx(NetClientState *nc, const uint8_t *buf, size_t size)
     axienet_eth_rx_notify(s);
 
     enet_update_irq(s);
-    return size;
+    return s->rxpos;
 }
 
 static size_t
-- 
2.34.1

Re: [PATCH 1/4] hw/dma: Enhance error handling in loading description

[Edgar E. Iglesias]

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

On Mon, Jun 3, 2024 at 7:47 AM Fea.Wang <fea.wang@sifive.com> wrote:

> Loading a description from memory may cause a bus-error. In this
> case, the DMA should stop working, set the error flag, and return
> the error value.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
>


Hi Fea,

I've got a couple of small comments:


---
>  hw/dma/xilinx_axidma.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 0ae056ed06..4b475e5484 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -71,8 +71,10 @@ enum {
>  enum {
>      DMASR_HALTED = 1,
>      DMASR_IDLE  = 2,
> +    DMASR_SLVERR = 1 << 5,
>

We should also add DMASR_DECERR = 1 << 6


>      DMASR_IOC_IRQ  = 1 << 12,
>      DMASR_DLY_IRQ  = 1 << 13,
> +    DMASR_ERR_IRQ  = 1 << 14,
>
>      DMASR_IRQ_MASK = 7 << 12
>  };
> @@ -190,17 +192,27 @@ static inline int streamid_from_addr(hwaddr addr)
>      return sid;
>  }
>
> -static void stream_desc_load(struct Stream *s, hwaddr addr)
> +static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
>  {
>      struct SDesc *d = &s->desc;
>
> -    address_space_read(&s->dma->as, addr, MEMTXATTRS_UNSPECIFIED, d,
> sizeof *d);
> +    MemTxResult result = address_space_read(&s->dma->as,
> +                                            addr, MEMTXATTRS_UNSPECIFIED,
> +                                            d, sizeof *d);
> +    if (result != MEMTX_OK) {
> +        s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
> +        s->regs[R_DMASR] |= DMASR_HALTED;
> +        s->regs[R_DMASR] |= DMASR_SLVERR;
>

... and map MEMTX_DECODE_ERROR to DMASR_DECERR and everything else to
SLVERR, for example:
if (result == MEMTX_DECODE_ERROR) {
    s->regs[R_DMASR] |= DMASR_DECERR;
} else {
    s->regs[R_DMASR] |= DMASR_SLVERR;
}


> +        s->regs[R_DMASR] |= DMASR_ERR_IRQ;
> +        return result;
> +    }
>
>      /* Convert from LE into host endianness.  */
>      d->buffer_address = le64_to_cpu(d->buffer_address);
>      d->nxtdesc = le64_to_cpu(d->nxtdesc);
>      d->control = le32_to_cpu(d->control);
>      d->status = le32_to_cpu(d->status);
> +    return result;
>  }
>
>  static void stream_desc_store(struct Stream *s, hwaddr addr)
> --
> 2.34.1
>
>

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

Re: [PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

[Edgar E. Iglesias]

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

On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang <fea.wang@sifive.com> wrote:

> When calling the loading a description function, it should be noticed
> that the function may return a failure value. Breaking the loop is one
> of the possible ways to handle it.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
>


Looks good, a nitpick comment, I would either squash this with patch #1
or move the change to return of error code in stream_desc_load() to this
patch.




> ---
>  hw/dma/xilinx_axidma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 4b475e5484..b8ab5a423d 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s,
> StreamSink *tx_data_dev,
>      }
>
>      while (1) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> @@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s,
> unsigned char *buf,
>      }
>
>      while (len) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> --
> 2.34.1
>
>

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

Re: [PATCH 3/4] hw/dma: Add a trace log for a description loading failure

[Edgar E. Iglesias]

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

On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang <fea.wang@sifive.com> wrote:

> Due to a description loading failure, adding a trace log makes observing
> the DMA behavior easy.
>
>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>



> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> ---
>  hw/dma/trace-events    | 3 +++
>  hw/dma/xilinx_axidma.c | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/dma/trace-events b/hw/dma/trace-events
> index 3c47df54e4..95db083be4 100644
> --- a/hw/dma/trace-events
> +++ b/hw/dma/trace-events
> @@ -44,3 +44,6 @@ pl330_debug_exec_stall(void) "stall of debug instruction
> not implemented"
>  pl330_iomem_write(uint32_t offset, uint32_t value) "addr: 0x%08"PRIx32"
> data: 0x%08"PRIx32
>  pl330_iomem_write_clr(int i) "event interrupt lowered %d"
>  pl330_iomem_read(uint32_t addr, uint32_t data) "addr: 0x%08"PRIx32" data:
> 0x%08"PRIx32
> +
> +# xilinx_axidma.c
> +xilinx_axidma_loading_desc_fail(uint32_t res) "error:%d"
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index b8ab5a423d..1bbb9d6c4c 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -36,6 +36,7 @@
>  #include "sysemu/dma.h"
>  #include "hw/stream.h"
>  #include "qom/object.h"
> +#include "trace.h"
>
>  #define D(x)
>
> @@ -200,6 +201,8 @@ static MemTxResult stream_desc_load(struct Stream *s,
> hwaddr addr)
>                                              addr, MEMTXATTRS_UNSPECIFIED,
>                                              d, sizeof *d);
>      if (result != MEMTX_OK) {
> +        trace_xilinx_axidma_loading_desc_fail(result);
> +
>          s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
>          s->regs[R_DMASR] |= DMASR_HALTED;
>          s->regs[R_DMASR] |= DMASR_SLVERR;
> --
> 2.34.1
>
>

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

Re: [PATCH 4/4] hw/net: Fix the transmission return size

[Edgar E. Iglesias]

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

On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang <fea.wang@sifive.com> wrote:

> Fix the transmission return size because not all bytes could be
> transmitted successfully. So, return a successful length instead of a
> constant value.
>
>
How did you test this patch, on Linux or something else? I have some
memory that we had some trouble with similar patches before.

Anyway, the change looks good to me:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>



> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> ---
>  hw/net/xilinx_axienet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index 7d1fd37b4a..05d41bd548 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -847,7 +847,7 @@ static ssize_t eth_rx(NetClientState *nc, const
> uint8_t *buf, size_t size)
>      axienet_eth_rx_notify(s);
>
>      enet_update_irq(s);
> -    return size;
> +    return s->rxpos;
>  }
>
>  static size_t
> --
> 2.34.1
>
>

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

Re: [PATCH 1/4] hw/dma: Enhance error handling in loading description

[Fea Wang]

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

Hi Edgar,
Thank you for recommending to me. I will make the change in the next
version of the patch series.

Sincerely,
Fea

On Mon, Jun 3, 2024 at 6:19 PM Edgar E. Iglesias <edgar.iglesias@gmail.com>
wrote:

> On Mon, Jun 3, 2024 at 7:47 AM Fea.Wang <fea.wang@sifive.com> wrote:
>
>> Loading a description from memory may cause a bus-error. In this
>> case, the DMA should stop working, set the error flag, and return
>> the error value.
>>
>> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
>>
>
>
> Hi Fea,
>
> I've got a couple of small comments:
>
>
> ---
>>  hw/dma/xilinx_axidma.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> index 0ae056ed06..4b475e5484 100644
>> --- a/hw/dma/xilinx_axidma.c
>> +++ b/hw/dma/xilinx_axidma.c
>> @@ -71,8 +71,10 @@ enum {
>>  enum {
>>      DMASR_HALTED = 1,
>>      DMASR_IDLE  = 2,
>> +    DMASR_SLVERR = 1 << 5,
>>
>
> We should also add DMASR_DECERR = 1 << 6
>
>
>>      DMASR_IOC_IRQ  = 1 << 12,
>>      DMASR_DLY_IRQ  = 1 << 13,
>> +    DMASR_ERR_IRQ  = 1 << 14,
>>
>>      DMASR_IRQ_MASK = 7 << 12
>>  };
>> @@ -190,17 +192,27 @@ static inline int streamid_from_addr(hwaddr addr)
>>      return sid;
>>  }
>>
>> -static void stream_desc_load(struct Stream *s, hwaddr addr)
>> +static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
>>  {
>>      struct SDesc *d = &s->desc;
>>
>> -    address_space_read(&s->dma->as, addr, MEMTXATTRS_UNSPECIFIED, d,
>> sizeof *d);
>> +    MemTxResult result = address_space_read(&s->dma->as,
>> +                                            addr, MEMTXATTRS_UNSPECIFIED,
>> +                                            d, sizeof *d);
>> +    if (result != MEMTX_OK) {
>> +        s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
>> +        s->regs[R_DMASR] |= DMASR_HALTED;
>> +        s->regs[R_DMASR] |= DMASR_SLVERR;
>>
>
> ... and map MEMTX_DECODE_ERROR to DMASR_DECERR and everything else to
> SLVERR, for example:
> if (result == MEMTX_DECODE_ERROR) {
>     s->regs[R_DMASR] |= DMASR_DECERR;
> } else {
>     s->regs[R_DMASR] |= DMASR_SLVERR;
> }
>
>
>> +        s->regs[R_DMASR] |= DMASR_ERR_IRQ;
>> +        return result;
>> +    }
>>
>>      /* Convert from LE into host endianness.  */
>>      d->buffer_address = le64_to_cpu(d->buffer_address);
>>      d->nxtdesc = le64_to_cpu(d->nxtdesc);
>>      d->control = le32_to_cpu(d->control);
>>      d->status = le32_to_cpu(d->status);
>> +    return result;
>>  }
>>
>>  static void stream_desc_store(struct Stream *s, hwaddr addr)
>> --
>> 2.34.1
>>
>>

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

Re: [PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

[Fea Wang]

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

Hi Edgar,
thank you for the advice, I will squash them in the next version of the
patch series.

Sincerely,
Fea

On Mon, Jun 3, 2024 at 6:21 PM Edgar E. Iglesias <edgar.iglesias@gmail.com>
wrote:

> On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang <fea.wang@sifive.com> wrote:
>
>> When calling the loading a description function, it should be noticed
>> that the function may return a failure value. Breaking the loop is one
>> of the possible ways to handle it.
>>
>> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
>>
>
>
> Looks good, a nitpick comment, I would either squash this with patch #1
> or move the change to return of error code in stream_desc_load() to this
> patch.
>
>
>
>
>> ---
>>  hw/dma/xilinx_axidma.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> index 4b475e5484..b8ab5a423d 100644
>> --- a/hw/dma/xilinx_axidma.c
>> +++ b/hw/dma/xilinx_axidma.c
>> @@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s,
>> StreamSink *tx_data_dev,
>>      }
>>
>>      while (1) {
>> -        stream_desc_load(s, s->regs[R_CURDESC]);
>> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
>> +            break;
>> +        }
>>
>>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>>              s->regs[R_DMASR] |= DMASR_HALTED;
>> @@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s,
>> unsigned char *buf,
>>      }
>>
>>      while (len) {
>> -        stream_desc_load(s, s->regs[R_CURDESC]);
>> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
>> +            break;
>> +        }
>>
>>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>>              s->regs[R_DMASR] |= DMASR_HALTED;
>> --
>> 2.34.1
>>
>>

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

Re: [PATCH 4/4] hw/net: Fix the transmission return size

[Fea Wang]

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

I just encountered this issue when running Linux, and the trouble will be
fixed after the patches. So I think they work.

Sincerely,
Fea

On Mon, Jun 3, 2024 at 6:31 PM Edgar E. Iglesias <edgar.iglesias@gmail.com>
wrote:

> On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang <fea.wang@sifive.com> wrote:
>
>> Fix the transmission return size because not all bytes could be
>> transmitted successfully. So, return a successful length instead of a
>> constant value.
>>
>>
> How did you test this patch, on Linux or something else? I have some
> memory that we had some trouble with similar patches before.
>
> Anyway, the change looks good to me:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>
>
>
>> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
>> ---
>>  hw/net/xilinx_axienet.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
>> index 7d1fd37b4a..05d41bd548 100644
>> --- a/hw/net/xilinx_axienet.c
>> +++ b/hw/net/xilinx_axienet.c
>> @@ -847,7 +847,7 @@ static ssize_t eth_rx(NetClientState *nc, const
>> uint8_t *buf, size_t size)
>>      axienet_eth_rx_notify(s);
>>
>>      enet_update_irq(s);
>> -    return size;
>> +    return s->rxpos;
>>  }
>>
>>  static size_t
>> --
>> 2.34.1
>>
>>

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

Re: [PATCH 1/4] hw/dma: Enhance error handling in loading description

[Frank Chang]

Reviewed-by: Frank Chang <frank.chang@sifive.com>

Fea.Wang <fea.wang@sifive.com> 於 2024年6月3日 週一 下午1:48寫道：
>
> Loading a description from memory may cause a bus-error. In this
> case, the DMA should stop working, set the error flag, and return
> the error value.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> ---
>  hw/dma/xilinx_axidma.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 0ae056ed06..4b475e5484 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -71,8 +71,10 @@ enum {
>  enum {
>      DMASR_HALTED = 1,
>      DMASR_IDLE  = 2,
> +    DMASR_SLVERR = 1 << 5,
>      DMASR_IOC_IRQ  = 1 << 12,
>      DMASR_DLY_IRQ  = 1 << 13,
> +    DMASR_ERR_IRQ  = 1 << 14,
>
>      DMASR_IRQ_MASK = 7 << 12
>  };
> @@ -190,17 +192,27 @@ static inline int streamid_from_addr(hwaddr addr)
>      return sid;
>  }
>
> -static void stream_desc_load(struct Stream *s, hwaddr addr)
> +static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
>  {
>      struct SDesc *d = &s->desc;
>
> -    address_space_read(&s->dma->as, addr, MEMTXATTRS_UNSPECIFIED, d, sizeof *d);
> +    MemTxResult result = address_space_read(&s->dma->as,
> +                                            addr, MEMTXATTRS_UNSPECIFIED,
> +                                            d, sizeof *d);
> +    if (result != MEMTX_OK) {
> +        s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
> +        s->regs[R_DMASR] |= DMASR_HALTED;
> +        s->regs[R_DMASR] |= DMASR_SLVERR;
> +        s->regs[R_DMASR] |= DMASR_ERR_IRQ;
> +        return result;
> +    }
>
>      /* Convert from LE into host endianness.  */
>      d->buffer_address = le64_to_cpu(d->buffer_address);
>      d->nxtdesc = le64_to_cpu(d->nxtdesc);
>      d->control = le32_to_cpu(d->control);
>      d->status = le32_to_cpu(d->status);
> +    return result;
>  }
>
>  static void stream_desc_store(struct Stream *s, hwaddr addr)
> --
> 2.34.1
>
>

Re: [PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

[Frank Chang]

Reviewed-by: Frank Chang <frank.chang@sifive.com>

Fea.Wang <fea.wang@sifive.com> 於 2024年6月3日 週一 下午1:48寫道：
>
> When calling the loading a description function, it should be noticed
> that the function may return a failure value. Breaking the loop is one
> of the possible ways to handle it.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> ---
>  hw/dma/xilinx_axidma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 4b475e5484..b8ab5a423d 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
>      }
>
>      while (1) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> @@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
>      }
>
>      while (len) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> --
> 2.34.1
>
>

Re: [PATCH 3/4] hw/dma: Add a trace log for a description loading failure

[Frank Chang]

Reviewed-by: Frank Chang <frank.chang@sifive.com>

Fea.Wang <fea.wang@sifive.com> 於 2024年6月3日 週一 下午1:49寫道：
>
> Due to a description loading failure, adding a trace log makes observing
> the DMA behavior easy.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> ---
>  hw/dma/trace-events    | 3 +++
>  hw/dma/xilinx_axidma.c | 3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/dma/trace-events b/hw/dma/trace-events
> index 3c47df54e4..95db083be4 100644
> --- a/hw/dma/trace-events
> +++ b/hw/dma/trace-events
> @@ -44,3 +44,6 @@ pl330_debug_exec_stall(void) "stall of debug instruction not implemented"
>  pl330_iomem_write(uint32_t offset, uint32_t value) "addr: 0x%08"PRIx32" data: 0x%08"PRIx32
>  pl330_iomem_write_clr(int i) "event interrupt lowered %d"
>  pl330_iomem_read(uint32_t addr, uint32_t data) "addr: 0x%08"PRIx32" data: 0x%08"PRIx32
> +
> +# xilinx_axidma.c
> +xilinx_axidma_loading_desc_fail(uint32_t res) "error:%d"
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index b8ab5a423d..1bbb9d6c4c 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -36,6 +36,7 @@
>  #include "sysemu/dma.h"
>  #include "hw/stream.h"
>  #include "qom/object.h"
> +#include "trace.h"
>
>  #define D(x)
>
> @@ -200,6 +201,8 @@ static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
>                                              addr, MEMTXATTRS_UNSPECIFIED,
>                                              d, sizeof *d);
>      if (result != MEMTX_OK) {
> +        trace_xilinx_axidma_loading_desc_fail(result);
> +
>          s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
>          s->regs[R_DMASR] |= DMASR_HALTED;
>          s->regs[R_DMASR] |= DMASR_SLVERR;
> --
> 2.34.1
>
>

Re: [PATCH 4/4] hw/net: Fix the transmission return size

[Frank Chang]

Reviewed-by: Frank Chang <frank.chang@sifive.com>

Fea.Wang <fea.wang@sifive.com> 於 2024年6月3日 週一 下午1:48寫道：
>
> Fix the transmission return size because not all bytes could be
> transmitted successfully. So, return a successful length instead of a
> constant value.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> ---
>  hw/net/xilinx_axienet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
> index 7d1fd37b4a..05d41bd548 100644
> --- a/hw/net/xilinx_axienet.c
> +++ b/hw/net/xilinx_axienet.c
> @@ -847,7 +847,7 @@ static ssize_t eth_rx(NetClientState *nc, const uint8_t *buf, size_t size)
>      axienet_eth_rx_notify(s);
>
>      enet_update_irq(s);
> -    return size;
> +    return s->rxpos;
>  }
>
>  static size_t
> --
> 2.34.1
>
>