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

[Peter Maydell]

On Thu, 1 Aug 2024 at 15:08, Jim Shu <jim.shu@sifive.com> wrote:
>
> 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;
> +    }

The MM2S_DMASR described in this doc:
https://docs.amd.com/r/en-US/pg021_axi_dma/MM2S_DMASR-MM2S-DMA-Status-Register-Offset-04h
has both DMASlvErr/DMADecErr bits and also
SGSlvErr/SGDecErr bits. Is that the wrong documentation
for the version of the device we model, or is there
a situation where we should be setting the other
SlvErr/DecErr bits when we get an error from our
memory accesses? Similarly there's a separate
S2MM_DMASR in those docs which we don't model at all
(so maybe it is the wrong datasheet...)

> +
> +    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;
> +            }

In this function we only use result in the immediately following
if(), so I think it would be slightly clearer to write them as
      if (stream_desc_store(...) != MEMTX_OK) {
          break;
      }

without the use of the variable.

> +
>              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;
> +        }

In these two places also we don't need the variable.

>          s->sof = eop;
>
>          /* Advance.  */
> --

thanks
-- PMM