Re: Memory leak in via_isa_realize()

[Bernhard Beschow]

Am 22. März 2022 08:23:09 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
>On 21/03/2022 20:35, Peter Maydell wrote:
>
>> 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.
>
>The issue with PPC IRQs also affects the OpenPIC implementation: when I last 
>looked a 
>while back I didn't see any obvious issues against using gpio IRQs, but the 
>main 
>blocker for me was not being able to test all the different PPC machine 
>configurations.
>
>Out of curiosity does anyone know how to test the KVM in-kernel OpenPIC 
>implementation in hw/intc/openpic_kvm.c? It seems to be used for e500 only.
>
>I think there is some good work to be done converting ISA devices over to 
>using GPIOs 
>and improving the interaction with PCI, but it's something that still remains 
>on my 
>TODO list. Again the changes would be mostly mechanical with the main concern 
>being 
>over testing to ensure that there are no regressions.

If the changes would be mostly mechanical: wouldn't they make for some good, 
bite-sized junior jobs? That way, progress could also be stretched over time, 
allowing potential regressions to be ascribed more easily.

Moreover, I would be interested in converting hw/ide/piix.c. AFAICS it contains 
the only invocation of isa_get_irq() where NULL is passed for *dev. If this 
invocation could be moved and a meaningful non-NULL value be passed, I think 
it'd be possible to remove the isabus global. This means that - in theory - we 
could create as many ISABuses as we'd like! Testing would be easy, too, because 
the Malta board seems to use this code path (at least it crashes when 
isa_get_irq() asserts dev != NULL).

Best regards,
Bernhard
>
>
>ATB,
>
>Mark.