Re: [PATCH v2 07/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization
+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.
Am 19. Februar 2023 21:54:34 UTC schrieb "Philippe Mathieu-Daudé" <philmd@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@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?
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.
On Mon, 20 Feb 2023, Bernhard Beschow wrote:
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.
I think that's not 2 major releases just 2 releases so in your example could be removed in 9.0. qemu/docs/about/deprecated.rst says: "The feature will remain functional for the release in which it was deprecated and one further release. After these two releases, the feature is liable to be removed." Regards, BALATON Zoltan
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 :|
participants (4)
-
BALATON Zoltan -
Bernhard Beschow -
Daniel P. Berrangé -
Philippe Mathieu-Daudé