| Message ID | 20230126211740.66874-5-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Resolve isabus global | expand |
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 <shentey@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.
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)));
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 <shentey@gmail.com> --- hw/ide/piix.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)