[RFC PATCH 00/10] Resolve isabus global

[RFC PATCH 00/10] Resolve isabus global

[Bernhard Beschow]

This series resolves the global "isabus" variable and is basically a v2 of [1].
Note that the majority of the work consists of fixing ISA API calls in PIIX IDE
which implicitly rely on the usage of the isabus global.

Rather than adding an ISABus pointer in PCIIDEState as in "v1" this series uses
a qemu_irq array which is roughly the approach outlined in [2]. Moreover, this
series considers backwards compatibility for user-created PIIX IDE
"Frankensten" devices by using a temporary hack. This hack can be removed again
once a deprecation period of user-createable PIIX IDE devices is over. This
deprecation wasn't announced yet but now might be a good time.

Testing done:
* `./qemu-system-x86_64 -M x-remote -device piix3-ide` still fails gracefully with
`qemu-system-x86_64: -device piix3-ide: Failed to realize piix3-ide: No such device`
* `make check-avocado` doesn't report errors
* Booting a live image with `./qemu-system-x86_64 -M pc` works
* Booting a MIPS Malta machine [3] works

[1] https://patchew.org/QEMU/20210518215545.1793947-1-philmd@redhat.com/
[2] https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html
[3] https://people.debian.org/~aurel32/qemu/mips/

Bernhard Beschow (10):
  hw/ide/piix: Check for presence of ISABus before using it
  Revert "hw/ide: Fix crash when plugging a piix3-ide device into the
    x-remote machine"
  hw/i386/pc_piix: Allow for setting properties on "piix3-ide" before
    realizing it
  hw/ide/piix: Avoid the isabus global when wiring ISA interrupts for
    internal devices
  hw/isa/isa-bus: assert() if isa_get_irq() gets passed a NULL ISADevice
  hw/ide/ioport: Rename ide_init_ioport() to isa_ide_init_ioport()
  hw/pci/pci: Introduce pci_register_portio_list()
  hw/ide/piix: Use pci_ide_init_ioport() rather than
    isa_ide_init_ioport()
  hw/isa: Resolve unneeded usage of isabus global
  hw/isa/isa-bus: Resolve isabus global

 hw/i386/pc_piix.c         |  6 +++-
 hw/ide/ioport.c           | 30 +++++++++++++-------
 hw/ide/isa.c              |  2 +-
 hw/ide/piix.c             | 59 +++++++++++++++++++++++++++++++--------
 hw/isa/isa-bus.c          | 46 ++++++++++++++----------------
 hw/isa/piix4.c            |  3 ++
 hw/pci/pci.c              | 18 ++++++++++++
 include/hw/ide/internal.h |  3 +-
 include/hw/ide/pci.h      |  2 ++
 include/hw/isa/isa.h      | 15 ++++------
 include/hw/pci/pci.h      | 21 ++++++++++++++
 11 files changed, 147 insertions(+), 58 deletions(-)

-- 
2.36.1

[RFC PATCH 01/10] hw/ide/piix: Check for presence of ISABus before using it

[Bernhard Beschow]

This is an alternative solution to commit
9405d87be25db6dff4d7b5ab48a81bbf6d083e47 '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 for an arbitrarily
long deprecation period.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/piix.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9a9b28078e..e8f3abc4b5 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -136,6 +136,17 @@ static int pci_piix_init_ports(PCIIDEState *d)
     };
     int i, ret;
 
+    {
+        ISABus *isa_bus;
+        bool ambiguous;
+
+        isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS,
+                                                   &ambiguous));
+        if (!isa_bus || ambiguous) {
+            return -ENODEV;
+        }
+    }
+
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
         ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-- 
2.36.1

[RFC PATCH 02/10] Revert "hw/ide: Fix crash when plugging a piix3-ide device into the x-remote machine"

[Bernhard Beschow]

Now that the PIIX IDE device models check for presence of an ISABus before
using it, this fix isn't needed any longer.

This reverts commit 9405d87be25db6dff4d7b5ab48a81bbf6d083e47.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/ioport.c           | 16 ++++++----------
 hw/ide/piix.c             |  9 +++------
 hw/isa/isa-bus.c          | 14 ++++----------
 include/hw/ide/internal.h |  2 +-
 include/hw/isa/isa.h      | 13 +++++--------
 5 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index e6caa537fa..b613ff3bba 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,19 +50,15 @@ static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
-    int ret;
-
     /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
        bridge has been setup properly to always register with ISA.  */
-    ret = isa_register_portio_list(dev, &bus->portio_list,
-                                   iobase, ide_portio_list, bus, "ide");
+    isa_register_portio_list(dev, &bus->portio_list,
+                             iobase, ide_portio_list, bus, "ide");
 
-    if (ret == 0 && iobase2) {
-        ret = isa_register_portio_list(dev, &bus->portio2_list,
-                                       iobase2, ide_portio2_list, bus, "ide");
+    if (iobase2) {
+        isa_register_portio_list(dev, &bus->portio2_list,
+                                 iobase2, ide_portio2_list, bus, "ide");
     }
-
-    return ret;
 }
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index e8f3abc4b5..21777ecc8b 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -134,7 +134,7 @@ static int pci_piix_init_ports(PCIIDEState *d)
         {0x1f0, 0x3f6, 14},
         {0x170, 0x376, 15},
     };
-    int i, ret;
+    int i;
 
     {
         ISABus *isa_bus;
@@ -149,11 +149,8 @@ static int pci_piix_init_ports(PCIIDEState *d)
 
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-        ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                              port_info[i].iobase2);
-        if (ret) {
-            return ret;
-        }
+        ide_init_ioport(&d->bus[i], NULL, 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);
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 1bee1a47f1..0537a9f2c1 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -119,17 +119,13 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
     isa_init_ioport(dev, start);
 }
 
-int isa_register_portio_list(ISADevice *dev,
-                             PortioList *piolist, uint16_t start,
-                             const MemoryRegionPortio *pio_start,
-                             void *opaque, const char *name)
+void isa_register_portio_list(ISADevice *dev,
+                              PortioList *piolist, uint16_t start,
+                              const MemoryRegionPortio *pio_start,
+                              void *opaque, const char *name)
 {
     assert(piolist && !piolist->owner);
 
-    if (!isabus) {
-        return -ENODEV;
-    }
-
     /* START is how we should treat DEV, regardless of the actual
        contents of the portio array.  This is how the old code
        actually handled e.g. the FDC device.  */
@@ -137,8 +133,6 @@ int isa_register_portio_list(ISADevice *dev,
 
     portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
     portio_list_add(piolist, isabus->address_space_io, start);
-
-    return 0;
 }
 
 ISADevice *isa_new(const char *name)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 97e7e59dc5..348e7f2510 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -624,7 +624,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    int chs_trans, Error **errp);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_exit(IDEState *s);
-int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 6c8a8a92cb..8dd2953211 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -114,15 +114,12 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
  * @portio: the ports, sorted by offset.
  * @opaque: passed into the portio callbacks.
  * @name: passed into memory_region_init_io.
- *
- * Returns: 0 on success, negative error code otherwise (e.g. if the
- *          ISA bus is not available)
  */
-int isa_register_portio_list(ISADevice *dev,
-                             PortioList *piolist,
-                             uint16_t start,
-                             const MemoryRegionPortio *portio,
-                             void *opaque, const char *name);
+void isa_register_portio_list(ISADevice *dev,
+                              PortioList *piolist,
+                              uint16_t start,
+                              const MemoryRegionPortio *portio,
+                              void *opaque, const char *name);
 
 static inline ISABus *isa_bus_from_device(ISADevice *d)
 {
-- 
2.36.1

[RFC PATCH 03/10] hw/i386/pc_piix: Allow for setting properties on "piix3-ide" before realizing it

[Bernhard Beschow]

The next patch will introduce a quirk for user-created PIIX IDE devices for
backwards compatibility. In order to opt-in to new behavior for builtin
devices a property will need to be set until a deprecation period is over.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 0fc2361ffe..2d146b19c0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -252,7 +252,8 @@ static void pc_init1(MachineState *machine,
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
 
-        dev = pci_create_simple(pci_bus, piix3_devfn + 1, "piix3-ide");
+        dev = pci_new_multifunction(piix3_devfn + 1, false, "piix3-ide");
+        pci_realize_and_unref(dev, pci_bus, &error_fatal);
         pci_ide_create_devs(dev);
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
-- 
2.36.1

[RFC PATCH 04/10] hw/ide/piix: Avoid the isabus global when wiring ISA interrupts for internal devices

[Bernhard Beschow]

isa_get_irq() currently always uses the "isabus" global to get the
desired qemu_irq. In order to resolve this global, we want
isa_get_irq() to determine the ISABus from its *dev parameter using
isa_bus_from_device(). As a preparation, all callers who pass NULL
as *dev need to be resolved which seems to happen in hw/ide/piix only.

This patch roughly implements the solution outlined in https://
lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01707.html.

In oder to address the PIIX IDE functions being user-creatable, (see
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg05655.html) a
backwards compatibility quirk is introduced which can be removed after some
deprecation period. The quirk consists of internal devices to opt into new
behavior where the ISA interrupt wiring is performed by the caller rather
than by the device itself. The opt-in can be performed by:

    qdev_prop_set_bit(DEVICE(dev), "user-created", false);

RFC: qdev_init_gpio_in() seems to expose interrupts internal to the device.
Can this be fixed?

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c    |  3 +++
 hw/ide/piix.c        | 41 +++++++++++++++++++++++++++++++++++------
 hw/isa/piix4.c       |  3 +++
 include/hw/ide/pci.h |  2 ++
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2d146b19c0..e24fbab334 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -253,8 +253,11 @@ static void pc_init1(MachineState *machine,
         PCIDevice *dev;
 
         dev = pci_new_multifunction(piix3_devfn + 1, false, "piix3-ide");
+        qdev_prop_set_bit(DEVICE(dev), "user-created", false);
         pci_realize_and_unref(dev, pci_bus, &error_fatal);
         pci_ide_create_devs(dev);
+        qdev_connect_gpio_out(DEVICE(dev), 0, x86ms->gsi[14]);
+        qdev_connect_gpio_out(DEVICE(dev), 1, x86ms->gsi[15]);
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
         pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 21777ecc8b..fbf2756b47 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -103,6 +103,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
     }
 }
 
+static void piix_ide_set_irq(void *opaque, int n, int level)
+{
+    PCIIDEState *d = opaque;
+
+    qemu_set_irq(d->isa_irqs[n], level);
+}
+
 static void piix_ide_reset(DeviceState *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -129,14 +136,14 @@ static int pci_piix_init_ports(PCIIDEState *d)
     static const struct {
         int iobase;
         int iobase2;
-        int isairq;
     } port_info[] = {
-        {0x1f0, 0x3f6, 14},
-        {0x170, 0x376, 15},
+        {0x1f0, 0x3f6},
+        {0x170, 0x376},
     };
+    DeviceState *dev = DEVICE(d);
     int i;
 
-    {
+    if (d->user_created) {
         ISABus *isa_bus;
         bool ambiguous;
 
@@ -145,13 +152,18 @@ static int pci_piix_init_ports(PCIIDEState *d)
         if (!isa_bus || ambiguous) {
             return -ENODEV;
         }
+
+        d->isa_irqs[0] = isa_bus->irqs[14];
+        d->isa_irqs[1] = isa_bus->irqs[15];
+    } else {
+        qdev_init_gpio_out(dev, d->isa_irqs, 2);
     }
 
     for (i = 0; i < 2; i++) {
-        ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
+        ide_bus_init(&d->bus[i], sizeof(d->bus[i]), dev, i, 2);
         ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
                         port_info[i].iobase2);
-        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+        ide_init2(&d->bus[i], qdev_get_gpio_in(dev, i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
         d->bmdma[i].bus = &d->bus[i];
@@ -181,6 +193,14 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
+static void pci_piix_ide_init(Object *obj)
+{
+    PCIIDEState *d = PCI_IDE(obj);
+    DeviceState *dev = DEVICE(d);
+
+    qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
+}
+
 static void pci_piix_ide_exitfn(PCIDevice *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -192,6 +212,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
     }
 }
 
+static Property piix_ide_properties[] = {
+    DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
@@ -206,11 +231,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
+    device_class_set_props(dc, piix_ide_properties);
 }
 
 static const TypeInfo piix3_ide_info = {
     .name          = "piix3-ide",
     .parent        = TYPE_PCI_IDE,
+    .instance_init = pci_piix_ide_init,
     .class_init    = piix3_ide_class_init,
 };
 
@@ -228,11 +255,13 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->hotpluggable = false;
+    device_class_set_props(dc, piix_ide_properties);
 }
 
 static const TypeInfo piix4_ide_info = {
     .name          = "piix4-ide",
     .parent        = TYPE_PCI_IDE,
+    .instance_init = pci_piix_ide_init,
     .class_init    = piix4_ide_class_init,
 };
 
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 15f344dbb7..900c77cf68 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -251,10 +251,13 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
 
     /* IDE */
     qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
+    qdev_prop_set_bit(DEVICE(&s->ide), "user-created", false);
     if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
         return;
     }
     pci_ide_create_devs(PCI_DEVICE(&s->ide));
+    qdev_connect_gpio_out(DEVICE(&s->ide), 0, s->isa[14]);
+    qdev_connect_gpio_out(DEVICE(&s->ide), 1, s->isa[15]);
 
     /* USB */
     qdev_prop_set_int32(DEVICE(&s->uhci), "addr", dev->devfn + 2);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..7ccbfa0a71 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -49,10 +49,12 @@ struct PCIIDEState {
 
     IDEBus bus[2];
     BMDMAState bmdma[2];
+    qemu_irq isa_irqs[2];
     uint32_t secondary; /* used only for cmd646 */
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
     MemoryRegion data_bar[2];
+    bool user_created;
 };
 
 static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
-- 
2.36.1

[RFC PATCH 05/10] hw/isa/isa-bus: assert() if isa_get_irq() gets passed a NULL ISADevice

[Bernhard Beschow]

Now that all call-sites have been fixed to pass non-NULL ISADevices, we can
assert() on NULL ISADevices to catch regressions.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/isa-bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 0537a9f2c1..9e8b5da027 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -81,9 +81,10 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
  */
 qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
 {
-    assert(!dev || ISA_BUS(qdev_get_parent_bus(DEVICE(dev))) == isabus);
+    assert(dev);
     assert(isairq < ISA_NUM_IRQS);
-    return isabus->irqs[isairq];
+
+    return isa_bus_from_device(dev)->irqs[isairq];
 }
 
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
-- 
2.36.1

[RFC PATCH 06/10] hw/ide/ioport: Rename ide_init_ioport() to isa_ide_init_ioport()

[Bernhard Beschow]

ide_init_ioport() takes an ISADevice* parameter which eventually gets passed
to isa_address_space_io(). Unfortunately, there is no ISADevice in hw/ide/
piix, so NULL gets passed instead. This causes isa_address_space_io() to
resort to using the isabus global - which we want to get rid of.

To resolve this, observe that hw/isa/piix* models pass PCI's IO address
space to ISA which can be used instead. The next patch therefore introduces
pci_ide_init_ioport() which takes a PCIDevice* parameter instead and is
available in hw/ide/piix.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/ioport.c           | 2 +-
 hw/ide/isa.c              | 2 +-
 hw/ide/piix.c             | 4 ++--
 include/hw/ide/internal.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index b613ff3bba..ed1f34f573 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -50,7 +50,7 @@ static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+void isa_ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
     /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
        bridge has been setup properly to always register with ISA.  */
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 8bedbd13f1..79ed33aefa 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -74,7 +74,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
     ISAIDEState *s = ISA_IDE(dev);
 
     ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
-    ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
+    isa_ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
     s->irq = isa_get_irq(isadev, s->isairq);
     ide_init2(&s->bus, s->irq);
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index fbf2756b47..312611c61f 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -161,8 +161,8 @@ static int pci_piix_init_ports(PCIIDEState *d)
 
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), dev, i, 2);
-        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                        port_info[i].iobase2);
+        isa_ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+                            port_info[i].iobase2);
         ide_init2(&d->bus[i], qdev_get_gpio_in(dev, i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 348e7f2510..86ecc04ce4 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -624,7 +624,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    int chs_trans, Error **errp);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_exit(IDEState *s);
-void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void isa_ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
-- 
2.36.1

[RFC PATCH 07/10] hw/pci/pci: Introduce pci_register_portio_list()

[Bernhard Beschow]

pci_ide_init_ioport() and pci_register_portio_list() are introduced which
mirror their ISA counterparts. But rather than asking for an ISADevice, the
functions ask for PCIDevice which can be used in hw/ide/piix which fixes
having to pass a NULL ISADevice which is not avialable there.

Passing NULL as ISADevice to pci_ide_init_ioport() also causes a NULL
ISADevice to be passed to isa_register_ioport(). Currently this function
always uses the isabus global. To fix this, we'll want to determine the
ISABus using isa_bus_from_device(), so no call-site must pass a NULL
ISADevice.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/ioport.c           | 14 ++++++++++++++
 hw/pci/pci.c              | 18 ++++++++++++++++++
 include/hw/ide/internal.h |  1 +
 include/hw/pci/pci.h      | 21 +++++++++++++++++++++
 4 files changed, 54 insertions(+)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index ed1f34f573..69e4fa15d4 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -25,6 +25,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/isa/isa.h"
+#include "hw/pci/pci_bus.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "sysemu/blockdev.h"
@@ -62,3 +63,16 @@ void isa_ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
                                  iobase2, ide_portio2_list, bus, "ide");
     }
 }
+
+void pci_ide_init_ioport(IDEBus *bus, PCIDevice *dev, int iobase, int iobase2)
+{
+    assert(dev);
+
+    pci_register_portio_list(dev, &bus->portio_list,
+                             iobase, ide_portio_list, bus, "ide");
+
+    if (iobase2) {
+        pci_register_portio_list(dev, &bus->portio2_list,
+                                 iobase2, ide_portio2_list, bus, "ide");
+    }
+}
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2f450f6a72..3046dd5477 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1440,6 +1440,24 @@ pcibus_t pci_bar_address(PCIDevice *d,
     return new_addr;
 }
 
+void pci_register_portio_list(PCIDevice *dev,
+                              PortioList *piolist, uint16_t start,
+                              const MemoryRegionPortio *pio_start,
+                              void *opaque, const char *name)
+{
+    PCIBus *bus;
+
+    assert(dev);
+    assert(piolist && !piolist->owner);
+
+    bus = pci_get_bus(dev);
+
+    assert(bus);
+
+    portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
+    portio_list_add(piolist, bus->address_space_io, start);
+}
+
 static void pci_update_mappings(PCIDevice *d)
 {
     PCIIORegion *r;
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 86ecc04ce4..4a375d3c09 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -625,6 +625,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_exit(IDEState *s);
 void isa_ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void pci_ide_init_ioport(IDEBus *bus, PCIDevice *isa, int iobase, int iobase2);
 void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b54b6ef88f..91b479d542 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -522,6 +522,27 @@ void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
 pcibus_t pci_bar_address(PCIDevice *d,
                          int reg, uint8_t type, pcibus_t size);
 
+/**
+ * pci_register_portio_list: Initialize a set of io ports
+ *
+ * Several ISA devices have many dis-joint I/O ports.  Worse, these I/O
+ * ports can be interleaved with I/O ports from other devices.  This
+ * function makes it easy to create multiple MemoryRegions for a single
+ * device and use the legacy portio routines.
+ *
+ * @dev: the PCIDevice against which these are registered
+ * @piolist: the PortioList associated with the io ports
+ * @start: the base I/O port against which the portio->offset is applied.
+ * @portio: the ports, sorted by offset.
+ * @opaque: passed into the portio callbacks.
+ * @name: passed into memory_region_init_io.
+ */
+void pci_register_portio_list(PCIDevice *dev,
+                              PortioList *piolist,
+                              uint16_t start,
+                              const MemoryRegionPortio *portio,
+                              void *opaque, const char *name);
+
 static inline void
 pci_set_byte(uint8_t *config, uint8_t val)
 {
-- 
2.36.1

[RFC PATCH 08/10] hw/ide/piix: Use pci_ide_init_ioport() rather than isa_ide_init_ioport()

[Bernhard Beschow]

This should fix the last caller causing a NULL ISADev to be passed to
isa_register_portio_list() which now allows for disusing the isabus global
there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 312611c61f..087568ecf1 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -161,7 +161,7 @@ static int pci_piix_init_ports(PCIIDEState *d)
 
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), dev, i, 2);
-        isa_ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+        pci_ide_init_ioport(&d->bus[i], PCI_DEVICE(d), port_info[i].iobase,
                             port_info[i].iobase2);
         ide_init2(&d->bus[i], qdev_get_gpio_in(dev, i));
 
-- 
2.36.1

[RFC PATCH 09/10] hw/isa: Resolve unneeded usage of isabus global

[Bernhard Beschow]

Now that all call sites of these functions are fixed to pass non-NULL
ISADevices, the ISABus can be determined from the ISADevice argument.

Patch based on https://lists.nongnu.org/archive/html/qemu-devel/2021-05/
msg05785.html.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/ioport.c      |  4 ++--
 hw/isa/isa-bus.c     | 21 +++++++++++++--------
 include/hw/isa/isa.h |  2 +-
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index 69e4fa15d4..112726e415 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -53,8 +53,8 @@ static const MemoryRegionPortio ide_portio2_list[] = {
 
 void isa_ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
-    /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
-       bridge has been setup properly to always register with ISA.  */
+    assert(dev);
+
     isa_register_portio_list(dev, &bus->portio_list,
                              iobase, ide_portio_list, bus, "ide");
 
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 9e8b5da027..5518db93cd 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -116,6 +116,10 @@ static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 
 void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
+    ISABus *isabus;
+
+    assert(dev);
+    isabus = isa_bus_from_device(dev);
     memory_region_add_subregion(isabus->address_space_io, start, io);
     isa_init_ioport(dev, start);
 }
@@ -125,8 +129,13 @@ void isa_register_portio_list(ISADevice *dev,
                               const MemoryRegionPortio *pio_start,
                               void *opaque, const char *name)
 {
+    ISABus *isabus;
+
+    assert(dev);
     assert(piolist && !piolist->owner);
 
+    isabus = isa_bus_from_device(dev);
+
     /* START is how we should treat DEV, regardless of the actual
        contents of the portio array.  This is how the old code
        actually handled e.g. the FDC device.  */
@@ -246,20 +255,16 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
 
 MemoryRegion *isa_address_space(ISADevice *dev)
 {
-    if (dev) {
-        return isa_bus_from_device(dev)->address_space;
-    }
+    assert(dev);
 
-    return isabus->address_space;
+    return isa_bus_from_device(dev)->address_space;
 }
 
 MemoryRegion *isa_address_space_io(ISADevice *dev)
 {
-    if (dev) {
-        return isa_bus_from_device(dev)->address_space_io;
-    }
+    assert(dev);
 
-    return isabus->address_space_io;
+    return isa_bus_from_device(dev)->address_space_io;
 }
 
 type_init(isabus_register_types)
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 8dd2953211..486851e7cb 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -108,7 +108,7 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
  * function makes it easy to create multiple MemoryRegions for a single
  * device and use the legacy portio routines.
  *
- * @dev: the ISADevice against which these are registered; may be NULL.
+ * @dev: the ISADevice against which these are registered
  * @piolist: the PortioList associated with the io ports
  * @start: the base I/O port against which the portio->offset is applied.
  * @portio: the ports, sorted by offset.
-- 
2.36.1

[RFC PATCH 10/10] hw/isa/isa-bus: Resolve isabus global

[Bernhard Beschow]

Now that only isa_bus_new() accesses the isabus global it can be removed
assuming that all call sites take care of not passing the same address
spaces twice to different isa_bus_new() invocations.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/isa-bus.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 5518db93cd..783506685d 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -26,8 +26,6 @@
 #include "hw/isa/isa.h"
 #include "hw/acpi/acpi_aml_interface.h"
 
-static ISABus *isabus;
-
 static char *isabus_get_fw_dev_path(DeviceState *dev);
 
 static void isa_bus_class_init(ObjectClass *klass, void *data)
@@ -53,10 +51,8 @@ static const TypeInfo isa_bus_info = {
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
                     MemoryRegion *address_space_io, Error **errp)
 {
-    if (isabus) {
-        error_setg(errp, "Can't create a second ISA bus");
-        return NULL;
-    }
+    ISABus *isabus;
+
     if (!dev) {
         dev = qdev_new("isabus-bridge");
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-- 
2.36.1