Mark, thanks for testing and confirming that this doesn't cause any
obvious breakage.
For my curiosity, which path should this patch take to get into
master? Peter, are you going to respin your pull request with this
included?
On Mon, Sep 16, 2024 at 11:06 PM Mark Cave-Ayland
<mark.cave-ayl...@ilande.co.uk> wrote:
>
> On 16/09/2024 18:57, Mattias Nissler wrote:
>
> > These were passing a NULL buffer pointer unconditionally, which happens
> > to behave in a mostly benign way (except for the chance of an excess
> > memory region unref and a bounce buffer leak). Per the function comment,
> > this was never meant to be accepted though, and triggers an assertion
> > with the "softmmu: Support concurrent bounce buffers" change.
> >
> > Given that the code in question never sets up any mappings, just remove
> > the unnecessary dma_memory_unmap calls along with the DBDMA_io struct
> > fields that are now entirely unused.
> >
> > Signed-off-by: Mattias Nissler <mniss...@rivosinc.com>
> > ---
> > hw/ide/macio.c | 6 ------
> > include/hw/ppc/mac_dbdma.h | 4 ----
> > 2 files changed, 10 deletions(-)
> >
> > diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> > index bec2e866d7..99477a3d13 100644
> > --- a/hw/ide/macio.c
> > +++ b/hw/ide/macio.c
> > @@ -119,9 +119,6 @@ static void pmac_ide_atapi_transfer_cb(void *opaque,
> > int ret)
> > return;
> >
> > done:
> > - dma_memory_unmap(&address_space_memory, io->dma_mem, io->dma_len,
> > - io->dir, io->dma_len);
> > -
> > if (ret < 0) {
> > block_acct_failed(blk_get_stats(s->blk), &s->acct);
> > } else {
> > @@ -202,9 +199,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
> > return;
> >
> > done:
> > - dma_memory_unmap(&address_space_memory, io->dma_mem, io->dma_len,
> > - io->dir, io->dma_len);
> > -
> > if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
> > if (ret < 0) {
> > block_acct_failed(blk_get_stats(s->blk), &s->acct);
> > diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
> > index 4a3f644516..c774f6bf84 100644
> > --- a/include/hw/ppc/mac_dbdma.h
> > +++ b/include/hw/ppc/mac_dbdma.h
> > @@ -44,10 +44,6 @@ struct DBDMA_io {
> > DBDMA_end dma_end;
> > /* DMA is in progress, don't start another one */
> > bool processing;
> > - /* DMA request */
> > - void *dma_mem;
> > - dma_addr_t dma_len;
> > - DMADirection dir;
> > };
> >
> > /*
>
> Thanks for looking at this, Matthias. I've given it a quick spin around
> various PPC
> Mac images and it looks good to me, so:
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> Tested-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
>
> My guess is that the current use of dma_memory_unmap() was a
> misunderstanding/bug
> when porting the macio IDE device over to use the byte-aligned block DMA
> helpers, so
> I think you can also add:
>
> Fixes: be1e343995 ("macio: switch over to new byte-aligned DMA helpers")
>
>
> ATB,
>
> Mark.
>
