Am 19. Februar 2023 21:54:34 UTC schrieb "Philippe Mathieu-Daudé" <philmd@xxxxxxxxxx>: >+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@xxxxxxxxxx <mailto:philmd@xxxxxxxxxx>> 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@xxxxxxxxxx >>> <mailto:philmd@xxxxxxxxxx>> >>> --- >>> 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? I just chose Q35 in the test case to demonstrate that we'd not meet our own deprecation rules. IIUC, QEMU guarantees a deprecation period for at least two major versions. So if we deprecated user-creatable piix-ide in 8.1, we are not allowed to remove it before 10.1. Let's stick to our rules to give our users a chance to adapt gracefully. > >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; piix3-ide and piix4-ide are implemented in the same file. This means that above test case should also apply for piix4-ide, even when CONFIG_PIIX4=n. To meet our deprecation rules, we're not allowed to treat piix4 differently. Best regards, Bernhard > } >--- > >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.
