|
| From: | BALATON Zoltan |
| Subject: | Re: [RFC 0/2] Fix Coverity and other errors in ppc440_uc DMA |
| Date: | Wed, 27 Jul 2022 13:55:26 +0200 (CEST) |
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.
I've marked these patches as RFC, partly because I don't have anyguest that would exercise the code changes[*],I build these : https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot/qemu_ppc_sam460ex-2022.02-4-geae5011c83-20220309 but none of the DCR DMA registers are used.
To my knowledge only AmigaOS needs it but that does use it and it boots and runs so this is not completely broken but maybe mapping random length memory areas could cause some problems if the length mapped is less than needed. I've seen random crashes with AmigaOS before that I could not explain but that seems to be related to processor speed (happens more on slower host) so it seems more like a race condition not wrong DMA and haven't seen it recently.
There are images for the sam460ex images here : http://www.aros.org/nightly1.php But AFAICT, it does not go beyond the bootloader.
Those did boot before (may take a while due to bitbanging i2c driver in guest taking time to read RTC) but haven't checked for a while. It could be some changes in AROS broke it or some recent change in QEMU. I'll test these later when I'll have some time.
and partly because I don't have any documentation of the hardware to tell me how it should behave, so patch 2 in particular has some FIXMEs. I also
I remember I've found some info on this in some similar SoC that had docs on-line but don't remember which. Maybe 440EPx/GPx or something like that. It may not be the same but seems similar enough for AmigaOS to work. I think the two main sources were PPC440EPx/GRx Embedded Processor User's Manual and NXP/Freescale Application Note AN2661 Software Migration from the IBM (AMCC) 440GP to the MPC8540 which seem to be similar to 460EX and have some info on the DMA controller registers.
notice that the code doesn't update any of the registers like the count or source/base addresses when the DMA transfer happens, which seems odd, but perhaps the real hardware does work like that.
The emulation is only partial, I were only concerned about the parts AmigaOS uses so it may be some things are off. The docs above talk about some increment bits so this may be settable but I did not read the whole docs again and don't remember anything about it now. Probably AmigaOS does not use it so I did not implement that.
I was going to test the first rc with all OSes I have on sam460ex and pegasos2 but maybe I'll try to do it this week then.
Regards, BALATON Zoltan
I think we should probably take patch 1 (which is a fairly minimal fix of the use-of-uninitialized-data problem),LGTM, Thanks, C.but patch 2 is a bit more unfinished. [*] The commit 3c409c1927efde2fc that added this code says it's used by AmigaOS.) thanks -- PMM Peter Maydell (2): hw/ppc/ppc440_uc: Initialize length passed to cpu_physical_memory_map() hw/ppc/ppc440_uc: Handle mapping failure in DMA engine hw/ppc/ppc440_uc.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
| [Prev in Thread] | Current Thread | [Next in Thread] |