Re: [PATCH 5/7] hw/ide/piix: Use generic ide_init_ioport()

[Philippe Mathieu-Daudé]

On 10/2/23 17:34, Bernhard Beschow wrote:
> Am 9. Februar 2023 09:04:49 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> > On Wed, Feb 8, 2023 at 1:08 AM Philippe Mathieu-Daudé <philmd@linaro.org>
> > wrote:
> >
> > > TYPE_PIIX3_IDE is a PCI function inheriting from QOM
> > > TYPE_PCI_DEVICE. To be able to call the ISA specific
> > > ide_init_ioport_isa(), we call this function passing
> > > a NULL ISADevice argument. Remove this hack by calling
> > > the recently added generic ide_init_ioport(), which
> > > doesn't expect any ISADevice.
> > >
> > > Inspired-by: Bernhard Beschow <shentey@gmail.com>
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/ide/piix.c | 10 ++++------
> > >   1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > > index a587541bb2..1cd4389611 100644
> > > --- a/hw/ide/piix.c
> > > +++ b/hw/ide/piix.c
> > > @@ -136,15 +136,13 @@ static int pci_piix_init_ports(PCIIDEState *d)
> > >           {0x1f0, 0x3f6, 14},
> > >           {0x170, 0x376, 15},
> > >       };
> > > -    int i, ret;
> > > +    int i;
> > >
> > >       for (i = 0; i < 2; i++) {
> > >           ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> > > -        ret = ide_init_ioport_isa(&d->bus[i], NULL,
> > > -                                  port_info[i].iobase,
> > > port_info[i].iobase2);
> > > -        if (ret) {
> > > -            return ret;
> > > -        }
> > > +        ide_init_ioport(&d->bus[i], OBJECT(d),
> > > +                        pci_address_space_io(PCI_DEVICE(d)),
> > > +                        port_info[i].iobase, port_info[i].iobase2);
> > >           ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
>
> Let me elaborete a bit on what I mean by the patch essentially circumventing 
> the crash fix:
>
> The reason for the crash with the x-remote machine is now caused by 
> isa_get_irq() which also uses the isabus global behind the scenes. So piix-ide 
> needs to be changed in two places to avoid the global usage and hence the crash.
>
> In his crash fix [1], Thomas was lucky: First, ide_init_ioport() didn't return 
> a value before, so adding one didn't cause changes in other device models. 
> Second, ide_init_ioport() is the first call here to access the global, so it 
> could be used to protect the call to isa_get_irq(). Note that isa_get_irq() 
> couldn't be changed in a similar way without affecting all its call sites.
>
> Fixing ide_init_ioport() to not access the global is certainly a step in the 
> right direction, but this means that ide_init_ioport() is now unable to protect 
> the isa_get_irq() call. Since isa_get_irq() can't conveniently protect itself, 
> we either need to avoid it or need another way to achieve that. That's why in 
> my series GPIOs are used for internal devices and  isa_get_irq() plus fishing 
> out the ISA bus for user-created ones.

The points you raised should be resolved by v2:
20230215161641.32663-1-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20230215161641.32663-1-philmd@linaro.org/
I involved more patches, but hopefully the problem got fixed once for
good without any circumvention.

> Fishing out the ISA bus is still a hack IMO, for two reasons: First, IIUC, 
> QOM'ified devices shall only care about its children while looking up one's 
> parent bus violates this rule. Second, using the global machine pointer to scan 
> for the ISA bus just trades one global for another. That's why I'm only doing 
> this for user-created instances. If we deprecated user-created piix IDE devices 
> we could eventually get rid of this hack.
>
> Best regards,
> Bernhard
>
> [1] https://lore.kernel.org/qemu-devel/20210416125256.2039734-1-thuth@redhat.com/ 
> "hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine"