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(-)

-- 
2.25.1

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(-)
>
>

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