| Message ID | 20220726182341.1888115-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | Fix Coverity and other errors in ppc440_uc DMA | expand |
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. > 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". > > I've marked these patches as RFC, partly because I don't have any > guest 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. There are images for the sam460ex images here : http://www.aros.org/nightly1.php But AFAICT, it does not go beyond the bootloader. > 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 > 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. > > 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(-) >
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 any >> guest 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(-) >> > >
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. > >> 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. Thanks, I'll see if I can track those down. -- PMM
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
On Tue, 26 Jul 2022, 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. > 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". > > I've marked these patches as RFC, partly because I don't have any > guest that would exercise the code changes[*], 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 > 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. > > I think we should probably take patch 1 (which is a fairly minimal > fix of the use-of-uninitialized-data problem), but patch 2 is a bit > more unfinished. > > [*] The commit 3c409c1927efde2fc that added this code says it's used > by AmigaOS.) AmigaOS still boots with these patches and I see no difference so Tested-by: BALATON Zoltan <balaton@eik.bme.hu> (I did not check what parameters AmigaOS uses (could not find a simple trace option for that, may need to add some debug printfs to test that) so not sure if the added code was actually run or it still only uses the code path as before. Fixing the map length should show some effect but I don't see any.) Regards, BALATON Zoltan > 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(-) > >
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. 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". I've marked these patches as RFC, partly because I don't have any guest that would exercise the code changes[*], 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 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. I think we should probably take patch 1 (which is a fairly minimal fix of the use-of-uninitialized-data problem), 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(-)