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

[Bernhard Beschow <shentey@gmail.com>]

Am 27. April 2023 12:31:10 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 26/04/2023 19:25, Bernhard Beschow wrote:
>
>> 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.
>
>Thomas posted some links from previous discussions where it seems that this hack is still in use:
>
>https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg00780.html
>https://lists.gnu.org/archive/html/qemu-block/2021-04/msg00746.html
>
>So it seems we can't even deprecate this, as it's working around missing functionality in q35 :(
>
>Certainly it seems that we should add a check that will fail the machine if there is more than one -device piix3-ide on the command line, since I can't see that could ever work properly.
>
>I'm leaning towards adding a device property that must be set to enabled in order for PIIX IDE realize() to succeed, leave it disabled by default and only enable it for the q35 machine. Does that seem like a reasonable solution?

I'd rather declare this to be out of scope of this series. First, this series contains a lot of material already. Second, this patch attempts to preserve current behavior.

This patch is actually a preparation for the next one. In the next patch the (non-obvious) check for presence of the ISABus get removed so we need this patch to preserve behavior. Otherwise machines without an ISA bus will crash if piix3-ide gets user-created. One machine that would crash is the "remote" machine IIRC.

Best regards,
Bernhard

>
>>> 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.