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.1On 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
On Thu, Aug 8, 2024 at 9:34 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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...)
I also use this spec (PG021, AXI DMA v7.1). I think it is the right one.
1. For DMASlvErr/DMADecErr & SGSlvErr/SGDecErr
Spec has Scatter/Gather mode & Direct Register mode. It looks like a
HW config instead of SW config.
The SG mode uses MM2S_CURDESC (0x08) / TAIL_DESC (0x10) and direct
mode uses MM2S_SA (0x18) / MM2S_LENGTH (0x28) to transfer.
I think the QEMU model only implements SG mode now (S2MM also).
stream_dma_error() should set SGSlvErr/SGDecErr instead of DMA ones.
I will fix it in v3.
2. For MM2S & S2MM
QEMU has implemented 2 Streams. Stream[0] is MM2S and Stream[1] is S2MM.
The register read/write function will redirect to Stream[0] if addr <
0x30 and Stream[1] if 0x30 <= addr < 0x60.
stream_dma_error() is based on Stream* so it could support both MM2S & S2MM.
>
> > +
> > + 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.
Thanks for the suggestion. I will fix it in v3.
>
> > +
> > 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.
Thanks for the suggestion. I will fix it in v3.
>
> > s->sof = eop;
> >
> > /* Advance. */
> > --
>
> thanks
> -- PMM
© 2016 - 2026 Red Hat, Inc.