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

[Mark Cave-Ayland]

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?

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