| Message ID | 20240603055248.3928469-2-fea.wang@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/dma: Add error handling for loading descriptions failing | expand |
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 > >
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 >> >>
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 > >
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)
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(-)