On 16/09/2024 12:44, Peter Maydell wrote:
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.
It has been a few years now, but I'm fairly sure the issue was that dma_blk_read()
didn't mark RAM containing code as dirty/invalid, and since MacOS 9 used overlays
then it would crash randomly trying to execute stale memory. dma_memory_unmap()
checks to see if the direction was to RAM, and then marks the memory dirty allowing
the new code to get picked up after a MMU fault.
If the memory barriers are already in place for the dma_blk_*() functions then the
analysis could be correct, in which case the bug is a misunderstanding I made in
be1e343995 ("macio: switch over to new byte-aligned DMA helpers") back in 2016.
ATB,
Mark.
