Am 19. Februar 2023 21:54:34 UTC schrieb "Philippe Mathieu-Daudé"
<phi...@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é <phi...@linaro.org
>>> <mailto:phi...@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é <phi...@linaro.org
>>> <mailto:phi...@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.