Re: [PULL 1/9] softmmu: Support concurrent bounce buffers

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 16/09/2024 12:44, Peter Maydell wrote:

> On Mon, 16 Sept 2024 at 12:29, Mark Cave-Ayland
> <mark.cave-ayland@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.