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

Bernhard Beschow shentey at gmail.com
Mon Feb 20 23:49:19 UTC 2023



Am 19. Februar 2023 21:54:34 UTC schrieb "Philippe Mathieu-Daudé" <philmd at linaro.org>:
>+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?

I just chose Q35 in the test case to demonstrate that we'd not meet our own deprecation rules.

IIUC, QEMU guarantees a deprecation period for at least two major versions. So if we deprecated user-creatable piix-ide in 8.1, we are not allowed to remove it before 10.1. Let's stick to our rules to give our users a chance to adapt gracefully.

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

piix3-ide and piix4-ide are implemented in the same file. This means that above test case should also apply for piix4-ide, even when CONFIG_PIIX4=n. To meet our deprecation rules, we're not allowed to treat piix4 differently.

Best regards,
Bernhard

> }
>---
>
>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.



More information about the libvir-list mailing list