Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

+Daniel, Igor, Marcel & libvirt On 16/2/23 16:33, Philippe Mathieu-Daudé wrote: > On 16/2/23 15:43, Bernhard Beschow wrote: >> >> >> On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé <philmd(a)linaro.org <mailto:philmd@linaro.org>> wrote: >> >>     Ensure both IDE output IRQ lines are wired. >> >>     We can remove the last use of isa_get_irq(NULL). >> >>     Signed-off-by: Philippe Mathieu-Daudé <philmd(a)linaro.org >>     <mailto:philmd@linaro.org>> >>     --- >>       hw/ide/piix.c | 13 ++++++++----- >>       1 file changed, 8 insertions(+), 5 deletions(-) >> >>     diff --git a/hw/ide/piix.c b/hw/ide/piix.c >>     index 9d876dd4a7..b75a4ddcca 100644 >>     --- a/hw/ide/piix.c >>     +++ b/hw/ide/piix.c >>     @@ -133,14 +133,17 @@ static bool pci_piix_init_bus(PCIIDEState *d, >>     unsigned i, Error **errp) >>           static const struct { >>               int iobase; >>               int iobase2; >>     -        int isairq; >>           } port_info[] = { >>     -        {0x1f0, 0x3f6, 14}, >>     -        {0x170, 0x376, 15}, >>     +        {0x1f0, 0x3f6}, >>     +        {0x170, 0x376}, >>           }; >>           int ret; >> >>     -    qemu_irq irq_out = d->irq[i] ? : isa_get_irq(NULL, >>     port_info[i].isairq); >>     +    if (!d->irq[i]) { >>     +        error_setg(errp, "output IDE IRQ %u not connected", i); >>     +        return false; >>     +    } >>     + >>           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2); >>           ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase, >>                                 port_info[i].iobase2); >>     @@ -149,7 +152,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, >>     unsigned i, Error **errp) >>                                object_get_typename(OBJECT(d)), i); >>               return false; >>           } >>     -    ide_bus_init_output_irq(&d->bus[i], irq_out); >>     +    ide_bus_init_output_irq(&d->bus[i], d->irq[i]); >> >>           bmdma_init(&d->bus[i], &d->bmdma[i], d); >>           d->bmdma[i].bus = &d->bus[i]; >>     --     2.38.1 >> >> >> This now breaks user-created  piix3-ide: >> >>    qemu-system-x86_64 -M q35 -device piix3-ide >> >> Results in: >> >>    qemu-system-x86_64: -device piix3-ide: output IDE IRQ 0 not connected > > Thank you for this real-life-impossible-but-exists-in-QEMU test-case! Do we really want to maintain this Frankenstein use case? 1/ Q35 comes with a PCIe bus on which sits a ICH chipset, which already contains a AHCI function exposing multiple IDE buses. What is the point on using an older tech?

2/ Why can we plug a PCI function on a PCIe bridge without using a pcie-pci-bridge? 3/ Chipsets come as a whole. Software drivers might expect all PCI functions working (Linux considering each function individually is not the norm) I get your use case working with the following diff [*]: -- >8 -- diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 74e2f4288d..cb1628963a 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -140,8 +140,19 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp) }; if (!d->irq[i]) { - error_setg(errp, "output IDE IRQ %u not connected", i); - return false; + if (DEVICE_GET_CLASS(d)->user_creatable) { + /* Returns NULL unless there is exactly one ISA bus */ + Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, NULL); + + if (!isabus) { + error_setg(errp, "Unable to find a single ISA bus"); + return false; + } + d->irq[i] = isa_bus_get_irq(ISA_BUS(isabus), 14 + i); + } else { + error_setg(errp, "output IDE IRQ %u not connected", i); + return false; + } } ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2); @@ -201,6 +212,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_STORAGE_IDE; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); dc->hotpluggable = false; + /* + * This function is part of a Super I/O chip and shouldn't be user + * creatable. However QEMU accepts impossible hardware setups such + * plugging a PIIX IDE function on a ICH ISA bridge. + * Keep this Frankenstein (ab)use working. + */ + dc->user_creatable = true; } static const TypeInfo piix3_ide_info = { @@ -224,6 +242,8 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_STORAGE_IDE; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); dc->hotpluggable = false; + /* Reason: Part of a Super I/O chip */ + dc->user_creatable = false;

} --- But the hardware really looks Frankenstein now: (qemu) info qom-tree /machine (pc-q35-8.0-machine) /peripheral-anon (container) /device[0] (piix3-ide) /bmdma[0] (memory-region) /bmdma[1] (memory-region) /bus master container[0] (memory-region) /bus master[0] (memory-region) /ide.6 (IDE) /ide.7 (IDE) /ide[0] (memory-region) /ide[1] (memory-region) /ide[2] (memory-region) /ide[3] (memory-region) /piix-bmdma-container[0] (memory-region) /piix-bmdma[0] (memory-region) /piix-bmdma[1] (memory-region) /q35 (q35-pcihost) /unattached (container) /device[17] (ich9-ahci) /ahci-idp[0] (memory-region) /ahci[0] (memory-region) /bus master container[0] (memory-region) /bus master[0] (memory-region) /ide.0 (IDE) /ide.1 (IDE) /ide.2 (IDE) /ide.3 (IDE) /ide.4 (IDE) /ide.5 (IDE) /device[2] (ICH9-LPC) /bus master container[0] (memory-region) /bus master[0] (memory-region) /ich9-pm[0] (memory-region) /isa.0 (ISA) I guess the diff [*] is acceptable as a kludge, but we might consider deprecating such use. Paolo, Michael & libvirt folks, what do you think? Regards, Phil.