[v2] hw/dma: Add error handling for loading descriptions failing

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

Posted by Fea.Wang 1 year, 8 months ago

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 failure value.

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

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 hw/dma/xilinx_axidma.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 0ae056ed06..ad307994c2 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -71,8 +71,11 @@ enum {
 enum {
     DMASR_HALTED = 1,
     DMASR_IDLE  = 2,
+    DMASR_SLVERR = 1 << 5,
+    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 +193,32 @@ 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) {
+        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;
+        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)
@@ -279,7 +297,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;
@@ -336,7 +356,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 v2 1/3] hw/dma: Enhance error handling in loading description

Posted by Edgar E. Iglesias 1 year, 8 months ago

On Tue, Jun 4, 2024 at 9:10 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 failure value.
>
> When calling the loading a description function, it should be noticed
> that the function may return a failure value. Breaking the loop in this
> case is one of the possible ways to handle it.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
>

Thanks!
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>



---
>  hw/dma/xilinx_axidma.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 0ae056ed06..ad307994c2 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -71,8 +71,11 @@ enum {
>  enum {
>      DMASR_HALTED = 1,
>      DMASR_IDLE  = 2,
> +    DMASR_SLVERR = 1 << 5,
> +    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 +193,32 @@ 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) {
> +        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;
> +        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)
> @@ -279,7 +297,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;
> @@ -336,7 +356,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 v2 1/3] hw/dma: Enhance error handling in loading description
[PATCH v2 2/3] hw/dma: Add a trace log for a description loading failure
[PATCH v2 3/3] hw/net: Fix the transmission return size