mac_dbdma: Remove leftover `dma_memory_unmap` calls

[PATCH] mac_dbdma: Remove leftover `dma_memory_unmap` calls

Posted by Mattias Nissler 2 months, 1 week ago

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;
 };
 
 /*
-- 
2.34.1

Re: [PATCH] mac_dbdma: Remove leftover `dma_memory_unmap` calls

Posted by Mark Cave-Ayland 2 months, 1 week ago

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.

Re: [PATCH] mac_dbdma: Remove leftover `dma_memory_unmap` calls

Posted by Mattias Nissler 2 months, 1 week ago

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.
>

Re: [PATCH] mac_dbdma: Remove leftover `dma_memory_unmap` calls

Posted by Peter Xu 2 months, 1 week ago

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,

-- 
Peter Xu

Re: [PATCH] mac_dbdma: Remove leftover `dma_memory_unmap` calls

Posted by Mark Cave-Ayland 2 months ago

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.