Re: [RFC PATCH 1/3] hw/sd/sdhci: Honor failed DMA transactions

[Thomas Huth <thuth@redhat.com>]

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