This series is 6.0 material really I think.  It's a bit of cleanup
prompted by a Coverity issue, CID 1421883.  There are another half
dozen or so similar issues, where Coverity is complaining that we
allocate an array of qemu_irqs with qemu_allocate_irqs() in a board
init function -- in this case the 'pic' array in q800_init() -- and
then we return from the board init function and the memory is leaked,
in the sense that nobody has a pointer to it any more.

The leak isn't real, in that board init functions are called only
once, and the array of qemu_irqs really does need to stay around for
the life of the simulation, so these are pretty much insignificant
as Coverity issues go. But this coding style which uses a free-floating
set of qemu_irqs is not very "modern QEMU", so the issues act as
a nudge that we should clean the code up by encapsulating the
interrupt-line behaviour in a QOM device. In the q800 case there
actually is already a GLUEState struct, it just needs to be turned
into a QOM device with GPIO input lines. Patch 2 does that.

Patch 1 fixes a bug I noticed while doing this work -- it's
not valid to connect two qemu_irq lines directly to the same
input (here both ESCC irq lines go to pic[3]) because it produces
weird behaviour like "both lines are asserted but the device
consuming the interrupt sees the line deassert when one of the
two inputs goes low, rather than only when they both go low".
You need to put an explicit OR gate in, assuming that logical-OR
is the desired behaviour, which it usually is.

Tested only with 'make check' and 'make check-acceptance',
but the latter does have a q800 bootup test.

thanks
-- PMM

Peter Maydell (2):
  hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
  hw/m68k/q800.c: Make the GLUE chip an actual QOM device

 hw/m68k/q800.c  | 92 ++++++++++++++++++++++++++++++++++++++++++-------
 hw/m68k/Kconfig |  1 +
 2 files changed, 80 insertions(+), 13 deletions(-)

-- 
2.20.1

On Fri, 6 Nov 2020 at 23:51, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This series is 6.0 material really I think.  It's a bit of cleanup
> prompted by a Coverity issue, CID 1421883.  There are another half
> dozen or so similar issues, where Coverity is complaining that we
> allocate an array of qemu_irqs with qemu_allocate_irqs() in a board
> init function -- in this case the 'pic' array in q800_init() -- and
> then we return from the board init function and the memory is leaked,
> in the sense that nobody has a pointer to it any more.
>
> The leak isn't real, in that board init functions are called only
> once, and the array of qemu_irqs really does need to stay around for
> the life of the simulation, so these are pretty much insignificant
> as Coverity issues go. But this coding style which uses a free-floating
> set of qemu_irqs is not very "modern QEMU", so the issues act as
> a nudge that we should clean the code up by encapsulating the
> interrupt-line behaviour in a QOM device. In the q800 case there
> actually is already a GLUEState struct, it just needs to be turned
> into a QOM device with GPIO input lines. Patch 2 does that.
>
> Patch 1 fixes a bug I noticed while doing this work -- it's
> not valid to connect two qemu_irq lines directly to the same
> input (here both ESCC irq lines go to pic[3]) because it produces
> weird behaviour like "both lines are asserted but the device
> consuming the interrupt sees the line deassert when one of the
> two inputs goes low, rather than only when they both go low".
> You need to put an explicit OR gate in, assuming that logical-OR
> is the desired behaviour, which it usually is.
>
> Tested only with 'make check' and 'make check-acceptance',
> but the latter does have a q800 bootup test.

Laurent, did you want to take this series as an m68k patchset,
or would you rather I just put it in via the target-arm queue?

thanks
-- PMM

Le 07/11/2020 à 00:51, Peter Maydell a écrit :
> This series is 6.0 material really I think.  It's a bit of cleanup
> prompted by a Coverity issue, CID 1421883.  There are another half
> dozen or so similar issues, where Coverity is complaining that we
> allocate an array of qemu_irqs with qemu_allocate_irqs() in a board
> init function -- in this case the 'pic' array in q800_init() -- and
> then we return from the board init function and the memory is leaked,
> in the sense that nobody has a pointer to it any more.
> 
> The leak isn't real, in that board init functions are called only
> once, and the array of qemu_irqs really does need to stay around for
> the life of the simulation, so these are pretty much insignificant
> as Coverity issues go. But this coding style which uses a free-floating
> set of qemu_irqs is not very "modern QEMU", so the issues act as
> a nudge that we should clean the code up by encapsulating the
> interrupt-line behaviour in a QOM device. In the q800 case there
> actually is already a GLUEState struct, it just needs to be turned
> into a QOM device with GPIO input lines. Patch 2 does that.
> 
> Patch 1 fixes a bug I noticed while doing this work -- it's
> not valid to connect two qemu_irq lines directly to the same
> input (here both ESCC irq lines go to pic[3]) because it produces
> weird behaviour like "both lines are asserted but the device
> consuming the interrupt sees the line deassert when one of the
> two inputs goes low, rather than only when they both go low".
> You need to put an explicit OR gate in, assuming that logical-OR
> is the desired behaviour, which it usually is.
> 
> Tested only with 'make check' and 'make check-acceptance',
> but the latter does have a q800 bootup test.
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>   hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
>   hw/m68k/q800.c: Make the GLUE chip an actual QOM device
> 
>  hw/m68k/q800.c  | 92 ++++++++++++++++++++++++++++++++++++++++++-------
>  hw/m68k/Kconfig |  1 +
>  2 files changed, 80 insertions(+), 13 deletions(-)
> 

Applied to my m68k-for-6.0 branch

Thanks,
Laurent