| Message ID | 20240801140609.26922-3-jim.shu@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | Several fixes of AXI-ethernet/DMA | expand |
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
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
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. */
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(-)