In order to allow Frankenstein uses such plugging a PIIX3
IDE function on a ICH9 chipset (which already exposes AHCI
ports...) as:

  $ qemu-system-x86_64 -M q35 -device piix3-ide

add a kludge to automatically wires the IDE IRQs on an ISA
bus exposed by a PCI-to-ISA bridge (usually function #0).
Restrict this kludge to the PIIX3.

Reported-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9d876dd4a7..50975a16b3 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -170,6 +170,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
 
+    if (!d->irq[0] && !d->irq[1] && DEVICE_GET_CLASS(d)->user_creatable) {
+        /* kludge specific to TYPE_PIIX3_IDE */
+        Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, NULL);
+
+        if (!isabus) {
+            error_setg(errp, "Unable to find a unique ISA bus");
+            return;
+        }
+        d->irq[0] = isa_bus_get_irq(ISA_BUS(isabus), 14);
+        d->irq[1] = isa_bus_get_irq(ISA_BUS(isabus), 15);
+    }
     for (unsigned i = 0; i < ARRAY_SIZE(d->irq); i++) {
         if (!pci_piix_init_bus(d, i, errp)) {
             return;
@@ -202,6 +213,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 = {
@@ -225,6 +243,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;
 }
 
 static const TypeInfo piix4_ide_info = {
-- 
2.38.1

On Mon, Feb 20, 2023 at 09:00:44AM +0100, Philippe Mathieu-Daudé wrote:
> In order to allow Frankenstein uses such plugging a PIIX3
> IDE function on a ICH9 chipset (which already exposes AHCI
> ports...) as:
> 
>   $ qemu-system-x86_64 -M q35 -device piix3-ide
> 
> add a kludge to automatically wires the IDE IRQs on an ISA
> bus exposed by a PCI-to-ISA bridge (usually function #0).
> Restrict this kludge to the PIIX3.

Well.  On physical hardware you have a config switch in the bios
setup which turns off sata and enables ide instead (i.e. the
chipset implements both and can be configured to expose the one
or the other).

If we want support ide for q35 we should IMHO do something simliar
instead of making piix-ide user-pluggable.  We already have -machine
q35,sata={on,off}.  We could extend that somehow, by adding
ide={on,off}, or by using storage={sata,ide,off} instead.

This has been discussed now and then in the past and the usual
conclusion was that there is little reason to implement that given
that you can just use the 'pc' machine type.  For guests that old
that they can't handle sata storage this is usually the better fit
anyway ...

take care,
  Gerd

On 20/2/23 10:10, Gerd Hoffmann wrote:
> On Mon, Feb 20, 2023 at 09:00:44AM +0100, Philippe Mathieu-Daudé wrote:
>> In order to allow Frankenstein uses such plugging a PIIX3
>> IDE function on a ICH9 chipset (which already exposes AHCI
>> ports...) as:
>>
>>    $ qemu-system-x86_64 -M q35 -device piix3-ide
>>
>> add a kludge to automatically wires the IDE IRQs on an ISA
>> bus exposed by a PCI-to-ISA bridge (usually function #0).
>> Restrict this kludge to the PIIX3.
> 
> Well.  On physical hardware you have a config switch in the bios
> setup which turns off sata and enables ide instead (i.e. the
> chipset implements both and can be configured to expose the one
> or the other).
> 
> If we want support ide for q35 we should IMHO do something simliar
> instead of making piix-ide user-pluggable.  We already have -machine
> q35,sata={on,off}.  We could extend that somehow, by adding
> ide={on,off}, or by using storage={sata,ide,off} instead.
> 
> This has been discussed now and then in the past and the usual
> conclusion was that there is little reason to implement that given
> that you can just use the 'pc' machine type.  For guests that old
> that they can't handle sata storage this is usually the better fit
> anyway ...

I think we might not using the same words, but agree on the goal :)

Since this has been discussed in the past, I suppose some users
want this config available. Why are they using this convoluted
config? Could it be due to lack of good documentation?

Do we really need a storage={sata,ide,off} flag? I don't see its
value. Help cloud users to have a sane config?

(old) boards exist with both IDE/SATA and we might want to emulate
some of them, but IMO such boards should be well modeled (Either
in C or later in declarative language) but not automagically created
from CLI.

IOW:

- using PIIX on Q35 (or any machine exposing a PCI bus) is
   acceptable, but the chipset should be instantiated as a whole.
   So to be clear I'm not against "-M q35 -device piix3", we should
   keep that working.

- we shouldn't try to maintain such Frankenstein corner cases which
   force us to add complex hacks, hard to remove, which limit us
   in implementing new features and cost us a lot of maintenance /
   testing time.

So I propose we deprecate this config so we can keep going forward.

(More generally I'd like to not allow instantiating part of chipset).

On 20/2/23 10:52, Philippe Mathieu-Daudé wrote:
> On 20/2/23 10:10, Gerd Hoffmann wrote:
>> On Mon, Feb 20, 2023 at 09:00:44AM +0100, Philippe Mathieu-Daudé wrote:
>>> In order to allow Frankenstein uses such plugging a PIIX3
>>> IDE function on a ICH9 chipset (which already exposes AHCI
>>> ports...) as:
>>>
>>>    $ qemu-system-x86_64 -M q35 -device piix3-ide
>>>
>>> add a kludge to automatically wires the IDE IRQs on an ISA
>>> bus exposed by a PCI-to-ISA bridge (usually function #0).
>>> Restrict this kludge to the PIIX3.
>>
>> Well.  On physical hardware you have a config switch in the bios
>> setup which turns off sata and enables ide instead (i.e. the
>> chipset implements both and can be configured to expose the one
>> or the other).
>>
>> If we want support ide for q35 we should IMHO do something simliar
>> instead of making piix-ide user-pluggable.  We already have -machine
>> q35,sata={on,off}.  We could extend that somehow, by adding
>> ide={on,off}, or by using storage={sata,ide,off} instead.
>>
>> This has been discussed now and then in the past and the usual
>> conclusion was that there is little reason to implement that given
>> that you can just use the 'pc' machine type.  For guests that old
>> that they can't handle sata storage this is usually the better fit
>> anyway ...
> 
> I think we might not using the same words, but agree on the goal :)
> 
> Since this has been discussed in the past, I suppose some users
> want this config available. Why are they using this convoluted
> config? Could it be due to lack of good documentation?
> 
> Do we really need a storage={sata,ide,off} flag? I don't see its
> value. Help cloud users to have a sane config?
> 
> (old) boards exist with both IDE/SATA and we might want to emulate
> some of them, but IMO such boards should be well modeled (Either
> in C or later in declarative language) but not automagically created
> from CLI.
> 
> IOW:
> 
> - using PIIX on Q35 (or any machine exposing a PCI bus) is
>    acceptable, but the chipset should be instantiated as a whole.
>    So to be clear I'm not against "-M q35 -device piix3", we should
>    keep that working.
> 
> - we shouldn't try to maintain such Frankenstein corner cases which
>    force us to add complex hacks, hard to remove, which limit us
>    in implementing new features and cost us a lot of maintenance /
>    testing time.
> 
> So I propose we deprecate this config so we can keep going forward.
> 
> (More generally I'd like to not allow instantiating part of chipset).

So there is a design problem here.

- ICH expose ISA bridge (fn0) with ISA IRQs.
- internal ICH SATA fn wires the IRQs 14/15

if we plug a cripple piix3-ide function which lookup for fn0 (ISA
bridge) to wire its IRQs 14/15, we end up having 2 devices able
to raise/lower IRQs while in the BQL iothread.
IOW, one device raise an IRQ, while the other lower the same IRQ...
thus the 1st device IRQ is acked from HW and missed from guest SW.

Daniel tested such config (2 drives used concurrently, one on IDE
and one on SATA) and reported "lost interrupt" from dmesg.

I haven't investigated using a SplitIrq object, but this doesn't
match real hw.

If user want to suffer from poor quality, we should find a different
(valid) config to add IDE drives to Q35 machine. Or maybe it is
a documentation problem, and we should redirect the users to the
best config.

Ref about similar problems:
https://lore.kernel.org/qemu-devel/b8d457d1-40b1-adb5-a2ac-98070f62ac1e@eik.bme.hu/
https://lore.kernel.org/qemu-devel/Y%2FSu8eB2nIO0INOX@redhat.com/

PD: For the remote-pci it is different because it is based on
PCIe which serializes MSIX IRQs, so no way to overwrite one.