[PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization
Daniel P. Berrangé
berrange at redhat.com
Tue Feb 21 11:45:53 UTC 2023
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 at linaro.org <mailto:philmd at 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 at linaro.org
> > > <mailto:philmd at 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?
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 :|
More information about the libvir-list
mailing list