| Message ID | 20240916175708.1829059-1-mnissler@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mac_dbdma: Remove leftover `dma_memory_unmap` calls | expand |
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 <mnissler@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-ayland@ilande.co.uk> Tested-by: Mark Cave-Ayland <mark.cave-ayland@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.
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-ayland@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 <mnissler@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-ayland@ilande.co.uk> > Tested-by: Mark Cave-Ayland <mark.cave-ayland@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. >
On Tue, Sep 17, 2024 at 08:25:06AM +0200, Mattias Nissler wrote: > 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? The pull has already landed master branch, so there'll be no respin. And considering this is a fix from the device side, may make more sense that the maintainer takes it, which points to John Snow here. John, would you take it via your tree? Thanks,
On 17/09/2024 17:31, Peter Xu wrote: > On Tue, Sep 17, 2024 at 08:25:06AM +0200, Mattias Nissler wrote: >> 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? > > The pull has already landed master branch, so there'll be no respin. And > considering this is a fix from the device side, may make more sense that > the maintainer takes it, which points to John Snow here. > > John, would you take it via your tree? Looks like people are busy, so I've queued this to my qemu-macppc branch and will send a PR if it passes CI. ATB, Mark.
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; }; /*
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 <mnissler@rivosinc.com> --- hw/ide/macio.c | 6 ------ include/hw/ppc/mac_dbdma.h | 4 ---- 2 files changed, 10 deletions(-)