Re: [PATCH 09/13] hw/ide/piix: Disuse isa_get_irq()

[Bernhard Beschow]

Am 26. April 2023 11:33:40 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> isa_get_irq() asks for an ISADevice which piix-ide doesn't provide.
>> Passing a NULL pointer works but causes the isabus global to be used
>> then. By fishing out TYPE_ISA_BUS from the QOM tree it is possible to
>> achieve the same as using isa_get_irq().
>> 
>> This is an alternative solution to commit 9405d87be25d 'hw/ide: Fix
>> crash when plugging a piix3-ide device into the x-remote machine' which
>> allows for cleaning up the ISA API while keeping PIIX IDE functions
>> user-createable.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ide/piix.c | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 6942b484f9..a3a15dc7db 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -104,7 +104,8 @@ static void piix_ide_reset(DeviceState *dev)
>>       pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>>   }
>>   -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>> +static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
>> +                              Error **errp)
>>   {
>>       static const struct {
>>           int iobase;
>> @@ -124,7 +125,8 @@ 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], isa_get_irq(NULL, 
>> port_info[i].isairq));
>> +    ide_bus_init_output_irq(&d->bus[i],
>> +                            isa_bus_get_irq(isa_bus, port_info[i].isairq));
>
>I don't think is the right solution here, since ultimately we want to move the 
>IRQ routing out of the device itself and into the PCI-ISA bridge. I'd go for 
>the same solution as you've done for VIA IDE in patch 2, i.e. update the PIIX 
>interrupt handler to set the legacy_irqs in PCIIDEState, and then wire them up 
>to the ISA IRQs 14 and 15 similar to as Phil as done in his patches:

The problem is user-creatable PIIX-IDE. IMO we should stick to our deprecation 
process before going this step because this will break it.

>
>https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/20230302224058.43315-4-philmd@linaro.org/
>
>https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/20230302224058.43315-5-philmd@linaro.org/
>
>This also reminds me, given that the first patch above is doing wiring in 
>pc_init1() then we are still missing part of your tidy-up series :/
>
>>       bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>       ide_bus_register_restart_cb(&d->bus[i]);
>> @@ -136,14 +138,29 @@ 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;
>>         pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>>         bmdma_init_ops(d, &piix_bmdma_ops);
>>       pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops);
>>   +    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(d)));
>> +        return;
>> +    }
>> +    if (!isa_bus) {
>> +        error_setg(errp, "No ISA bus found while %s requires one",
>> +                   object_get_typename(OBJECT(d)));
>> +        return;
>> +    }
>
>Again I think this should go away with using PCIIDEState's legacy_irqs, since 
>you simply let the board wire them up to the ISABus (or not) as required.

Same here: This breaks user-creatable PIIX-IDE.

>
>>       for (unsigned i = 0; i < 2; i++) {
>> -        if (!pci_piix_init_bus(d, i, errp)) {
>> +        if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
>>               return;
>>           }
>>       }
>
>
>ATB,
>
>Mark.