Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances

[Bernhard Beschow]

Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
>On 05/02/2023 22:21, BALATON Zoltan wrote:
>
>> On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
>>> On 26/01/2023 21:17, Bernhard Beschow wrote:
>>>> Internal instances now defer interrupt wiring to the caller which
>>>> decouples them from the ISABus. User-created devices still fish out the
>>>> ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
>>>> The latter mechanism is considered a workaround and intended to be
>>>> removed once a deprecation period for user-created PIIX IDE devices is
>>>> over.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>   include/hw/ide/pci.h |  1 +
>>>>   hw/ide/piix.c        | 64 ++++++++++++++++++++++++++++++++++----------
>>>>   hw/isa/piix.c        |  5 ++++
>>>>   3 files changed, 56 insertions(+), 14 deletions(-)
>>>> 
>>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>>>> index 24c0b7a2dd..ee2c8781b7 100644
>>>> --- a/include/hw/ide/pci.h
>>>> +++ b/include/hw/ide/pci.h
>>>> @@ -54,6 +54,7 @@ struct PCIIDEState {
>>>>       MemoryRegion bmdma_bar;
>>>>       MemoryRegion cmd_bar[2];
>>>>       MemoryRegion data_bar[2];
>>>> +    bool user_created;
>>>>   };
>>>>     static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
>>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>>>> index 5980045db0..f0d95761ac 100644
>>>> --- a/hw/ide/piix.c
>>>> +++ b/hw/ide/piix.c
>>>> @@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>>>       }
>>>>   }
>>>>   +static void piix_ide_set_irq(void *opaque, int n, int level)
>>>> +{
>>>> +    PCIIDEState *d = opaque;
>>>> +
>>>> +    qemu_set_irq(d->isa_irqs[n], level);
>>>> +}
>>>> +
>>>>   static void piix_ide_reset(DeviceState *dev)
>>>>   {
>>>>       PCIIDEState *d = PCI_IDE(dev);
>>>> @@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, 
>>>> ISABus *isa_bus)
>>>>       };
>>>>       int i;
>>>>   +    if (isa_bus) {
>>>> +        d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
>>>> +        d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
>>>> +    } else {
>>>> +        qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
>>>> +    }
>>>> +
>>>>       for (i = 0; i < 2; i++) {
>>>>           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>>>           ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>>>>                           port_info[i].iobase2);
>>>> -        ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
>>>> +        ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
>>>>             bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>>>           d->bmdma[i].bus = &d->bus[i];
>>>> @@ -154,8 +168,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
>>>> **errp)
>>>>   {
>>>>       PCIIDEState *d = PCI_IDE(dev);
>>>>       uint8_t *pci_conf = dev->config;
>>>> -    ISABus *isa_bus;
>>>> -    bool ambiguous;
>>>> +    ISABus *isa_bus = NULL;
>>>>         pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>>>   @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, 
>>>> Error **errp)
>>>>         vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>>>>   -    isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS, 
>>>> &ambiguous));
>>>> -    if (ambiguous) {
>>>> -        error_setg(errp,
>>>> -                   "More than one ISA bus found while %s supports only 
>>>> one",
>>>> -                   object_get_typename(OBJECT(dev)));
>>>> -        return;
>>>> -    }
>>>> -    if (!isa_bus) {
>>>> -        error_setg(errp, "No ISA bus found while %s requires one",
>>>> -                   object_get_typename(OBJECT(dev)));
>>>> -        return;
>>>> +    if (d->user_created) {
>>>> +        bool ambiguous;
>>>> +
>>>> +        isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS,
>>>> +                                                   &ambiguous));
>>>> +
>>>> +        if (ambiguous) {
>>>> +            error_setg(errp,
>>>> +                       "More than one ISA bus found while %s supports 
>>>> only one",
>>>> +                       object_get_typename(OBJECT(dev)));
>>>> +            return;
>>>> +        }
>>>> +
>>>> +        if (!isa_bus) {
>>>> +            error_setg(errp, "No ISA bus found while %s requires one",
>>>> +                       object_get_typename(OBJECT(dev)));
>>>> +            return;
>>>> +        }
>>>>       }
>>>>         pci_piix_init_ports(d, isa_bus);
>>>>   }
>>>>   +static void pci_piix_ide_init(Object *obj)
>>>> +{
>>>> +    DeviceState *dev = DEVICE(obj);
>>>> +
>>>> +    qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
>>>> +}
>>>> +
>>>>   static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>   {
>>>>       PCIIDEState *d = PCI_IDE(dev);
>>>> @@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>>>       }
>>>>   }
>>>>   +static Property piix_ide_properties[] = {
>>>> +    DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>> +
>>>>   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>>>>   static void piix3_ide_class_init(ObjectClass *klass, void *data)
>>>>   {
>>>> @@ -205,11 +237,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;
>>>> +    device_class_set_props(dc, piix_ide_properties);
>>>>   }
>>>>     static const TypeInfo piix3_ide_info = {
>>>>       .name          = TYPE_PIIX3_IDE,
>>>>       .parent        = TYPE_PCI_IDE,
>>>> +    .instance_init = pci_piix_ide_init,
>>>>       .class_init    = piix3_ide_class_init,
>>>>   };
>>>>   @@ -227,11 +261,13 @@ 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;
>>>> +    device_class_set_props(dc, piix_ide_properties);
>>>>   }
>>>>     static const TypeInfo piix4_ide_info = {
>>>>       .name          = TYPE_PIIX4_IDE,
>>>>       .parent        = TYPE_PCI_IDE,
>>>> +    .instance_init = pci_piix_ide_init,
>>>>       .class_init    = piix4_ide_class_init,
>>>>   };
>>>>   diff --git a/hw/isa/piix.c b/hw/isa/piix.c
>>>> index 54a1246a9d..f9974c2a77 100644
>>>> --- a/hw/isa/piix.c
>>>> +++ b/hw/isa/piix.c
>>>> @@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const 
>>>> char *uhci_type,
>>>>         /* IDE */
>>>>       qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
>>>> +    qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
>>>>       if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
>>>>           return;
>>>>       }
>>>> +    qdev_connect_gpio_out(DEVICE(&d->ide), 0,
>>>> +                          qdev_get_gpio_in(DEVICE(&d->pic), 14));
>>>> +    qdev_connect_gpio_out(DEVICE(&d->ide), 1,
>>>> +                          qdev_get_gpio_in(DEVICE(&d->pic), 15));
>>>>         /* USB */
>>>>       if (d->has_usb) {
>>> 
>>> I haven't checked the datasheet, but I suspect this will be similar to the 
>>> cmd646/via PCI-IDE interfaces in that there will be a PCI configuration 
>>> register that will switch between ISA compatibility mode (and ISA irqs) and 
>>> PCI mode (with PCI IRQs). So it would be the device configuration that 
>>> would specify PCI or ISA mode, rather than the presence of an ISABus.
>> 
>> I forgot about this topic already and haven't follwed this series either so 
>> what I say may not fully make sense but I think CMD646 and via-ide are 
>> different. CMD646 is a PCI device and should use PCI interrupts while 
>> via-ide is part of a southbridge/superio complex and connected to the ISA 
>> PICs within that southbride, so I think via-ide always uses ISA IRQs and the 
>> ISA btidge within the same chip may convert that to PCI IRQs or not (that 
>> part is where I'm lost also because we may not actually model it that way). 
>> After a long debate we managed to find a solution back then that works for 
>> every guest we use it for now so I think we don't want to touch it now until 
>> some real need arises. It does not worth the trouble and added complexity to 
>> model something that is not used just for the sake of correctness. By the 
>> time we find a use for that, the ISA emulation may evolve so it's easier to 
>> implement the missing switching between isa and native mode or we may want 
>> to do it differently (such as we do things differently now compared to what 
>> we did years ago). So I think it does not worth keeping the ISA model from 
>> being simplified for some theoretical uses in the future which we may not 
>> actually do any time soon. But I don't want to get into this again so just 
>> shared my thoughts and feel free to ignore it. I don't care where these 
>> patches go as long as the VIA model keeps working for me.
>
>I have a vague memory that ISA compatibility mode was part of the original 
>PCI-BMDMA specification, but it has been a while since I last looked.
>
>Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference 
>the cmd646 datasheet specifies that ISA mode or PCI mode is determined by 
>register PROG_IF (0x9) in PCI configuration space.

I've found the following:

  "Only PCI masters have access to the IDE port. ISA Bus masters cannot access 
the IDE I/O port addresses. Memory targeted by the IDE interface acting as a 
PCI Bus master on behalf of IDE DMA slaves must reside on PCI, usually main 
memory implemented by the host-to-PCI bridge."

And:

  "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."

Does this perhaps mean that piix-ide does indeed have no ISA bus?

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.