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

[Philippe Mathieu-Daudé]

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

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.