| Message ID | 20211215205656.488940-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/sd/sdhci: Fix DMA re-entrancy issue | expand |
On 15/12/2021 21.56, Philippe Mathieu-Daudé wrote: > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > DMA transactions might fail. The DMA API returns a MemTxResult, > indicating such failures. Do not ignore it. On failure, raise > the ADMA error flag and eventually triggering an IRQ (see spec > chapter 1.13.5: "ADMA2 States"). > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/sd/sdhci.c | 34 +++++++++++++++++++++++++--------- > 1 file changed, 25 insertions(+), 9 deletions(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index e0bbc903446..fe2f21f0c37 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -742,6 +742,7 @@ static void sdhci_do_adma(SDHCIState *s) > unsigned int begin, length; > const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK; > ADMADescr dscr = {}; > + MemTxResult res; > int i; > > if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) { > @@ -790,10 +791,13 @@ static void sdhci_do_adma(SDHCIState *s) > s->data_count = block_size; > length -= block_size - begin; > } > - dma_memory_write(s->dma_as, dscr.addr, > - &s->fifo_buffer[begin], > - s->data_count - begin, > - MEMTXATTRS_UNSPECIFIED); > + res = dma_memory_write(s->dma_as, dscr.addr, > + &s->fifo_buffer[begin], > + s->data_count - begin, > + MEMTXATTRS_UNSPECIFIED); > + if (res != MEMTX_OK) { > + break; > + } > dscr.addr += s->data_count - begin; > if (s->data_count == block_size) { > s->data_count = 0; > @@ -816,10 +820,13 @@ static void sdhci_do_adma(SDHCIState *s) > s->data_count = block_size; > length -= block_size - begin; > } > - dma_memory_read(s->dma_as, dscr.addr, > - &s->fifo_buffer[begin], > - s->data_count - begin, > - MEMTXATTRS_UNSPECIFIED); > + res = dma_memory_read(s->dma_as, dscr.addr, > + &s->fifo_buffer[begin], > + s->data_count - begin, > + MEMTXATTRS_UNSPECIFIED); > + if (res != MEMTX_OK) { > + break; > + } > dscr.addr += s->data_count - begin; > if (s->data_count == block_size) { > sdbus_write_data(&s->sdbus, s->fifo_buffer, block_size); > @@ -833,7 +840,16 @@ static void sdhci_do_adma(SDHCIState *s) > } > } > } > - s->admasysaddr += dscr.incr; > + if (res != MEMTX_OK) { > + if (s->errintstsen & SDHC_EISEN_ADMAERR) { > + trace_sdhci_error("Set ADMA error flag"); > + s->errintsts |= SDHC_EIS_ADMAERR; > + s->norintsts |= SDHC_NIS_ERR; > + } > + sdhci_update_irq(s); > + } else { > + s->admasysaddr += dscr.incr; > + } > break; > case SDHC_ADMA_ATTR_ACT_LINK: /* link to next descriptor table */ > s->admasysaddr = dscr.addr; Patch looks sane to me: Reviewed-by: Thomas Huth <thuth@redhat.com> Are you still considering it or did you drop this from your TODO list? (since it was just marked as RFC?) Thomas
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index e0bbc903446..fe2f21f0c37 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -742,6 +742,7 @@ static void sdhci_do_adma(SDHCIState *s) unsigned int begin, length; const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK; ADMADescr dscr = {}; + MemTxResult res; int i; if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) { @@ -790,10 +791,13 @@ static void sdhci_do_adma(SDHCIState *s) s->data_count = block_size; length -= block_size - begin; } - dma_memory_write(s->dma_as, dscr.addr, - &s->fifo_buffer[begin], - s->data_count - begin, - MEMTXATTRS_UNSPECIFIED); + res = dma_memory_write(s->dma_as, dscr.addr, + &s->fifo_buffer[begin], + s->data_count - begin, + MEMTXATTRS_UNSPECIFIED); + if (res != MEMTX_OK) { + break; + } dscr.addr += s->data_count - begin; if (s->data_count == block_size) { s->data_count = 0; @@ -816,10 +820,13 @@ static void sdhci_do_adma(SDHCIState *s) s->data_count = block_size; length -= block_size - begin; } - dma_memory_read(s->dma_as, dscr.addr, - &s->fifo_buffer[begin], - s->data_count - begin, - MEMTXATTRS_UNSPECIFIED); + res = dma_memory_read(s->dma_as, dscr.addr, + &s->fifo_buffer[begin], + s->data_count - begin, + MEMTXATTRS_UNSPECIFIED); + if (res != MEMTX_OK) { + break; + } dscr.addr += s->data_count - begin; if (s->data_count == block_size) { sdbus_write_data(&s->sdbus, s->fifo_buffer, block_size); @@ -833,7 +840,16 @@ static void sdhci_do_adma(SDHCIState *s) } } } - s->admasysaddr += dscr.incr; + if (res != MEMTX_OK) { + if (s->errintstsen & SDHC_EISEN_ADMAERR) { + trace_sdhci_error("Set ADMA error flag"); + s->errintsts |= SDHC_EIS_ADMAERR; + s->norintsts |= SDHC_NIS_ERR; + } + sdhci_update_irq(s); + } else { + s->admasysaddr += dscr.incr; + } break; case SDHC_ADMA_ATTR_ACT_LINK: /* link to next descriptor table */ s->admasysaddr = dscr.addr;