On Sun, Feb 19, 2023 at 10:54:34PM +0100, Philippe Mathieu-Daudé wrote: > +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? > > 2/ Why can we plug a PCI function on a PCIe bridge without using a > pcie-pci-bridge? FYI, this scenario is explicitly permitted by QEMU's PCIE docs, as without any bus= attr, the -device piix3-ide will end up attached to the PCI-E root complex. It thus becomes an integrated endpoint and does not support hotplug. If you want hotpluggable PCI-only device, you need to add a pcie-pci-bridge and a pci-bridge, attaching the endpoint to the latter PCE-E endpoints should always be in a pcie-root-port (or switch downstream port) > 3/ Chipsets come as a whole. Software drivers might expect all PCI > functions working (Linux considering each function individually > is not the norm) > 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? AFAICT, libvirt will never use '-device piix3-ide'. I vaguely recall that we discussed allowing it to enable use of > 4 IDE units, but decide it was too niche to care about. With q35 we'll just use ahci only With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
