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

[Bernhard Beschow]

Am 1. März 2023 16:42:16 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
>On 23/02/2023 20:46, Bernhard Beschow wrote:
>> 
>> 
>> Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland 
>> <mark.cave-ayland@ilande.co.uk>:
>>> On 06/02/2023 23:40, Bernhard Beschow wrote:
>>> 
>>>> 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?
>>> 
>>> I'd be amazed if that were the case: certainly when the first motherboards 
>>> came out with PCI and ISA slots, I'd expect the IDE legacy mode to be 
>>> enabled by default since BIOSes and OSs such as DOS wouldn't have been PCI 
>>> aware and would access the ISA ioports directly. From memory the OF PCI 
>>> specification has mention of workarounds such as mapping the old VGA memory 
>>> to PCI MMIO space for compatibility reasons, so I'd be surprised if there 
>>> wasn't something similar for IDE.
>>> 
>>> The wording above is a bit ambiguous because I can see the above statements 
>>> would be true if the PCI-IDE device were already switched to PCI mode, and 
>>> what we're looking for is whether a switch between the two is supported or 
>>> possible.
>> 
>> A switch is definitely impossible: The PIIX IDE function has the 0x3c 
>> (interrupt line) and 0x3d (interrupt pin) registers reserved. It's fixed in 
>> legacy mode (prog-if is 0x80).
>> 
>> Best regards,
>> Bernhard
>
>Thanks for checking this.

Thanks for the valuable pointer... interesting and very comprehensive read!

>If you have any upcoming patches that touch this file, it's worth adding a 
>comment explaining this with a reference to the relevant part of the 
>datasheet, and also update wmask accordingly.

Will do -- if I ever get to it...

Phil, you have an alternate series. Would you be able to do this, too?

Thanks,
Bernhard

>
>
>ATB,
>
>Mark.