| Message ID | 20230208000743.79415-6-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ide: Untangle ISA/PCI abuses of ide_init_ioport() | expand |
On 8/2/23 01:07, Philippe Mathieu-Daudé 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)); BTW this portio rework series is the first step. The second part will take care of isa_get_irq(NULL).
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)); > > bmdma_init(&d->bus[i], &d->bmdma[i], d); > -- > 2.38.1 > > This patch essentially circumvents the mitigations introduced by 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": `qemu-system-x86_64 -M x-remote -device piix3-ide` now crashes. This has been considered in https://lore.kernel.org/qemu-devel/20230126211740.66874-1-shentey@gmail.com/ -- see cover letter there. TBH it's not entirely clear to me why we need two competing series here at all. Best regards, Bernhard
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. 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" >> >> bmdma_init(&d->bus[i], &d->bmdma[i], d); >> -- >> 2.38.1 >> >> This patch essentially circumvents the mitigations introduced by >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": `qemu-system-x86_64 -M x-remote -device piix3-ide` now crashes. >This has been considered in >https://lore.kernel.org/qemu-devel/20230126211740.66874-1-shentey@gmail.com/ >-- see cover letter there. TBH it's not entirely clear to me why we need >two competing series here at all. > >Best regards, >Bernhard
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: 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"
On 10/02/2023 16:34, Bernhard Beschow wrote:
> 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.
As far as I can tell the solution to QOMified devices finding their parent bus is
easy: turn DeviceState::parent_bus into a QOM link property called "parent-bus" or
similar which accepts TYPE_BUS, and then any object of TYPE_DEVICE can locate its
parent bus using object_property_get_link() with a standardised property name.
I think it may be impossible to completely remove parent bus references, since buses
like PCI currently make use of upwards hierarchy navigation for things like IRQ
mapping and PCI-PCI bridge traversal.
ATB,
Mark.
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)); bmdma_init(&d->bus[i], &d->bmdma[i], d);
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(-)