On Wed, 27 Jul 2022 at 14:01, Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Wed, 27 Jul 2022 at 12:55, BALATON Zoltan <bala...@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
