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

[Daniel P . Berrangé]

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@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@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?

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 :|