3:54 p.m.
+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(a)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(a)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.