From 38200011319b5819ff268dadb1b175faa6b0764a Mon Sep 17 00:00:00 2001 From: BALATON Zoltan Date: Wed, 1 Mar 2023 01:17:08 +0100 Subject: [PATCH] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To be 'usable', QDev objects (which are QOM objects) must be 1/ initialized (at this point their properties can be modified), then 2/ realized (properties are consumed). Some devices (objects) might depend on other devices. When creating the 'QOM composition tree', parent objects can't be 'realized' until all their children are. We might also have circular dependencies. A common circular dependency occurs with IRQs. Device (A) has an output IRQ wired to device (B), and device (B) has one to device (A). When (A) is realized and connects its IRQ to an unrealized (B), the IRQ handler on (B) is not yet created. QEMU pass IRQ between objects as pointer. When (A) poll (B)'s IRQ, it is NULL. Later (B) is realized and its IRQ pointers are populated, but (A) keeps a reference to a NULL pointer. A common pattern to bypass this circular limitation is to use 'proxy' objects. Proxy (P) is created (and realized) before (A) and (B). Then (A) and (B) can be created in different order, it doesn't matter: (P) pointers are already populated. Commit bb98e0f59cde ("hw/isa/vt82c686: Remove intermediate IRQ forwarder") neglected the QOM/QDev circular dependency issue, and removed the 'proxy' between the southbridge, its PCI functions and the interrupt controller, resulting in PCI functions wiring output IRQs to 'NULL', leading to guest failures (IRQ never delivered) [1] [2]. Since we are entering feature freeze, it is safer to revert the offending patch until we figure a way to strengthen our APIs. [1] https://lore.kernel.org/qemu-devel/928a8552-ab62-9e6c-a492-d6453e338b9d@redhat.com/ [2] https://lore.kernel.org/qemu-devel/cover.1677628524.git.balaton@eik.bme.hu/ Signed-off-by: BALATON Zoltan Tested-by: Rene Engel Reviewed-by: Daniel Henrique Barboza Message-Id: [PMD: Reworded description] Inspired-by: Bernhard Beschow Signed-off-by: Philippe Mathieu-Daudé --- hw/isa/vt82c686.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index f4c40965cd..01e0148967 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -598,15 +598,23 @@ void via_isa_set_irq(PCIDevice *d, int n, int level) qemu_set_irq(s->isa_irqs_in[n], 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(dev, &s->cpu_intr, 1); + 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); @@ -614,7 +622,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) return; } - s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr); + s->isa_irqs_in = i8259_init(isa_bus, *isa_irq); isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in); i8254_pit_init(isa_bus, 0x40, 0, NULL); i8257_dma_init(isa_bus, 0);