On Mon, 16 Sept 2024 at 12:29, Mark Cave-Ayland
<mark.cave-ayl...@ilande.co.uk> wrote:
>
> On 16/09/2024 09:23, Mattias Nissler wrote:
> > Looking at the code, the dma_memory_unmap calls in hw/ide/macio.c seem
> > to be passing buffer=NULL unconditionally, since the dma_mem field in
> > struct DBDMA_io is never set to anything non-zero. In fact, I believe
> > after commit be1e343995ef81fc05d9a4e1ec263ca171d842e7 "macio: switch
> > over to new byte-aligned DMA helpers", the dma_memory_unmap calls in
> > hw/ide/macio.c aren't doing anything and should probably have been
> > removed together with the dma_mem, dma_len and dir fields in struct
> > DBDMA_io. Speculative patch:
> >
> > diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> > index e84bf2c9f6..15dd40138e 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;
> > };
> >
> > /*
> >
> > Cédric, can you try with the above patch and/or share more details of
> > your setup so I can verify (I tried booting a ppc64el-pseries dqib
> > image but didn't see the issue)?
>
> I'm fairly sure that this patch would break MacOS 9 which was the reason that
> dma_memory_unmap() was added here in the first place: what I was finding was
> that
> without the dma_memory_unmap() the destination RAM wasn't being invalidated
> (or
> marked dirty), causing random crashes during boot.
dma_memory_unmap() of something you never mapped is
definitely wrong. Whatever is going on here, leaving the unmap
call in after you removed the dma_memory_map() call is just
papering over the actual cause of the crashes.
> Would the issue be solved by adding a corresponding dma_memory_map()
> beforehand at
> the relevant places in hw/ide/macio.c? If that's required as part of the
> setup for
> bounce buffers then I can see how not having this present could cause
> problems.
The only purpose of this API is sequences of:
host_ptr = dma_memory_map(...);
access the host_ptr directly;
dma_memory_unmap(...);
The bounce-buffer stuff is an internal implementation detail
of making this API work when the DMA is going to a device.
We need to find whatever the actual cause of the macos failure is.
Mattias' suggested change looks right to me.
I do wonder if something needs the memory barrier than
unmap does as part of its operation, e.g. in the
implementation of the dma_blk_* functions.
-- PMM
