Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA

[Peter Maydell]

On Wed, 27 Jul 2022 at 14:01, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 27 Jul 2022 at 12:55, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >
> > On Wed, 27 Jul 2022, Cédric Le Goater wrote:
> > > On 7/26/22 20:23, Peter Maydell wrote:
> > >> This patchset is mainly trying to fix a problem that Coverity spotted
> > >> in the dcr_write_dma() function in hw/ppc/ppc440_uc.c, where the code
> > >> is not correctly using the cpu_physical_memory_map() function.
> >
> > Likely I did not know how this function works when implementing it and may
> > have used it wrong but none of the reviews spotted it either. (I may have
> > used some other DMA device model as an inspiration but don't remember
> > which.)
> >
> > >> While I was fixing that I noticed a second problem in this code,
> > >> where it doesn't have a fallback for when cpu_physical_memory_map()
> > >> says "I couldn't map that for you".
> >
> > When can that happen? If only in cases when guest gives invalid parameters
> > then maybe we don't have to bother with that and can let it fail but
> > having a fallback does not hurt.
>
> Mostly it happens when the thing being DMA'd to or from is not RAM.
> Ordinarily I wouldn't expect that to be likely, but the DMA device
> here has a "don't advance the src/destination" option which I assume
> would be used for things like DMA'ing to or from a device FIFO.
> Perhaps AmigaOS doesn't in practice do that.

Oh, and we should probably use the 'call address_space_read/write'
fallback for all cases of "don't advance the pointer", because
address_space_map() will not handle that correctly -- it will
typically return a 'bounce buffer' and then copy the bounce buffer
to the device, so the effect will be like reading/writing the
FIFO device only once instead of reading/writing all the data.

thanks
-- PMM