Re: Memory leak in via_isa_realize()

[Peter Maydell]

On Mon, 21 Mar 2022 at 18:55, Cédric Le Goater <clg@kaod.org> wrote:
> I introduced quite a few of these calls,
>
>    hw/ppc/pnv_lpc.c:    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
>    hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(ics_set_irq, ics, 
> ics->nr_irqs);
>    hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(xive_source_set_irq, 
> xsrc, xsrc->nr_irqs);
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppc6xx_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppc970_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&power7_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&power9_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppc40x_set_irq,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppce500_set_irq,
>    hw/ppc/spapr_irq.c:    spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, 
> spapr,
>
> and may be I can remove some. What's the best practice ?

The 'best practice' is that if you have an irq line it should be
because it is the input (gpio or sysbus irq) or output (gpio) of
some device, ie something that is a subtype of TYPE_DEVICE.

For the ones in hw/ppc/ppc.c: we used to need to write code like that
because CPU objects weren't TYPE_DEVICE; now they are, and so you
can give them inbound gpio lines using qdev_init_gpio_in(), typically
in the cpu initfn. (See target/riscv for an example, or grep for
that function name in target/ for others.) Then the board code
needs to wire up to those IRQs in the usual way for GPIO lines,
ie using qdev_get_gpio_in(cpudev, ...), instead of directly
reaching into the CPU struct env->irq_inputs. (There's probably
a way to structure this change to avoid having to change the CPU
and all the board code at once, but I haven't thought it through.)

For the spapr one: this is in machine model code, and currently machines
aren't subtypes of TYPE_DEVICE. I'd leave this one alone for now;
we can come back and think about it later.

For pnv_psi.c: these appear to be because the PnvPsi device is
allocating irq lines which really should belong to the ICSState
object (and as a result the ICSState code is having to expose
ics->nr_irqs and the ics_set_irq function when they could be
internal to the ICSState code). The ICSState's init function
should be creating these as qdev gpio lines.

pnv_lpc.c seems to be ISA related. hw/isa/lpc_ich9.c is an
example of setting up IRQs for isa_bus_irqs() without using
qemu_allocate_irqs(), but there may be some more generalised
ISA cleanup possible here.

thanks
-- PMM