Re: [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5901 bytes --]

On Thu, Apr 16, 2026 at 10:27:22AM +0200, Philippe Mathieu-Daudé wrote:
> Cc'ing Paolo &  Stefan.
> 
> On 16/4/26 09:52, Christian Speich wrote:
> > On Tue, Apr 14, 2026 at 03:51:24PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 4/2/26 14:22, Christian Speich wrote:
> > > > Currently, ADMA will temporarily store data into a local bounce buffer
> > > > when transferring it. This will produce unneeded copies of the data and
> > > > limit us to the bounce buffer size for each step.
> > > > 
> > > > This patch now maps the requested DMA address and passes this buffer
> > > > directly to sdbus_{read,write}_data. This allows to pass much larger
> > > > buffers down to increase the performance. sdbus_{read,write}_data is
> > > > already able to handle arbitrary length and alignments, so we do not
> > > > need to ensure this.
> > > > 
> > > > Signed-off-by: Christian Speich <c.speich@avm.de>
> > > > ---
> > > >    hw/sd/sdhci.c | 102 +++++++++++++++++++++++++++++++---------------------------
> > > >    1 file changed, 55 insertions(+), 47 deletions(-)
> > > > 
> > > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > > > index c86dfa281f4b0218bf6dda7a38d46abfc9638450..6e07711478cb6ca046a7d371a82e2c682ebbda00 100644
> > > > --- a/hw/sd/sdhci.c
> > > > +++ b/hw/sd/sdhci.c
> > > > @@ -775,7 +775,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
> > > >    static void sdhci_do_adma(SDHCIState *s)
> > > >    {
> > > > -    unsigned int begin, length;
> > > > +    unsigned int length;
> > > >        const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
> > > >        const MemTxAttrs attrs = { .memory = true };
> > > >        ADMADescr dscr = {};
> > > > @@ -817,66 +817,74 @@ static void sdhci_do_adma(SDHCIState *s)
> > > >                if (s->trnmod & SDHC_TRNS_READ) {
> > > >                    s->prnsts |= SDHC_DOING_READ;
> > > >                    while (length) {
> > > > -                    if (s->data_count == 0) {
> > > > -                        sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
> > > > -                    }
> > > > -                    begin = s->data_count;
> > > > -                    if ((length + begin) < block_size) {
> > > > -                        s->data_count = length + begin;
> > > > -                        length = 0;
> > > > -                     } else {
> > > > -                        s->data_count = block_size;
> > > > -                        length -= block_size - begin;
> > > > -                    }
> > > > -                    res = dma_memory_write(s->dma_as, dscr.addr,
> > > > -                                           &s->fifo_buffer[begin],
> > > > -                                           s->data_count - begin,
> > > > -                                           attrs);
> > > > -                    if (res != MEMTX_OK) {
> > > > +                    dma_addr_t dma_len = length;
> > > > +
> > > > +                    void *buf = dma_memory_map(s->dma_as, dscr.addr, &dma_len,
> > > > +                                               DMA_DIRECTION_FROM_DEVICE,
> > > > +                                               attrs);
> > > 
> > > We know the descriptor is a consecutive range. Couldn't we map/unmap
> > > outside of the while() loop? (Ditto write path).
> > 
> > The loop is there to handle dma_memory_map mapping less that we've requested. I'm not
> > really sure if there are any guranteed cases where it always maps the requested length
> > fully (at least there are non in the dma_memory_map doc string).
> > 
> > Iff this is guranteed to always map the requested length, we could remove the whie()
> > loop completly.

I think this loop is required because:

- A consecutive DMA address range can span multiple host address ranges.
  This can happen when there is more than 1 RAM block backing the DMA
  address space.

- When the DMA address range includes non-RAM addresses (e.g. MMIO
  regions), then it is mapped via a bounce buffer that has a size limit
  and could require multiple iterations.

> > 
> > Kind Regards,
> > Christian
> > 
> > > 
> > > > +
> > > > +                    if (buf == NULL) {
> > > > +                        res = MEMTX_ERROR;
> > > >                            break;
> > > > +                    } else {
> > > > +                        res = MEMTX_OK;
> > > >                        }
> > > > -                    dscr.addr += s->data_count - begin;
> > > > -                    if (s->data_count == block_size) {
> > > > -                        s->data_count = 0;
> > > > -                        if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
> > > > -                            s->blkcnt--;
> > > > -                            if (s->blkcnt == 0) {
> > > > -                                break;
> > > > -                            }
> > > > +
> > > > +                    sdbus_read_data(&s->sdbus, buf, dma_len);
> > > > +                    length -= dma_len;
> > > > +                    dscr.addr += dma_len;
> > > > +
> > > > +                    dma_memory_unmap(s->dma_as, buf, dma_len,
> > > > +                                     DMA_DIRECTION_FROM_DEVICE, dma_len);
> > > > +
> > > > +                    if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
> > > > +                        size_t transfered = s->data_count + dma_len;
> > > > +
> > > > +                        s->blkcnt -= transfered / block_size;
> > > > +                        s->data_count = transfered % block_size;
> > > > +
> > > > +                        if (s->blkcnt == 0) {
> > > > +                            s->data_count = 0;
> > > > +                            break;
> > > >                            }
> > > >                        }
> > > >                    }
> > > >                } else {
> > > 
> > > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]