Re: [PATCH 2/3] hw/isa/vt82c686: Resolve intermediate IRQ forwarder

[BALATON Zoltan]

On Fri, 5 Jul 2024, BALATON Zoltan wrote:
> On Thu, 4 Jul 2024, Bernhard Beschow wrote:
> > When @cpu_intr is populated before vt82xx's realize(), it can be directly 
> > passed
> > to i8259_init(), avoiding the need for the intermediate
> > via_isa_request_i8259_irq() handler. The result is less code and runtime
> > overhead, and a fixed memory leak caused by qemu_allocate_irqs().
> >
> > Inspired-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> > hw/isa/vt82c686.c   | 12 ++----------
> > hw/mips/fuloong2e.c |  2 +-
> > hw/ppc/amigaone.c   |  8 ++++----
> > hw/ppc/pegasos2.c   |  4 ++--
> > 4 files changed, 9 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 505b44c4e6..ca02ad4c20 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -624,6 +624,7 @@ static void via_isa_init(Object *obj)
> >     object_initialize_child(obj, "uhci2", &s->uhci[1], 
> > TYPE_VT82C686B_USB_UHCI);
> >     object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
> >     object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> > +    qdev_init_gpio_out_named(DEVICE(obj), &s->cpu_intr, "intr", 1);
> > }
> >
> > static const TypeInfo via_isa_info = {
> > @@ -704,24 +705,15 @@ static void via_isa_pirq(void *opaque, int pin, int 
> > level)
> >     via_isa_set_irq(opaque, pin, level);
> > }
> >
> > -static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
> > -{
> > -    ViaISAState *s = opaque;
> > -    qemu_set_irq(s->cpu_intr, level);
> > -}
> > -
> > static void via_isa_realize(PCIDevice *d, Error **errp)
> > {
> >     ViaISAState *s = VIA_ISA(d);
> >     DeviceState *dev = DEVICE(d);
> >     PCIBus *pci_bus = pci_get_bus(d);
> > -    qemu_irq *isa_irq;
> >     ISABus *isa_bus;
> >     int i;
> >
> > -    qdev_init_gpio_out_named(dev, &s->cpu_intr, "intr", 1);
> >     qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
>
> I still don't like how this makes handling of out and in gpios different and 
> it also prevents to create the device with pci_create_simple_multifunction() 
> and needs tweaking before realize. I think the fix should be in i8259 and not 
> in this device.

I mean users of this device should not need changing.

> Regards,
> BALATON Zoltan
>
> > -    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
> >     isa_bus = isa_bus_new(dev, pci_address_space(d), 
> > pci_address_space_io(d),
> >                           errp);
> >
> > @@ -729,7 +721,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
> >         return;
> >     }
> >
> > -    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
> > +    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
> >     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
> >     i8254_pit_init(isa_bus, 0x40, 0, NULL);
> >     i8257_dma_init(OBJECT(d), isa_bus, 0);
> > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> > index 6e4303ba47..e6487c34d8 100644
> > --- a/hw/mips/fuloong2e.c
> > +++ b/hw/mips/fuloong2e.c
> > @@ -286,6 +286,7 @@ static void mips_fuloong2e_init(MachineState *machine)
> >     /* South bridge -> IP5 */
> >     pci_dev = pci_new_multifunction(PCI_DEVFN(FULOONG2E_VIA_SLOT, 0),
> >                                     TYPE_VT82C686B_ISA);
> > +    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 0, env->irq[5]);
> >
> >     /* Set properties on individual devices before realizing the south 
> > bridge */
> >     if (machine->audiodev) {
> > @@ -299,7 +300,6 @@ static void mips_fuloong2e_init(MachineState *machine)
> >                               object_resolve_path_component(OBJECT(pci_dev),
> >                                                             "rtc"),
> >                               "date");
> > -    qdev_connect_gpio_out_named(DEVICE(pci_dev), "intr", 0, env->irq[5]);
> >
> >     dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "ide"));
> >     pci_ide_create_devs(PCI_DEVICE(dev));
> > diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> > index 9dcc486c1a..2110875f56 100644
> > --- a/hw/ppc/amigaone.c
> > +++ b/hw/ppc/amigaone.c
> > @@ -148,13 +148,13 @@ static void amigaone_init(MachineState *machine)
> >     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
> >
> >     /* VIA VT82c686B South Bridge (multifunction PCI device) */
> > -    via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
> > -                                                 TYPE_VT82C686B_ISA));
> > +    via = OBJECT(pci_new_multifunction(PCI_DEVFN(7, 0), 
> > TYPE_VT82C686B_ISA));
> > +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> > +                                qdev_get_gpio_in(DEVICE(cpu), 
> > PPC6xx_INPUT_INT));
> > +    pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_abort);
> >     object_property_add_alias(OBJECT(machine), "rtc-time",
> >                               object_resolve_path_component(via, "rtc"),
> >                               "date");
> > -    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> > -                                qdev_get_gpio_in(DEVICE(cpu), 
> > PPC6xx_INPUT_INT));
> >     for (i = 0; i < PCI_NUM_PINS; i++) {
> >         qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
> >                                                              "pirq", i));
> > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> > index 9b0a6b70ab..54e60082ce 100644
> > --- a/hw/ppc/pegasos2.c
> > +++ b/hw/ppc/pegasos2.c
> > @@ -181,6 +181,8 @@ static void pegasos2_init(MachineState *machine)
> >
> >     /* VIA VT8231 South Bridge (multifunction PCI device) */
> >     via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), TYPE_VT8231_ISA));
> > +    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> > +                                qdev_get_gpio_in_named(pm->mv, "gpp", 
> > 31));
> >
> >     /* Set properties on individual devices before realizing the south 
> > bridge */
> >     if (machine->audiodev) {
> > @@ -195,8 +197,6 @@ static void pegasos2_init(MachineState *machine)
> >     object_property_add_alias(OBJECT(machine), "rtc-time",
> >                               object_resolve_path_component(via, "rtc"),
> >                               "date");
> > -    qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
> > -                                qdev_get_gpio_in_named(pm->mv, "gpp", 
> > 31));
> >
> >     dev = PCI_DEVICE(object_resolve_path_component(via, "ide"));
> >     pci_ide_create_devs(dev);