On 26/01/2023 21:17, 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 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 <shen...@gmail.com>
---
hw/ide/piix.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 267dbf37db..a6646d9657 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -126,7 +126,7 @@ static void piix_ide_reset(DeviceState *dev)
pci_set_byte(pci_conf + 0x20, 0x01); /* BMIBA: 20-23h */
}
-static int pci_piix_init_ports(PCIIDEState *d)
+static int pci_piix_init_ports(PCIIDEState *d, ISABus *isa_bus)
{
static const struct {
int iobase;
@@ -145,7 +145,7 @@ static int pci_piix_init_ports(PCIIDEState *d)
if (ret) {
return ret;
}
- ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+ ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
bmdma_init(&d->bus[i], &d->bmdma[i], d);
d->bmdma[i].bus = &d->bus[i];
@@ -159,6 +159,8 @@ 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;
int rc;
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
@@ -168,7 +170,20 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
- rc = pci_piix_init_ports(d);
+ 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(dev)));
+ return;
+ }
+ if (!isa_bus) {
+ error_setg(errp, "No ISA bus found while %s requires one",
+ object_get_typename(OBJECT(dev)));
+ return;
+ }
+
+ rc = pci_piix_init_ports(d, isa_bus);
if (rc) {
error_setg_errno(errp, -rc, "Failed to realize %s",
object_get_typename(OBJECT(dev)));
I think the approach here to allow the PCI-ISA bridge to locate the ISABus is a good
one, but I think it's worth keeping isa_get_irq() to avoid exposing the internals to
devices.
For me the problem here is that isa_get_irq() accepts a NULL argument for ISADevice.
I'd expect the function to look something like isa_bus_get_irq(ISABus *isa_bus,
unsigned isairq) and then it is the responsibility of the caller to locate and
specify the correct ISABus to avoid falling back to the isabus global.
In particular I can see a PCIDevice should be able to attempt a pci_get_isa_bus() or
similar which would locate the PCI-ISA bridge and return the child ISA bus, which
again is related to the cmd646/via compatibility mode feature.
This is along the lines of the similar approach I discussed in
https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg01443.html which is an
evolution of the ideas discussed in the original thread a couple of years earlier at
https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html.
ATB,
Mark.
