Am 22. März 2022 08:23:09 UTC schrieb Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>: >On 21/03/2022 20:35, Peter Maydell wrote: > >> On Mon, 21 Mar 2022 at 18:55, Cédric Le Goater <c...@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.