| Message ID | 20230215161641.32663-8-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/ide: Untangle ISA/PCI abuses of ide_init_ioport() | expand |
On Wed, Feb 15, 2023 at 5:20 PM Philippe Mathieu-Daudé <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> > --- > 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 Best regards, Bernhard
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! Should we cover it in qtests?
Am 16. Februar 2023 15:33:47 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >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! > >Should we cover it in qtests? Yes, why not. Preferably along with the x-remote machine.
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
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];
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> --- hw/ide/piix.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)