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

[Mark Cave-Ayland]

On 13/05/2023 12:53, Bernhard Beschow wrote:

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

Hmmm. At the moment we seem to have a circular dependency around all the various IDE 
tidy-up series around :/  If we decide that this series should go first then I prefer 
this solution to Phil's proposal which breaks the contract that 
qdev_connect_gpio_out() should always occur after realize.

Phil, how are things looking for your time re: these IDE changes - are you able to 
spend time looking at them?

Alternatively if you are happy for me to pick up the IDE stuff, pull everything 
together into a series proposal, and then submit the final PR then I'm happy to do 
that too.

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