[PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Philippe Mathieu-Daudé]

Since v2: rebased

I'm posting this series as it to not block Bernhard's PIIX
cleanup work. I don't have code change planned, but eventually
reword / improve commit descriptions.

Tested commit after commit to be sure it is bisectable. Sadly
this was before Zoltan & Thomas report a problem with commit
bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").

Background thread:
https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/

Philippe Mathieu-Daudé (18):
  hw/ide/piix: Expose output IRQ as properties for late object
    population
  hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function
  hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15
  hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15
  hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa
  hw/ide/piix: Ensure IDE output IRQs are wired at realization
  hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()
  hw/ide: Introduce generic ide_init_ioport()
  hw/ide/piix: Use generic ide_bus_init_ioport()
  hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device
  hw/isa: Simplify isa_address_space[_io]()
  hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()
  exec/ioport: Factor portio_list_register_flush_coalesced() out
  exec/ioport: Factor portio_list_register() out
  hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro
  hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro
  hw/isa/piix: Unify QOM type name of PIIX ISA function
  hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases

 hw/audio/adlib.c              |  4 +--
 hw/display/qxl.c              |  7 ++--
 hw/display/vga.c              |  9 +++--
 hw/dma/i82374.c               |  7 ++--
 hw/i386/pc_piix.c             | 13 +++++---
 hw/ide/ioport.c               | 15 +++++++--
 hw/ide/isa.c                  |  2 +-
 hw/ide/piix.c                 | 54 +++++++++++++++++++++++-------
 hw/isa/isa-bus.c              | 36 ++++++++------------
 hw/isa/piix3.c                | 63 +++++++++++++++--------------------
 hw/isa/piix4.c                | 12 ++++---
 hw/mips/malta.c               |  2 +-
 hw/watchdog/wdt_ib700.c       |  4 +--
 include/exec/ioport.h         | 15 +++++----
 include/hw/ide/internal.h     |  3 +-
 include/hw/ide/isa.h          |  3 ++
 include/hw/ide/piix.h         |  4 +++
 include/hw/isa/isa.h          |  3 +-
 include/hw/southbridge/piix.h | 14 ++++----
 softmmu/ioport.c              | 48 +++++++++++++++++++-------
 softmmu/qdev-monitor.c        |  3 ++
 21 files changed, 190 insertions(+), 131 deletions(-)

-- 
2.38.1

[PATCH v3 01/18] hw/ide/piix: Expose output IRQ as properties for late object population

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c         | 14 ++++++++++++--
 include/hw/ide/piix.h |  4 ++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 41d60921e3..a36dac8469 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -121,6 +121,13 @@ static void piix_ide_reset(DeviceState *dev)
     pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
 }
 
+static void piix_ide_initfn(Object *obj)
+{
+    PCIIDEState *dev = PCI_IDE(obj);
+
+    qdev_init_gpio_out_named(DEVICE(obj), dev->isa_irq, "ide-irq", 2);
+}
+
 static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
 {
     static const struct {
@@ -133,6 +140,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     };
     int ret;
 
+    qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
     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);
@@ -141,7 +149,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
                          object_get_typename(OBJECT(d)), i);
         return false;
     }
-    ide_bus_init_output_irq(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+    ide_bus_init_output_irq(&d->bus[i], irq_out);
 
     bmdma_init(&d->bus[i], &d->bmdma[i], d);
     d->bmdma[i].bus = &d->bus[i];
@@ -162,7 +170,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
 
-    for (unsigned i = 0; i < 2; i++) {
+    for (unsigned i = 0; i < ARRAY_SIZE(d->isa_irq); i++) {
         if (!pci_piix_init_bus(d, i, errp)) {
             return;
         }
@@ -199,6 +207,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
 static const TypeInfo piix3_ide_info = {
     .name          = TYPE_PIIX3_IDE,
     .parent        = TYPE_PCI_IDE,
+    .instance_init = piix_ide_initfn,
     .class_init    = piix3_ide_class_init,
 };
 
@@ -221,6 +230,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
 static const TypeInfo piix4_ide_info = {
     .name          = TYPE_PIIX4_IDE,
     .parent        = TYPE_PCI_IDE,
+    .instance_init = piix_ide_initfn,
     .class_init    = piix4_ide_class_init,
 };
 
diff --git a/include/hw/ide/piix.h b/include/hw/ide/piix.h
index ef3ef3d62d..533d24d408 100644
--- a/include/hw/ide/piix.h
+++ b/include/hw/ide/piix.h
@@ -1,6 +1,10 @@
 #ifndef HW_IDE_PIIX_H
 #define HW_IDE_PIIX_H
 
+/*
+ * QEMU interface:
+ *  + named GPIO outputs "ide-irq": asserted by each IDE channel
+ */
 #define TYPE_PIIX3_IDE "piix3-ide"
 #define TYPE_PIIX4_IDE "piix4-ide"
 
-- 
2.38.1

[PATCH v3 02/18] hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function

[Philippe Mathieu-Daudé]

In order to allow Frankenstein uses such plugging a PIIX3
IDE function on a ICH9 chipset (which already exposes AHCI
ports...) as:

  $ qemu-system-x86_64 -M q35 -device piix3-ide

add a kludge to automatically wires the IDE IRQs on an ISA
bus exposed by a PCI-to-ISA bridge (usually function #0).
Restrict this kludge to the PIIX3.

Reported-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
TODO: describe why this configuration is broken (multiple
output IRQs wired to the same input IRQ can lead to various
IRQ level changed in the iothread, thus missed by the vCPUs).
---
 hw/ide/piix.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a36dac8469..7cb96ef67f 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -170,6 +170,18 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
 
+    if (!d->isa_irq[0] && !d->isa_irq[1]
+                       && DEVICE_GET_CLASS(d)->user_creatable) {
+        /* kludge specific to TYPE_PIIX3_IDE */
+        Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, NULL);
+
+        if (!isabus) {
+            error_setg(errp, "Unable to find a single ISA bus");
+            return;
+        }
+        d->isa_irq[0] = isa_bus_get_irq(ISA_BUS(isabus), 14);
+        d->isa_irq[1] = isa_bus_get_irq(ISA_BUS(isabus), 15);
+    }
     for (unsigned i = 0; i < ARRAY_SIZE(d->isa_irq); i++) {
         if (!pci_piix_init_bus(d, i, errp)) {
             return;
@@ -202,6 +214,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;
+    /*
+     * This function is part of a Super I/O chip and shouldn't be user
+     * creatable. However QEMU accepts impossible hardware setups such
+     * plugging a PIIX IDE function on a ICH ISA bridge.
+     * Keep this Frankenstein (ab)use working.
+     */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo piix3_ide_info = {
@@ -225,6 +244,8 @@ 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;
+    /* Reason: Part of a Super I/O chip */
+    dc->user_creatable = false;
 }
 
 static const TypeInfo piix4_ide_info = {
-- 
2.38.1

[PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Philippe Mathieu-Daudé]

Since pc_init1() has access to the ISABus*, retrieve the
ISA IRQs with isa_bus_get_irq().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 126b6c11df..1e90b9ff0d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
 
-        dev = pci_create_simple(pci_bus, piix3_devfn + 1, TYPE_PIIX3_IDE);
+        dev = pci_new_multifunction(piix3_devfn + 1, false, TYPE_PIIX3_IDE);
+        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
+                                    isa_bus_get_irq(isa_bus, 14));
+        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
+                                    isa_bus_get_irq(isa_bus, 15));
+        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.38.1

[PATCH v3 04/18] hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15

[Philippe Mathieu-Daudé]

piix4_realize() initialized an array of 16 ISA IRQs in
PIIX4State::isa[], use it to wire the IDE output IRQs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/piix4.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index e0b149f8eb..702b458a3e 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -229,6 +229,8 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
 
     /* IDE */
     qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
+    qdev_connect_gpio_out_named(DEVICE(&s->ide), "ide-irq", 0, s->isa[14]);
+    qdev_connect_gpio_out_named(DEVICE(&s->ide), "ide-irq", 1, s->isa[15]);
     if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
         return;
     }
-- 
2.38.1

[PATCH v3 05/18] hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa

[Philippe Mathieu-Daudé]

Rename ide_init_ioport() as ide_bus_init_ioport_isa() to make
explicit it expects an ISA device. Move the declaration to
"hw/ide/isa.h" where it belongs.

Message-Id: <20230215161641.32663-13-philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ioport.c           | 4 +++-
 hw/ide/isa.c              | 2 +-
 hw/ide/piix.c             | 5 +++--
 include/hw/ide/internal.h | 1 -
 include/hw/ide/isa.h      | 3 +++
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index e2ecc6230c..d869f8018a 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/ide/isa.h"
 #include "hw/ide/internal.h"
 #include "trace.h"
 
@@ -40,7 +41,8 @@ static const MemoryRegionPortio ide_portio2_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
-int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
+int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
+                            int iobase, int iobase2)
 {
     int ret;
 
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 95053e026f..6eed16bf87 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -71,7 +71,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);
+    ide_bus_init_ioport_isa(&s->bus, isadev, s->iobase, s->iobase2);
     ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum));
     vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
     ide_bus_register_restart_cb(&s->bus);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 7cb96ef67f..cb527553e2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -33,6 +33,7 @@
 #include "hw/pci/pci.h"
 #include "hw/ide/piix.h"
 #include "hw/ide/pci.h"
+#include "hw/ide/isa.h"
 #include "trace.h"
 
 static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
@@ -142,8 +143,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
 
     qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
     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);
+    ret = ide_bus_init_ioport_isa(&d->bus[i], NULL, port_info[i].iobase,
+                                  port_info[i].iobase2);
     if (ret) {
         error_setg_errno(errp, -ret, "Failed to realize %s port %u",
                          object_get_typename(OBJECT(d)), i);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d9f1f77dd5..d3b7fdc504 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -618,7 +618,6 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    int chs_trans, Error **errp);
 void ide_exit(IDEState *s);
 void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
-int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 void ide_bus_set_irq(IDEBus *bus);
 void ide_bus_register_restart_cb(IDEBus *bus);
 
diff --git a/include/hw/ide/isa.h b/include/hw/ide/isa.h
index 1cd0ff1fa6..7f7a850265 100644
--- a/include/hw/ide/isa.h
+++ b/include/hw/ide/isa.h
@@ -10,11 +10,14 @@
 #define HW_IDE_ISA_H
 
 #include "qom/object.h"
+#include "hw/ide/internal.h"
 
 #define TYPE_ISA_IDE "isa-ide"
 OBJECT_DECLARE_SIMPLE_TYPE(ISAIDEState, ISA_IDE)
 
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int irqnum,
                         DriveInfo *hd0, DriveInfo *hd1);
+int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *isa,
+                            int iobase, int iobase2);
 
 #endif
-- 
2.38.1

[PATCH v3 06/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Philippe Mathieu-Daudé]

Ensure both IDE output IRQ lines are wired.

We can remove the last use of isa_get_irq(NULL).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/piix.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index cb527553e2..91424e5249 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -134,14 +134,17 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     static const struct {
         int iobase;
         int iobase2;
-        int isairq;
     } port_info[] = {
-        {0x1f0, 0x3f6, 14},
-        {0x170, 0x376, 15},
+        {0x1f0, 0x3f6},
+        {0x170, 0x376},
     };
     int ret;
 
-    qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
+    if (!d->isa_irq[i]) {
+        error_setg(errp, "output IDE IRQ %u not connected", i);
+        return false;
+    }
+
     ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
     ret = ide_bus_init_ioport_isa(&d->bus[i], NULL, port_info[i].iobase,
                                   port_info[i].iobase2);
@@ -150,7 +153,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
                          object_get_typename(OBJECT(d)), i);
         return false;
     }
-    ide_bus_init_output_irq(&d->bus[i], irq_out);
+    ide_bus_init_output_irq(&d->bus[i], d->isa_irq[i]);
 
     bmdma_init(&d->bus[i], &d->bmdma[i], d);
     d->bmdma[i].bus = &d->bus[i];
-- 
2.38.1

[PATCH v3 07/18] hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()

[Philippe Mathieu-Daudé]

Last commit removed the last use of isa_get_irq(NULL).
Add an assertion to ensure we won't use that hack again.
Deprecate in favor of the BUS API: isa_bus_get_irq().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c     | 6 +++---
 include/hw/isa/isa.h | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index a289eccfb1..081bac18ee 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -85,10 +85,10 @@ qemu_irq isa_bus_get_irq(ISABus *bus, unsigned irqnum)
  * This function is only for special cases such as the 'ferr', and
  * temporary use for normal devices until they are converted to qdev.
  */
-qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
+qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum)
 {
-    assert(!dev || ISA_BUS(qdev_get_parent_bus(DEVICE(dev))) == isabus);
-    return isa_bus_get_irq(isabus, isairq);
+    assert(dev);
+    return isa_bus_get_irq(ISA_BUS(qdev_get_parent_bus(DEVICE(dev))), irqnum);
 }
 
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 40d6224a4e..75fb620782 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -87,7 +87,8 @@ ISADevice *isa_create_simple(ISABus *bus, const char *name);
 
 ISADevice *isa_vga_init(ISABus *bus);
 
-qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
+/*  isa_get_irq() is deprecated, please use isa_bus_get_irq() instead. */
+qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum);
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
 MemoryRegion *isa_address_space(ISADevice *dev);
 MemoryRegion *isa_address_space_io(ISADevice *dev);
-- 
2.38.1

[PATCH v3 08/18] hw/ide: Introduce generic ide_init_ioport()

[Philippe Mathieu-Daudé]

Add ide_init_ioport() which is not restricted to the ISA bus.
(Next commit will use it for a PCI device).

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ioport.c           | 12 ++++++++++--
 include/hw/ide/internal.h |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index d869f8018a..ed7957dbae 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -46,8 +46,6 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
 {
     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");
 
@@ -58,3 +56,13 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
 
     return ret;
 }
+
+void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+                         int iobase, int iobase2)
+{
+    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide");
+    portio_list_add(&bus->portio_list, io, iobase);
+
+    portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus, "ide");
+    portio_list_add(&bus->portio_list, io, iobase2);
+}
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d3b7fdc504..6967ca13e0 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -617,6 +617,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    uint32_t cylinders, uint32_t heads, uint32_t secs,
                    int chs_trans, Error **errp);
 void ide_exit(IDEState *s);
+void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
+                         int iobase, int iobase2);
 void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
 void ide_bus_set_irq(IDEBus *bus);
 void ide_bus_register_restart_cb(IDEBus *bus);
-- 
2.38.1

[PATCH v3 09/18] hw/ide/piix: Use generic ide_bus_init_ioport()

[Philippe Mathieu-Daudé]

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 | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 91424e5249..2136895401 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -138,7 +138,6 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
         {0x1f0, 0x3f6},
         {0x170, 0x376},
     };
-    int ret;
 
     if (!d->isa_irq[i]) {
         error_setg(errp, "output IDE IRQ %u not connected", i);
@@ -146,13 +145,9 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     }
 
     ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-    ret = ide_bus_init_ioport_isa(&d->bus[i], NULL, port_info[i].iobase,
-                                  port_info[i].iobase2);
-    if (ret) {
-        error_setg_errno(errp, -ret, "Failed to realize %s port %u",
-                         object_get_typename(OBJECT(d)), i);
-        return false;
-    }
+    ide_bus_init_ioport(&d->bus[i], OBJECT(d),
+                        pci_address_space_io(PCI_DEVICE(d)),
+                        port_info[i].iobase, port_info[i].iobase2);
     ide_bus_init_output_irq(&d->bus[i], d->isa_irq[i]);
 
     bmdma_init(&d->bus[i], &d->bmdma[i], d);
-- 
2.38.1

[PATCH v3 10/18] hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device

[Philippe Mathieu-Daudé]

Previous commit removed the single call to isa_register_portio_list()
with dev=NULL. To be sure we won't reintroduce such weird (ab)use,
assert dev is non-NULL.

We can now calls isa_address_space_io() to get the device I/O region.

Note we can then remove the NULL check in isa_init_ioport() because
it is only called in 2 places (and is static to this file):
- isa_register_ioport() which first calls isa_address_space_io(),
  itself asserting dev is not NULL.
- isa_register_portio_list() which also asserts dev is not NULL
  since the previous commit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 081bac18ee..9c8224afa5 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -113,7 +113,7 @@ IsaDma *isa_bus_get_dma(ISABus *bus, int nchan)
 
 static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 {
-    if (dev && (dev->ioport_id == 0 || ioport < dev->ioport_id)) {
+    if (dev->ioport_id == 0 || ioport < dev->ioport_id) {
         dev->ioport_id = ioport;
     }
 }
@@ -129,6 +129,7 @@ int isa_register_portio_list(ISADevice *dev,
                              const MemoryRegionPortio *pio_start,
                              void *opaque, const char *name)
 {
+    assert(dev);
     assert(piolist && !piolist->owner);
 
     if (!isabus) {
-- 
2.38.1

[PATCH v3 11/18] hw/isa: Simplify isa_address_space[_io]()

[Philippe Mathieu-Daudé]

We don't have any caller passing a NULL device argument,
so we can simplify, avoiding to access the global 'isabus'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/isa-bus.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 9c8224afa5..3036341d3b 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -252,20 +252,14 @@ 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;
-    }
-
-    return isabus->address_space;
+    assert(dev);
+    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;
-    }
-
-    return isabus->address_space_io;
+    assert(dev);
+    return isa_bus_from_device(dev)->address_space_io;
 }
 
 type_init(isabus_register_types)
-- 
2.38.1

[PATCH v3 12/18] hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()

[Philippe Mathieu-Daudé]

Previous commit ensured when entering isa_register_portio_list(),
'dev' is not NULL. Being a TYPE_ISA_DEVICE, the device must sit
on a ISA bus. This means isa_bus_new() as already been called
and 'isabus' can not be NULL.

Simplify by removing the 'isabus' NULL check in
isa_register_portio_list(). 'isabus' is now only used in
isa_bus_new(). Reduce its scope by only declaring it the
function using it (this will allows us to create multiple
ISA buses later).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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 3036341d3b..8e3ca3785e 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -25,8 +25,6 @@
 #include "sysemu/sysemu.h"
 #include "hw/isa/isa.h"
 
-static ISABus *isabus;
-
 static char *isabus_get_fw_dev_path(DeviceState *dev);
 
 static void isa_bus_class_init(ObjectClass *klass, void *data)
@@ -52,6 +50,8 @@ static const TypeInfo isa_bus_info = {
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
                     MemoryRegion *address_space_io, Error **errp)
 {
+    static ISABus *isabus;
+
     if (isabus) {
         error_setg(errp, "Can't create a second ISA bus");
         return NULL;
@@ -132,10 +132,6 @@ int isa_register_portio_list(ISADevice *dev,
     assert(dev);
     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.  */
-- 
2.38.1

[PATCH v3 13/18] exec/ioport: Factor portio_list_register_flush_coalesced() out

[Philippe Mathieu-Daudé]

We always follow the same pattern when registering
coalesced portio:

  - portio_list_init()
  - portio_list_set_flush_coalesced()
  - portio_list_add()

Factor these 3 operations in a single helper named
portio_list_register_flush_coalesced().

Drop portio_list_set_flush_coalesced() which is now
inlined.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230207234615.77300-2-philmd@linaro.org>
---
 hw/display/qxl.c      |  7 +++----
 hw/display/vga.c      |  5 ++---
 include/exec/ioport.h |  5 ++++-
 softmmu/ioport.c      | 27 ++++++++++++++++++++++-----
 4 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..2ecaa0643f 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2224,10 +2224,9 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
     }
     vga_init(vga, OBJECT(dev),
              pci_address_space(dev), pci_address_space_io(dev), false);
-    portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
-                     vga, "vga");
-    portio_list_set_flush_coalesced(&qxl->vga_port_list);
-    portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
+    portio_list_register_flush_coalesced(&qxl->vga_port_list, OBJECT(dev),
+                                         qxl_vga_portio_list, vga, "vga",
+                                         pci_address_space_io(dev), 0x3b0);
     qxl->have_vga = true;
 
     vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 7a5fdff649..98d644922e 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2309,9 +2309,8 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
                                         1);
     memory_region_set_coalescing(vga_io_memory);
     if (init_vga_ports) {
-        portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
-        portio_list_set_flush_coalesced(&s->vga_port_list);
-        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
+        portio_list_register_flush_coalesced(&s->vga_port_list, obj, vga_ports,
+                                             s, "vga", address_space_io, 0x3b0);
     }
     if (vbe_ports) {
         portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index e34f668998..eb9882a3ee 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -65,7 +65,10 @@ typedef struct PortioList {
 void portio_list_init(PortioList *piolist, Object *owner,
                       const struct MemoryRegionPortio *callbacks,
                       void *opaque, const char *name);
-void portio_list_set_flush_coalesced(PortioList *piolist);
+void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
+                                          const MemoryRegionPortio *callbacks,
+                                          void *opaque, const char *name,
+                                          MemoryRegion *mr, uint32_t offset);
 void portio_list_destroy(PortioList *piolist);
 void portio_list_add(PortioList *piolist,
                      struct MemoryRegion *address_space,
diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index cb8adb0b93..be0c920c5c 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -124,6 +124,7 @@ void portio_list_init(PortioList *piolist,
         ++n;
     }
 
+    assert(owner);
     piolist->ports = callbacks;
     piolist->nr = 0;
     piolist->regions = g_new0(MemoryRegion *, n);
@@ -134,11 +135,6 @@ void portio_list_init(PortioList *piolist,
     piolist->flush_coalesced_mmio = false;
 }
 
-void portio_list_set_flush_coalesced(PortioList *piolist)
-{
-    piolist->flush_coalesced_mmio = true;
-}
-
 void portio_list_destroy(PortioList *piolist)
 {
     MemoryRegionPortioList *mrpio;
@@ -297,3 +293,24 @@ void portio_list_del(PortioList *piolist)
         memory_region_del_subregion(piolist->address_space, &mrpio->mr);
     }
 }
+
+static void do_portio_list_register(PortioList *piolist, Object *owner,
+                                    const MemoryRegionPortio *callbacks,
+                                    void *opaque, const char *name,
+                                    MemoryRegion *mr, uint32_t offset,
+                                    bool flush_coalesced_mmio)
+{
+    assert(piolist && !piolist->owner);
+    portio_list_init(piolist, owner, callbacks, opaque, name);
+    piolist->flush_coalesced_mmio = flush_coalesced_mmio;
+    portio_list_add(piolist, mr, offset);
+}
+
+void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
+                                          const MemoryRegionPortio *callbacks,
+                                          void *opaque, const char *name,
+                                          MemoryRegion *mr, uint32_t offset)
+{
+    do_portio_list_register(piolist, owner, callbacks,
+                            opaque, name, mr, offset, true);
+}
-- 
2.38.1

[PATCH v3 14/18] exec/ioport: Factor portio_list_register() out

[Philippe Mathieu-Daudé]

We always follow the same pattern when registering
non-coalesced portio:

  - portio_list_init()
  - portio_list_add()

Factor these 2 operations in a single helper named
portio_list_register(). Since both calls become local
to ioport.c, reduce their scope by declaring them static.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230207234615.77300-3-philmd@linaro.org>
---
 hw/audio/adlib.c        |  4 ++--
 hw/display/vga.c        |  4 ++--
 hw/dma/i82374.c         |  7 +++----
 hw/ide/ioport.c         |  9 ++++-----
 hw/isa/isa-bus.c        |  5 ++---
 hw/watchdog/wdt_ib700.c |  4 ++--
 include/exec/ioport.h   | 10 ++++------
 softmmu/ioport.c        | 21 ++++++++++++++-------
 8 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
index 5f979b1487..cc03c99306 100644
--- a/hw/audio/adlib.c
+++ b/hw/audio/adlib.c
@@ -291,8 +291,8 @@ static void adlib_realizefn (DeviceState *dev, Error **errp)
 
     adlib_portio_list[0].offset = s->port;
     adlib_portio_list[1].offset = s->port + 8;
-    portio_list_init (&s->port_list, OBJECT(s), adlib_portio_list, s, "adlib");
-    portio_list_add (&s->port_list, isa_address_space_io(&s->parent_obj), 0);
+    portio_list_register(&s->port_list, OBJECT(s), adlib_portio_list, s,
+                         "adlib", isa_address_space_io(&s->parent_obj), 0);
 }
 
 static Property adlib_properties[] = {
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 98d644922e..aa899fddc3 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2313,7 +2313,7 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
                                              s, "vga", address_space_io, 0x3b0);
     }
     if (vbe_ports) {
-        portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
-        portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce);
+        portio_list_register(&s->vbe_port_list, obj, vbe_ports, s,
+                             "vbe", address_space_io, 0x1ce);
     }
 }
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index 63734c22c9..aeca0e8323 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -131,10 +131,9 @@ static void i82374_realize(DeviceState *dev, Error **errp)
     }
     i8257_dma_init(isa_bus, true);
 
-    portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
-                     "i82374");
-    portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
-                    s->iobase);
+    portio_list_register(&s->port_list, OBJECT(s), i82374_portio_list, s,
+                         "i82374", isa_address_space_io(&s->parent_obj),
+                         s->iobase);
 
     memset(s->commands, 0, sizeof(s->commands));
 }
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index ed7957dbae..7a6f29955f 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -60,9 +60,8 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
 void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
                          int iobase, int iobase2)
 {
-    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide");
-    portio_list_add(&bus->portio_list, io, iobase);
-
-    portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus, "ide");
-    portio_list_add(&bus->portio_list, io, iobase2);
+    portio_list_register(&bus->portio_list, owner, ide_portio_list,
+                         bus, "ide", io, iobase);
+    portio_list_register(&bus->portio2_list, owner, ide_portio2_list,
+                         bus, "ide", io, iobase2);
 }
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 8e3ca3785e..087293108e 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -130,15 +130,14 @@ int isa_register_portio_list(ISADevice *dev,
                              void *opaque, const char *name)
 {
     assert(dev);
-    assert(piolist && !piolist->owner);
 
     /* 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.  */
     isa_init_ioport(dev, start);
 
-    portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
-    portio_list_add(piolist, isa_address_space_io(dev), start);
+    portio_list_register(piolist, OBJECT(dev), pio_start, opaque, name,
+                         isa_address_space_io(dev), start);
 
     return 0;
 }
diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
index b116c3a3aa..ac4f0be7d8 100644
--- a/hw/watchdog/wdt_ib700.c
+++ b/hw/watchdog/wdt_ib700.c
@@ -115,8 +115,8 @@ static void wdt_ib700_realize(DeviceState *dev, Error **errp)
 
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ib700_timer_expired, s);
 
-    portio_list_init(&s->port_list, OBJECT(s), wdt_portio_list, s, "ib700");
-    portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
+    portio_list_register(&s->port_list, OBJECT(s), wdt_portio_list, s,
+                         "ib700", isa_address_space_io(&s->parent_obj), 0);
 }
 
 static void wdt_ib700_reset(DeviceState *dev)
diff --git a/include/exec/ioport.h b/include/exec/ioport.h
index eb9882a3ee..ca44f269ea 100644
--- a/include/exec/ioport.h
+++ b/include/exec/ioport.h
@@ -62,17 +62,15 @@ typedef struct PortioList {
     bool flush_coalesced_mmio;
 } PortioList;
 
-void portio_list_init(PortioList *piolist, Object *owner,
-                      const struct MemoryRegionPortio *callbacks,
-                      void *opaque, const char *name);
+void portio_list_register(PortioList *piolist, Object *owner,
+                          const MemoryRegionPortio *callbacks,
+                          void *opaque, const char *name,
+                          MemoryRegion *mr, uint32_t offset);
 void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
                                           const MemoryRegionPortio *callbacks,
                                           void *opaque, const char *name,
                                           MemoryRegion *mr, uint32_t offset);
 void portio_list_destroy(PortioList *piolist);
-void portio_list_add(PortioList *piolist,
-                     struct MemoryRegion *address_space,
-                     uint32_t addr);
 void portio_list_del(PortioList *piolist);
 
 #endif /* IOPORT_H */
diff --git a/softmmu/ioport.c b/softmmu/ioport.c
index be0c920c5c..42d43f8b27 100644
--- a/softmmu/ioport.c
+++ b/softmmu/ioport.c
@@ -113,10 +113,9 @@ uint32_t cpu_inl(uint32_t addr)
     return val;
 }
 
-void portio_list_init(PortioList *piolist,
-                      Object *owner,
-                      const MemoryRegionPortio *callbacks,
-                      void *opaque, const char *name)
+static void portio_list_init(PortioList *piolist, Object *owner,
+                             const MemoryRegionPortio *callbacks,
+                             void *opaque, const char *name)
 {
     unsigned n = 0;
 
@@ -246,9 +245,8 @@ static void portio_list_add_1(PortioList *piolist,
     ++piolist->nr;
 }
 
-void portio_list_add(PortioList *piolist,
-                     MemoryRegion *address_space,
-                     uint32_t start)
+static void portio_list_add(PortioList *piolist, MemoryRegion *address_space,
+                            uint32_t start)
 {
     const MemoryRegionPortio *pio, *pio_start = piolist->ports;
     unsigned int off_low, off_high, off_last, count;
@@ -314,3 +312,12 @@ void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
     do_portio_list_register(piolist, owner, callbacks,
                             opaque, name, mr, offset, true);
 }
+
+void portio_list_register(PortioList *piolist, Object *owner,
+                          const MemoryRegionPortio *callbacks,
+                          void *opaque, const char *name,
+                          MemoryRegion *mr, uint32_t offset)
+{
+    do_portio_list_register(piolist, owner, callbacks,
+                            opaque, name, mr, offset, false);
+}
-- 
2.38.1

[PATCH v3 15/18] hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro

[Philippe Mathieu-Daudé]

Manually convert to OBJECT_DECLARE_SIMPLE_TYPE() macro,
similarly to automatic conversion from commit 8063396bf3
("Use OBJECT_DECLARE_SIMPLE_TYPE when possible").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/southbridge/piix.h | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 0bf48e936d..a58bf13a41 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -29,7 +29,7 @@
 #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
 #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
 
-struct PIIXState {
+struct PIIX3State {
     PCIDevice dev;
 
     /*
@@ -57,14 +57,12 @@ struct PIIXState {
     /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
     MemoryRegion rcr_mem;
 };
-typedef struct PIIXState PIIX3State;
 
 #define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
-DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
-                         TYPE_PIIX3_PCI_DEVICE)
-
 #define TYPE_PIIX3_DEVICE "PIIX3"
 #define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
 #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
 
+OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_PCI_DEVICE)
+
 #endif
-- 
2.38.1

[PATCH v3 16/18] hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro

[Philippe Mathieu-Daudé]

See rationale in commit 38b5d79b2e ("qom: add helper
macro DEFINE_TYPES()").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/piix3.c | 53 +++++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index a9cb39bf21..0ee94a2313 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -346,19 +346,6 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
     adevc->build_dev_aml = build_pci_isa_aml;
 }
 
-static const TypeInfo piix3_pci_type_info = {
-    .name = TYPE_PIIX3_PCI_DEVICE,
-    .parent = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIX3State),
-    .abstract = true,
-    .class_init = pci_piix3_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { TYPE_ACPI_DEV_AML_IF },
-        { },
-    },
-};
-
 static void piix3_realize(PCIDevice *dev, Error **errp)
 {
     ERRP_GUARD();
@@ -382,12 +369,6 @@ static void piix3_class_init(ObjectClass *klass, void *data)
     k->realize = piix3_realize;
 }
 
-static const TypeInfo piix3_info = {
-    .name          = TYPE_PIIX3_DEVICE,
-    .parent        = TYPE_PIIX3_PCI_DEVICE,
-    .class_init    = piix3_class_init,
-};
-
 static void piix3_xen_realize(PCIDevice *dev, Error **errp)
 {
     ERRP_GUARD();
@@ -416,17 +397,27 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
     k->realize = piix3_xen_realize;
 }
 
-static const TypeInfo piix3_xen_info = {
-    .name          = TYPE_PIIX3_XEN_DEVICE,
-    .parent        = TYPE_PIIX3_PCI_DEVICE,
-    .class_init    = piix3_xen_class_init,
+static const TypeInfo piix_isa_types[] = {
+    {
+        .name           = TYPE_PIIX3_PCI_DEVICE,
+        .parent         = TYPE_PCI_DEVICE,
+        .instance_size  = sizeof(PIIX3State),
+        .class_init     = pci_piix3_class_init,
+        .abstract       = true,
+        .interfaces = (InterfaceInfo[]) {
+            { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+            { TYPE_ACPI_DEV_AML_IF },
+            { },
+        },
+    }, {
+        .name           = TYPE_PIIX3_DEVICE,
+        .parent         = TYPE_PIIX3_PCI_DEVICE,
+        .class_init     = piix3_class_init,
+    }, {
+        .name           = TYPE_PIIX3_XEN_DEVICE,
+        .parent         = TYPE_PIIX3_PCI_DEVICE,
+        .class_init     = piix3_xen_class_init,
+    }
 };
 
-static void piix3_register_types(void)
-{
-    type_register_static(&piix3_pci_type_info);
-    type_register_static(&piix3_info);
-    type_register_static(&piix3_xen_info);
-}
-
-type_init(piix3_register_types)
+DEFINE_TYPES(piix_isa_types)
-- 
2.38.1

[PATCH v3 17/18] hw/isa/piix: Unify QOM type name of PIIX ISA function

[Philippe Mathieu-Daudé]

Mechanical change doing:

  $ sed -i -e 's/PIIX4_PCI_DEVICE/PIIX4_ISA/g' $(git grep -l PIIX4_PCI_DEVICE)
  $ sed -i -e 's/PIIX3_XEN_DEVICE/PIIX3_ISA_XEN/g' $(git grep -l PIIX3_XEN_DEVICE)
  $ sed -i -e 's/PIIX3_DEVICE/PIIX3_ISA/g' $(git grep -l PIIX3_DEVICE)
  $ sed -i -e 's/PIIX3_PCI_DEVICE/PIIX_ISA/g' $(git grep -l PIIX3_PCI_DEVICE)

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c             |  5 ++---
 hw/isa/piix3.c                | 20 ++++++++++----------
 hw/isa/piix4.c                | 10 +++++-----
 hw/mips/malta.c               |  2 +-
 include/hw/southbridge/piix.h | 10 +++++-----
 5 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1e90b9ff0d..c887b27009 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -221,8 +221,7 @@ static void pc_init1(MachineState *machine,
     if (pcmc->pci_enabled) {
         PIIX3State *piix3;
         PCIDevice *pci_dev;
-        const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
-                                         : TYPE_PIIX3_DEVICE;
+        const char *type = xen_enabled() ? TYPE_PIIX3_ISA_XEN : TYPE_PIIX3_ISA;
 
         pci_bus = i440fx_init(pci_type,
                               i440fx_host,
@@ -236,7 +235,7 @@ static void pc_init1(MachineState *machine,
         pcms->bus = pci_bus;
 
         pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
-        piix3 = PIIX3_PCI_DEVICE(pci_dev);
+        piix3 = PIIX3_ISA(pci_dev);
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
         isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 0ee94a2313..38e0c269ae 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -112,7 +112,7 @@ static void piix3_write_config(PCIDevice *dev,
 {
     pci_default_write_config(dev, address, val, len);
     if (ranges_overlap(address, len, PIIX_PIRQCA, 4)) {
-        PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+        PIIX3State *piix3 = PIIX3_ISA(dev);
         int pic_irq;
 
         pci_bus_fire_intx_routing_notifier(pci_get_bus(&piix3->dev));
@@ -145,7 +145,7 @@ static void piix3_write_config_xen(PCIDevice *dev,
 
 static void piix3_reset(DeviceState *dev)
 {
-    PIIX3State *d = PIIX3_PCI_DEVICE(dev);
+    PIIX3State *d = PIIX3_ISA(dev);
     uint8_t *pci_conf = d->dev.config;
 
     pci_conf[0x04] = 0x07; /* master, memory and I/O */
@@ -286,7 +286,7 @@ static const MemoryRegionOps rcr_ops = {
 
 static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 {
-    PIIX3State *d = PIIX3_PCI_DEVICE(dev);
+    PIIX3State *d = PIIX3_ISA(dev);
     ISABus *isa_bus;
 
     isa_bus = isa_bus_new(DEVICE(d), pci_address_space(dev),
@@ -349,7 +349,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
 static void piix3_realize(PCIDevice *dev, Error **errp)
 {
     ERRP_GUARD();
-    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PIIX3State *piix3 = PIIX3_ISA(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
     pci_piix3_realize(dev, errp);
@@ -372,7 +372,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
 static void piix3_xen_realize(PCIDevice *dev, Error **errp)
 {
     ERRP_GUARD();
-    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PIIX3State *piix3 = PIIX3_ISA(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
     pci_piix3_realize(dev, errp);
@@ -399,7 +399,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo piix_isa_types[] = {
     {
-        .name           = TYPE_PIIX3_PCI_DEVICE,
+        .name           = TYPE_PIIX_ISA,
         .parent         = TYPE_PCI_DEVICE,
         .instance_size  = sizeof(PIIX3State),
         .class_init     = pci_piix3_class_init,
@@ -410,12 +410,12 @@ static const TypeInfo piix_isa_types[] = {
             { },
         },
     }, {
-        .name           = TYPE_PIIX3_DEVICE,
-        .parent         = TYPE_PIIX3_PCI_DEVICE,
+        .name           = TYPE_PIIX3_ISA,
+        .parent         = TYPE_PIIX_ISA,
         .class_init     = piix3_class_init,
     }, {
-        .name           = TYPE_PIIX3_XEN_DEVICE,
-        .parent         = TYPE_PIIX3_PCI_DEVICE,
+        .name           = TYPE_PIIX3_ISA_XEN,
+        .parent         = TYPE_PIIX_ISA,
         .class_init     = piix3_xen_class_init,
     }
 };
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 702b458a3e..90e19a4c37 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -56,7 +56,7 @@ struct PIIX4State {
     uint8_t rcr;
 };
 
-OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
+OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_ISA)
 
 static void piix4_set_irq(void *opaque, int irq_num, int level)
 {
@@ -81,7 +81,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
 
 static void piix4_isa_reset(DeviceState *dev)
 {
-    PIIX4State *d = PIIX4_PCI_DEVICE(dev);
+    PIIX4State *d = PIIX4_ISA(dev);
     uint8_t *pci_conf = d->dev.config;
 
     pci_conf[0x04] = 0x07; // master, memory and I/O
@@ -186,7 +186,7 @@ static const MemoryRegionOps piix4_rcr_ops = {
 
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
-    PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    PIIX4State *s = PIIX4_ISA(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
     ISABus *isa_bus;
     qemu_irq *i8259_out_irq;
@@ -253,7 +253,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
 
 static void piix4_init(Object *obj)
 {
-    PIIX4State *s = PIIX4_PCI_DEVICE(obj);
+    PIIX4State *s = PIIX4_ISA(obj);
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
@@ -285,7 +285,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo piix4_info = {
-    .name          = TYPE_PIIX4_PCI_DEVICE,
+    .name          = TYPE_PIIX4_ISA,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PIIX4State),
     .instance_init = piix4_init,
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index ec172b111a..5aefeba581 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1255,7 +1255,7 @@ void mips_malta_init(MachineState *machine)
 
     /* Southbridge */
     piix4 = pci_create_simple_multifunction(pci_bus, PIIX4_PCI_DEVFN, true,
-                                            TYPE_PIIX4_PCI_DEVICE);
+                                            TYPE_PIIX4_ISA);
     isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix4), "isa.0"));
 
     dev = DEVICE(object_resolve_path_component(OBJECT(piix4), "ide"));
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index a58bf13a41..71a82ef266 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -58,11 +58,11 @@ struct PIIX3State {
     MemoryRegion rcr_mem;
 };
 
-#define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
-#define TYPE_PIIX3_DEVICE "PIIX3"
-#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
-#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
+#define TYPE_PIIX_ISA       "pci-piix3"
+#define TYPE_PIIX3_ISA      "PIIX3"
+#define TYPE_PIIX3_ISA_XEN  "PIIX3-xen"
+#define TYPE_PIIX4_ISA      "piix4-isa"
 
-OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_PCI_DEVICE)
+OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_ISA)
 
 #endif
-- 
2.38.1

[PATCH v3 18/18] hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases

[Philippe Mathieu-Daudé]

Unify PIIX ISA (PCI function #0) as:

 pci-piix3 -> piix-isa       (abstract base class)
 PIIX3     -> piix3-isa      (PIIX3 implementation)
 PIIX3-xen -> piix3-isa-xen  (PIIX3 implementation with Xen extensions)
 piix4-isa -> piix4-isa      (PIIX4 implementation)

Alias previous names in the QDevAlias table.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/southbridge/piix.h | 6 +++---
 softmmu/qdev-monitor.c        | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 71a82ef266..cce65e8f44 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -58,9 +58,9 @@ struct PIIX3State {
     MemoryRegion rcr_mem;
 };
 
-#define TYPE_PIIX_ISA       "pci-piix3"
-#define TYPE_PIIX3_ISA      "PIIX3"
-#define TYPE_PIIX3_ISA_XEN  "PIIX3-xen"
+#define TYPE_PIIX_ISA       "piix-isa"
+#define TYPE_PIIX3_ISA      "piix3-isa"
+#define TYPE_PIIX3_ISA_XEN  "piix3-isa-xen"
 #define TYPE_PIIX4_ISA      "piix4-isa"
 
 OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_ISA)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index b8d2c4dadd..820e7f52ad 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -72,6 +72,9 @@ static const QDevAlias qdev_alias_table[] = {
     { "ES1370", "es1370" }, /* -soundhw name */
     { "ich9-ahci", "ahci" },
     { "lsi53c895a", "lsi" },
+    { "piix-isa", "pci-piix3" },
+    { "piix3-isa", "PIIX3" },
+    { "piix3-isa-xen", "PIIX3-xen" },
     { "virtio-9p-device", "virtio-9p", QEMU_ARCH_VIRTIO_MMIO },
     { "virtio-9p-ccw", "virtio-9p", QEMU_ARCH_VIRTIO_CCW },
     { "virtio-9p-pci", "virtio-9p", QEMU_ARCH_VIRTIO_PCI },
-- 
2.38.1

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 11:40:40PM +0100, Philippe Mathieu-Daudé wrote:
> Since v2: rebased
> 
> I'm posting this series as it to not block Bernhard's PIIX
> cleanup work. I don't have code change planned, but eventually
> reword / improve commit descriptions.
> 
> Tested commit after commit to be sure it is bisectable. Sadly
> this was before Zoltan & Thomas report a problem with commit
> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
> 
> Background thread:
> https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/


Acked-by: Michael S. Tsirkin <mst@redhat.com>

who's merging this you?

I am unsure about interdependencies between all these patchsets at this
point.

> Philippe Mathieu-Daudé (18):
>   hw/ide/piix: Expose output IRQ as properties for late object
>     population
>   hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function
>   hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15
>   hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15
>   hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa
>   hw/ide/piix: Ensure IDE output IRQs are wired at realization
>   hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()
>   hw/ide: Introduce generic ide_init_ioport()
>   hw/ide/piix: Use generic ide_bus_init_ioport()
>   hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device
>   hw/isa: Simplify isa_address_space[_io]()
>   hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()
>   exec/ioport: Factor portio_list_register_flush_coalesced() out
>   exec/ioport: Factor portio_list_register() out
>   hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro
>   hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro
>   hw/isa/piix: Unify QOM type name of PIIX ISA function
>   hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases
> 
>  hw/audio/adlib.c              |  4 +--
>  hw/display/qxl.c              |  7 ++--
>  hw/display/vga.c              |  9 +++--
>  hw/dma/i82374.c               |  7 ++--
>  hw/i386/pc_piix.c             | 13 +++++---
>  hw/ide/ioport.c               | 15 +++++++--
>  hw/ide/isa.c                  |  2 +-
>  hw/ide/piix.c                 | 54 +++++++++++++++++++++++-------
>  hw/isa/isa-bus.c              | 36 ++++++++------------
>  hw/isa/piix3.c                | 63 +++++++++++++++--------------------
>  hw/isa/piix4.c                | 12 ++++---
>  hw/mips/malta.c               |  2 +-
>  hw/watchdog/wdt_ib700.c       |  4 +--
>  include/exec/ioport.h         | 15 +++++----
>  include/hw/ide/internal.h     |  3 +-
>  include/hw/ide/isa.h          |  3 ++
>  include/hw/ide/piix.h         |  4 +++
>  include/hw/isa/isa.h          |  3 +-
>  include/hw/southbridge/piix.h | 14 ++++----
>  softmmu/ioport.c              | 48 +++++++++++++++++++-------
>  softmmu/qdev-monitor.c        |  3 ++
>  21 files changed, 190 insertions(+), 131 deletions(-)
> 
> -- 
> 2.38.1
> 
> 
> 

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[David Woodhouse]

On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote:
>Since v2: rebased
>
>I'm posting this series as it to not block Bernhard's PIIX
>cleanup work. I don't have code change planned, but eventually
>reword / improve commit descriptions.
>
>Tested commit after commit to be sure it is bisectable. Sadly
>this was before Zoltan & Thomas report a problem with commit
>bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").

However much I stare at the partial revert which fixes it, I just cannot believe that the change could make any difference at all. There's got to be something weird going on there.

I was going to ask if the level mode for the PIT made any difference, but this is the output IRQ from the PIT to the CPU itself so I don't see how it would.

Would like to see a report with tracing from pic_update_irq, the CPU interrupt "handler" and the intermediate IRQ handler. With the intermediate present and without it. To compare the two.

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Mark Cave-Ayland]

On 03/03/2023 06:58, David Woodhouse wrote:

> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote:
>> Since v2: rebased
>>
>> I'm posting this series as it to not block Bernhard's PIIX
>> cleanup work. I don't have code change planned, but eventually
>> reword / improve commit descriptions.
>>
>> Tested commit after commit to be sure it is bisectable. Sadly
>> this was before Zoltan & Thomas report a problem with commit
>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
> 
> However much I stare at the partial revert which fixes it, I just cannot believe that the change could make any difference at all. There's got to be something weird going on there.
> 
> I was going to ask if the level mode for the PIT made any difference, but this is the output IRQ from the PIT to the CPU itself so I don't see how it would.
> 
> Would like to see a report with tracing from pic_update_irq, the CPU interrupt "handler" and the intermediate IRQ handler. With the intermediate present and without it. To compare the two.

I suspect it's related to the removal of the allocation of the qemu_irq: qdev gpios 
work by adding a child IRQ object to the device, so it could be possible that 
something in the gpio internals isn't being updated correctly when the value is 
overwritten directly.

Is the problem picked up when running a binary built with --enable-sanitizers? That's 
normally quite good at detecting this kind of issue.


ATB,

Mark.

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

On Fri, 3 Mar 2023, David Woodhouse wrote:
> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote:
>> Since v2: rebased
>>
>> I'm posting this series as it to not block Bernhard's PIIX
>> cleanup work. I don't have code change planned, but eventually
>> reword / improve commit descriptions.
>>
>> Tested commit after commit to be sure it is bisectable. Sadly
>> this was before Zoltan & Thomas report a problem with commit
>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
>
> However much I stare at the partial revert which fixes it, I just cannot 
> believe that the change could make any difference at all. There's got to 
> be something weird going on there.
>
> I was going to ask if the level mode for the PIT made any difference, 
> but this is the output IRQ from the PIT to the CPU itself so I don't see 
> how it would.

This is independent of the ltim patch and I've found this even before 
you've sent that patch as this is in my v5 series which you've replied to. 
I've found this problem before when I've first written this model back in 
2019 and did not understand why it's needed but as now shown also with the 
prep machine there's some other problem somewhere that makes this 
necessary. As the way we had before works for the last few years reverting 
it is the safest bet now but we can try to find out and clean up 
eventually.

> Would like to see a report with tracing from pic_update_irq, the CPU 
> interrupt "handler" and the intermediate IRQ handler. With the 
> intermediate present and without it. To compare the two.

I'll try to collect such trace when I'll have time but if you want to 
experiment debian 8.11.0 netinstall cd should boot either with -kernel or 
with the -bios pegasos2.rom (type boot cd install/pegasos at the ok prompt 
in that case but the rom needs to be extracted from an updater as it's not 
freely distributable). I think Thomas Huth also reproduced the same with 
prep or 40p firmware after a similar change on that machine now.

In any case it's unrelated to level sensitive mode which is only needed by 
MorphOS on pegasos2 to fix simultaneous interrupts e.g.with sound and USB 
or PCI cards which all share IRQ9 on that machine. Other guests don't even 
enable that ltim bit so it should not affect anything else.

Regards,
BALATON Zoltan

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Philippe Mathieu-Daudé]

On 3/3/23 08:46, Mark Cave-Ayland wrote:
> On 03/03/2023 06:58, David Woodhouse wrote:
> 
>> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" 
>> <philmd@linaro.org> wrote:
>>> Since v2: rebased
>>>
>>> I'm posting this series as it to not block Bernhard's PIIX
>>> cleanup work. I don't have code change planned, but eventually
>>> reword / improve commit descriptions.
>>>
>>> Tested commit after commit to be sure it is bisectable. Sadly
>>> this was before Zoltan & Thomas report a problem with commit
>>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
>>
>> However much I stare at the partial revert which fixes it, I just 
>> cannot believe that the change could make any difference at all. 
>> There's got to be something weird going on there.
>>
>> I was going to ask if the level mode for the PIT made any difference, 
>> but this is the output IRQ from the PIT to the CPU itself so I don't 
>> see how it would.
>>
>> Would like to see a report with tracing from pic_update_irq, the CPU 
>> interrupt "handler" and the intermediate IRQ handler. With the 
>> intermediate present and without it. To compare the two.
> 
> I suspect it's related to the removal of the allocation of the qemu_irq: 
> qdev gpios work by adding a child IRQ object to the device, so it could 
> be possible that something in the gpio internals isn't being updated 
> correctly when the value is overwritten directly.
> 
> Is the problem picked up when running a binary built with 
> --enable-sanitizers? That's normally quite good at detecting this kind 
> of issue.

No ASan warning. However I see (before/after bb98e0f59c):

qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 00)
qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 84)
qemu-system-ppc: pc87312: unsupported device reconfiguration (09 01 84)

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]

On Fri, 3 Mar 2023, Philippe Mathieu-Daudé wrote:
> On 3/3/23 08:46, Mark Cave-Ayland wrote:
>> On 03/03/2023 06:58, David Woodhouse wrote:
>> 
>>> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> 
>>> wrote:
>>>> Since v2: rebased
>>>> 
>>>> I'm posting this series as it to not block Bernhard's PIIX
>>>> cleanup work. I don't have code change planned, but eventually
>>>> reword / improve commit descriptions.
>>>> 
>>>> Tested commit after commit to be sure it is bisectable. Sadly
>>>> this was before Zoltan & Thomas report a problem with commit
>>>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
>>> 
>>> However much I stare at the partial revert which fixes it, I just cannot 
>>> believe that the change could make any difference at all. There's got to 
>>> be something weird going on there.
>>> 
>>> I was going to ask if the level mode for the PIT made any difference, but 
>>> this is the output IRQ from the PIT to the CPU itself so I don't see how 
>>> it would.
>>> 
>>> Would like to see a report with tracing from pic_update_irq, the CPU 
>>> interrupt "handler" and the intermediate IRQ handler. With the 
>>> intermediate present and without it. To compare the two.
>> 
>> I suspect it's related to the removal of the allocation of the qemu_irq: 
>> qdev gpios work by adding a child IRQ object to the device, so it could be 
>> possible that something in the gpio internals isn't being updated correctly 
>> when the value is overwritten directly.
>> 
>> Is the problem picked up when running a binary built with 
>> --enable-sanitizers? That's normally quite good at detecting this kind of 
>> issue.
>
> No ASan warning. However I see (before/after bb98e0f59c):
>
> qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 00)
> qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 84)
> qemu-system-ppc: pc87312: unsupported device reconfiguration (09 01 84)

This does not seem related at all especially if you also see it before 
because we have the same problem in vt82c686 and this error above rather 
looks liek it should be a qemu_log_mask(LOG_UNIMP) as that's all that 
function seems to do where this is printed so looks like it's just 
unimplemented functionality.

Regards,
BALATON Zoltan

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 14109 bytes --]

On Fri, 3 Mar 2023, David Woodhouse wrote:
> Would like to see a report with tracing from pic_update_irq, the CPU 
> interrupt "handler" and the intermediate IRQ handler. With the 
> intermediate present and without it. To compare the two.

Here it is witout revert when it hangs after printing:

0.536| Memory used before SYS_Init: 9MB
0.606|
0.606|
0.606| ABox 1.30 (2.7.2018) © 1999-2022 by Ralph Schmidt, Emmanuel Lesueur, Teemu Suikki, Harry Sintonen
1.257| PCI ATA/ATAPI Driver@2: PIO Mode 4
1.257| PCI ATA/ATAPI Driver@2: UDMA Mode 5

U pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
D pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
M pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
A pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
M pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
o pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
d pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
e pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0
5 pic_update_irq master 1 imr 248 irr 7 padd 0
   pic_update_irq master 1 imr 248 irr 7 padd 0

pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0

pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
[more of the above repeating some more then]
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 16 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 144 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 144 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 0 imr 45 irr 144 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0
pic_update_irq master 1 imr 248 irr 7 padd 0

and there seems to be no more interrupts
with the revert when it boots this should print:

1.513| PCI ATA/ATAPI Driver@2: PIO Mode 4
1.514| PCI ATA/ATAPI Driver@2: UDMA Mode 5
1.517| ide.device@2: QEMU     QEMU DVD-ROM     <CDROM>
1.523| ide.device@2:  CDRom <CD001>,<MORPHOS > found, bootable
1.525| ide.device@2:  Mount <CD0>
1.526| ide.device@2:  Partition <CD0> DosType 0x43444653 BootPri 127

and the trace including the irq forwarder func in vt82c686 isa:

M via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
o via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
d via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
e via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
5 via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0

  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0

  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1

  via_isa_request_i8259_irq: irq=0 level=1
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
1 via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
. via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
2 via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
8 via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
2 via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
| via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
i via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
d via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
e via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
. via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
d via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
e via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
v via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
i via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
c via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
e via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
@ via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
2 via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
: via_isa_request_i8259_irq: irq=0 level=0
[...]
< via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
C via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
D via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
R via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
O via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
M via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0
> via_isa_request_i8259_irq: irq=0 level=0
   via_isa_request_i8259_irq: irq=0 level=0

  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0

  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
pic_update_irq master 0 imr 45 irr 128 padd 0
pic_update_irq master 1 imr 248 irr 4 padd 1
  via_isa_request_i8259_irq: irq=0 level=1
  via_isa_request_i8259_irq: irq=0 level=0
  via_isa_request_i8259_irq: irq=0 level=0

and so on. Neither of the above had your ltim patch applied yet.

Regards,
BALATON Zoltan

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Bernhard Beschow]

Am 3. März 2023 07:46:31 UTC schrieb Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk>:
>On 03/03/2023 06:58, David Woodhouse wrote:
>
>> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote:
>>> Since v2: rebased
>>>
>>> I'm posting this series as it to not block Bernhard's PIIX
>>> cleanup work. I don't have code change planned, but eventually
>>> reword / improve commit descriptions.
>>>
>>> Tested commit after commit to be sure it is bisectable. Sadly
>>> this was before Zoltan & Thomas report a problem with commit
>>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
>>
>> However much I stare at the partial revert which fixes it, I just cannot believe that the change could make any difference at all. There's got to be something weird going on there.
>>
>> I was going to ask if the level mode for the PIT made any difference, but this is the output IRQ from the PIT to the CPU itself so I don't see how it would.
>>
>> Would like to see a report with tracing from pic_update_irq, the CPU interrupt "handler" and the intermediate IRQ handler. With the intermediate present and without it. To compare the two.
>
>I suspect it's related to the removal of the allocation of the qemu_irq: qdev gpios work by adding a child IRQ object to the device, so it could be possible that something in the gpio internals isn't being updated correctly when the value is overwritten directly.

I've just sent a series fixing the issue.

The problem was that cpu_intr gets populated by
qdev_connect_gpio_out() in board code which happens after via's
realize method has been executed. So in via's realize method cpu_intr
is still NULL which causes a NULL qemu_irq to be passed to the i8259.

One way to fix this is to move qdev_connect_gpio_out() in board code
between pci_new_multifunction() and pci_realize_and_unref().

By having an intermediate IRQ handler the problem didn't appear since
the (non-NULL) qemu_irq holding the intermediate handler is passed to
the i8259. The intermediate handler delays reading cpu_intr to
runtime, so initializing it after realize() is no problem. The price,
however, is that an indirection occurs at runtime every time cpu_intr
is triggered.

BTW, the PIC proxy in my PIIX consolidation series attempted to solve
the same problem: The ISABus IRQs need to be already populated in
piix-ide's realize method, otherwise NULL qemu_irqs are used. As long
as piix-ide is realized in board code, separately from the piix south
bridge, the ISABus IRQs can be populated in between. However, once
piix-ide is realized in the south bridge, the ISA IRQs must be
populated before the south bridge's realize(). The PIC proxy solved
this by introducing intermediate ISA IRQs while the latest incarnation
of the PIIX consolidation series uses the same approach as described
above.

Best regards,
Bernhard
>
>Is the problem picked up when running a binary built with --enable-sanitizers? That's normally quite good at detecting this kind of issue.
>
>
>ATB,
>
>Mark.

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 11:40:40PM +0100, Philippe Mathieu-Daudé wrote:
> Since v2: rebased
> 
> I'm posting this series as it to not block Bernhard's PIIX
> cleanup work. I don't have code change planned, but eventually
> reword / improve commit descriptions.

> Tested commit after commit to be sure it is bisectable. Sadly
> this was before Zoltan & Thomas report a problem with commit
> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").

I'm not sure what this implies, or how do you want to
resolve the conflicts with Bernhard's work.

did my best to review, series:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> 
> Background thread:
> https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/
> 
> Philippe Mathieu-Daudé (18):
>   hw/ide/piix: Expose output IRQ as properties for late object
>     population
>   hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function
>   hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15
>   hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15
>   hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa
>   hw/ide/piix: Ensure IDE output IRQs are wired at realization
>   hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()
>   hw/ide: Introduce generic ide_init_ioport()
>   hw/ide/piix: Use generic ide_bus_init_ioport()
>   hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device
>   hw/isa: Simplify isa_address_space[_io]()
>   hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()
>   exec/ioport: Factor portio_list_register_flush_coalesced() out
>   exec/ioport: Factor portio_list_register() out
>   hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro
>   hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro
>   hw/isa/piix: Unify QOM type name of PIIX ISA function
>   hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases
> 
>  hw/audio/adlib.c              |  4 +--
>  hw/display/qxl.c              |  7 ++--
>  hw/display/vga.c              |  9 +++--
>  hw/dma/i82374.c               |  7 ++--
>  hw/i386/pc_piix.c             | 13 +++++---
>  hw/ide/ioport.c               | 15 +++++++--
>  hw/ide/isa.c                  |  2 +-
>  hw/ide/piix.c                 | 54 +++++++++++++++++++++++-------
>  hw/isa/isa-bus.c              | 36 ++++++++------------
>  hw/isa/piix3.c                | 63 +++++++++++++++--------------------
>  hw/isa/piix4.c                | 12 ++++---
>  hw/mips/malta.c               |  2 +-
>  hw/watchdog/wdt_ib700.c       |  4 +--
>  include/exec/ioport.h         | 15 +++++----
>  include/hw/ide/internal.h     |  3 +-
>  include/hw/ide/isa.h          |  3 ++
>  include/hw/ide/piix.h         |  4 +++
>  include/hw/isa/isa.h          |  3 +-
>  include/hw/southbridge/piix.h | 14 ++++----
>  softmmu/ioport.c              | 48 +++++++++++++++++++-------
>  softmmu/qdev-monitor.c        |  3 ++
>  21 files changed, 190 insertions(+), 131 deletions(-)
> 
> -- 
> 2.38.1
> 
> 
> 

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Bernhard Beschow]

Am 2. März 2023 22:40:40 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Since v2: rebased
>
>I'm posting this series as it to not block Bernhard's PIIX
>cleanup work. I don't have code change planned, but eventually
>reword / improve commit descriptions.
>
>Tested commit after commit to be sure it is bisectable. Sadly
>this was before Zoltan & Thomas report a problem with commit
>bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
>
>Background thread:
>https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/

Hi,

I've just sent yet another proposal which might make some renamings done in this series appear unneccessary.

Best regards,
Bernhard

>
>Philippe Mathieu-Daudé (18):
>  hw/ide/piix: Expose output IRQ as properties for late object
>    population
>  hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function
>  hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15
>  hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15
>  hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa
>  hw/ide/piix: Ensure IDE output IRQs are wired at realization
>  hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()
>  hw/ide: Introduce generic ide_init_ioport()
>  hw/ide/piix: Use generic ide_bus_init_ioport()
>  hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device
>  hw/isa: Simplify isa_address_space[_io]()
>  hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()
>  exec/ioport: Factor portio_list_register_flush_coalesced() out
>  exec/ioport: Factor portio_list_register() out
>  hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro
>  hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro
>  hw/isa/piix: Unify QOM type name of PIIX ISA function
>  hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases
>
> hw/audio/adlib.c              |  4 +--
> hw/display/qxl.c              |  7 ++--
> hw/display/vga.c              |  9 +++--
> hw/dma/i82374.c               |  7 ++--
> hw/i386/pc_piix.c             | 13 +++++---
> hw/ide/ioport.c               | 15 +++++++--
> hw/ide/isa.c                  |  2 +-
> hw/ide/piix.c                 | 54 +++++++++++++++++++++++-------
> hw/isa/isa-bus.c              | 36 ++++++++------------
> hw/isa/piix3.c                | 63 +++++++++++++++--------------------
> hw/isa/piix4.c                | 12 ++++---
> hw/mips/malta.c               |  2 +-
> hw/watchdog/wdt_ib700.c       |  4 +--
> include/exec/ioport.h         | 15 +++++----
> include/hw/ide/internal.h     |  3 +-
> include/hw/ide/isa.h          |  3 ++
> include/hw/ide/piix.h         |  4 +++
> include/hw/isa/isa.h          |  3 +-
> include/hw/southbridge/piix.h | 14 ++++----
> softmmu/ioport.c              | 48 +++++++++++++++++++-------
> softmmu/qdev-monitor.c        |  3 ++
> 21 files changed, 190 insertions(+), 131 deletions(-)
>

Re: [PATCH v3 01/18] hw/ide/piix: Expose output IRQ as properties for late object population

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ide/piix.c         | 14 ++++++++++++--
>   include/hw/ide/piix.h |  4 ++++
>   2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 41d60921e3..a36dac8469 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -121,6 +121,13 @@ static void piix_ide_reset(DeviceState *dev)
>       pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
>   }
>   
> +static void piix_ide_initfn(Object *obj)
> +{
> +    PCIIDEState *dev = PCI_IDE(obj);
> +
> +    qdev_init_gpio_out_named(DEVICE(obj), dev->isa_irq, "ide-irq", 2);
> +}
> +
>   static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>   {
>       static const struct {
> @@ -133,6 +140,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>       };
>       int ret;
>   
> +    qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
>       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);
> @@ -141,7 +149,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>                            object_get_typename(OBJECT(d)), i);
>           return false;
>       }
> -    ide_bus_init_output_irq(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
> +    ide_bus_init_output_irq(&d->bus[i], irq_out);
>   
>       bmdma_init(&d->bus[i], &d->bmdma[i], d);
>       d->bmdma[i].bus = &d->bus[i];
> @@ -162,7 +170,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>   
>       vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>   
> -    for (unsigned i = 0; i < 2; i++) {
> +    for (unsigned i = 0; i < ARRAY_SIZE(d->isa_irq); i++) {
>           if (!pci_piix_init_bus(d, i, errp)) {
>               return;
>           }
> @@ -199,6 +207,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
>   static const TypeInfo piix3_ide_info = {
>       .name          = TYPE_PIIX3_IDE,
>       .parent        = TYPE_PCI_IDE,
> +    .instance_init = piix_ide_initfn,
>       .class_init    = piix3_ide_class_init,
>   };
>   
> @@ -221,6 +230,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>   static const TypeInfo piix4_ide_info = {
>       .name          = TYPE_PIIX4_IDE,
>       .parent        = TYPE_PCI_IDE,
> +    .instance_init = piix_ide_initfn,
>       .class_init    = piix4_ide_class_init,
>   };
>   
> diff --git a/include/hw/ide/piix.h b/include/hw/ide/piix.h
> index ef3ef3d62d..533d24d408 100644
> --- a/include/hw/ide/piix.h
> +++ b/include/hw/ide/piix.h
> @@ -1,6 +1,10 @@
>   #ifndef HW_IDE_PIIX_H
>   #define HW_IDE_PIIX_H
>   
> +/*
> + * QEMU interface:
> + *  + named GPIO outputs "ide-irq": asserted by each IDE channel
> + */
>   #define TYPE_PIIX3_IDE "piix3-ide"
>   #define TYPE_PIIX4_IDE "piix4-ide"

Comparing this with Bernhard's latest series, Bernhard's patch at 
https://patchew.org/QEMU/20230422150728.176512-1-shentey@gmail.com/20230422150728.176512-2-shentey@gmail.com/ 
(with a small change) is the version we should use, since legacy IRQs are a feature 
of all PCI IDE controllers and not just the PIIX controllers.

If we do it this way then it is possible for all PCI IDE controllers to share the 
same logic for BDMA and legacy/native mode switching moving forward: if a PCI IDE 
controller doesn't implement legacy IRQ routing then the board can leave the IRQs 
disconnected.


ATB,

Mark.

Re: [PATCH v3 02/18] hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> In order to allow Frankenstein uses such plugging a PIIX3
> IDE function on a ICH9 chipset (which already exposes AHCI
> ports...) as:
> 
>    $ qemu-system-x86_64 -M q35 -device piix3-ide
> 
> add a kludge to automatically wires the IDE IRQs on an ISA
> bus exposed by a PCI-to-ISA bridge (usually function #0).
> Restrict this kludge to the PIIX3.
> 
> Reported-by: Bernhard Beschow <shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> TODO: describe why this configuration is broken (multiple
> output IRQs wired to the same input IRQ can lead to various
> IRQ level changed in the iothread, thus missed by the vCPUs).
> ---
>   hw/ide/piix.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index a36dac8469..7cb96ef67f 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -170,6 +170,18 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>   
>       vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
>   
> +    if (!d->isa_irq[0] && !d->isa_irq[1]
> +                       && DEVICE_GET_CLASS(d)->user_creatable) {
> +        /* kludge specific to TYPE_PIIX3_IDE */
> +        Object *isabus = object_resolve_path_type("", TYPE_ISA_BUS, NULL);
> +
> +        if (!isabus) {
> +            error_setg(errp, "Unable to find a single ISA bus");
> +            return;
> +        }
> +        d->isa_irq[0] = isa_bus_get_irq(ISA_BUS(isabus), 14);
> +        d->isa_irq[1] = isa_bus_get_irq(ISA_BUS(isabus), 15);
> +    }
>       for (unsigned i = 0; i < ARRAY_SIZE(d->isa_irq); i++) {
>           if (!pci_piix_init_bus(d, i, errp)) {
>               return;
> @@ -202,6 +214,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;
> +    /*
> +     * This function is part of a Super I/O chip and shouldn't be user
> +     * creatable. However QEMU accepts impossible hardware setups such
> +     * plugging a PIIX IDE function on a ICH ISA bridge.
> +     * Keep this Frankenstein (ab)use working.
> +     */
> +    dc->user_creatable = true;
>   }
>   
>   static const TypeInfo piix3_ide_info = {
> @@ -225,6 +244,8 @@ 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;
> +    /* Reason: Part of a Super I/O chip */
> +    dc->user_creatable = false;
>   }
>   
>   static const TypeInfo piix4_ide_info = {

Heh. Do we really want to support this configuration? :O  If PIIX is hard-wired to 
use legacy IRQs then our options are limited, since the device will always require 
separate routing to an ISABus which can only be present in the PIIX's own PCI-ISA 
bridge (i.e. it cannot exist standalone).

Having said that, it should be possible to do this with the VIA IDE since that can 
simply be switched to native mode.


ATB,

Mark.

Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Since pc_init1() has access to the ISABus*, retrieve the
> ISA IRQs with isa_bus_get_irq().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 126b6c11df..1e90b9ff0d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
>       if (pcmc->pci_enabled) {
>           PCIDevice *dev;
>   
> -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, TYPE_PIIX3_IDE);
> +        dev = pci_new_multifunction(piix3_devfn + 1, false, TYPE_PIIX3_IDE);
> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
> +                                    isa_bus_get_irq(isa_bus, 14));
> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
> +                                    isa_bus_get_irq(isa_bus, 15));
> +        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");

Another reason this probably isn't a good idea: you're having to call 
qdev_connect_gpio_*() before realizing the device :(


ATB,

Mark.

Re: [PATCH v3 04/18] hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> piix4_realize() initialized an array of 16 ISA IRQs in
> PIIX4State::isa[], use it to wire the IDE output IRQs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/isa/piix4.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index e0b149f8eb..702b458a3e 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -229,6 +229,8 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>   
>       /* IDE */
>       qdev_prop_set_int32(DEVICE(&s->ide), "addr", dev->devfn + 1);
> +    qdev_connect_gpio_out_named(DEVICE(&s->ide), "ide-irq", 0, s->isa[14]);
> +    qdev_connect_gpio_out_named(DEVICE(&s->ide), "ide-irq", 1, s->isa[15]);
>       if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
>           return;
>       }

The wiring approach looks right, although again the qdev_connect_gpio_out*() should 
be after qdev_realize().


ATB,

Mark.

Re: [PATCH v3 05/18] hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Rename ide_init_ioport() as ide_bus_init_ioport_isa() to make
> explicit it expects an ISA device. Move the declaration to
> "hw/ide/isa.h" where it belongs.
> 
> Message-Id: <20230215161641.32663-13-philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ide/ioport.c           | 4 +++-
>   hw/ide/isa.c              | 2 +-
>   hw/ide/piix.c             | 5 +++--
>   include/hw/ide/internal.h | 1 -
>   include/hw/ide/isa.h      | 3 +++
>   5 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index e2ecc6230c..d869f8018a 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/ide/isa.h"
>   #include "hw/ide/internal.h"
>   #include "trace.h"
>   
> @@ -40,7 +41,8 @@ static const MemoryRegionPortio ide_portio2_list[] = {
>       PORTIO_END_OF_LIST(),
>   };
>   
> -int ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
> +                            int iobase, int iobase2)
>   {
>       int ret;
>   
> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> index 95053e026f..6eed16bf87 100644
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -71,7 +71,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);
> +    ide_bus_init_ioport_isa(&s->bus, isadev, s->iobase, s->iobase2);
>       ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum));
>       vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
>       ide_bus_register_restart_cb(&s->bus);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 7cb96ef67f..cb527553e2 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -33,6 +33,7 @@
>   #include "hw/pci/pci.h"
>   #include "hw/ide/piix.h"
>   #include "hw/ide/pci.h"
> +#include "hw/ide/isa.h"
>   #include "trace.h"
>   
>   static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
> @@ -142,8 +143,8 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>   
>       qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
>       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);
> +    ret = ide_bus_init_ioport_isa(&d->bus[i], NULL, port_info[i].iobase,
> +                                  port_info[i].iobase2);
>       if (ret) {
>           error_setg_errno(errp, -ret, "Failed to realize %s port %u",
>                            object_get_typename(OBJECT(d)), i);
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index d9f1f77dd5..d3b7fdc504 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -618,7 +618,6 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>                      int chs_trans, Error **errp);
>   void ide_exit(IDEState *s);
>   void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
> -int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>   void ide_bus_set_irq(IDEBus *bus);
>   void ide_bus_register_restart_cb(IDEBus *bus);
>   
> diff --git a/include/hw/ide/isa.h b/include/hw/ide/isa.h
> index 1cd0ff1fa6..7f7a850265 100644
> --- a/include/hw/ide/isa.h
> +++ b/include/hw/ide/isa.h
> @@ -10,11 +10,14 @@
>   #define HW_IDE_ISA_H
>   
>   #include "qom/object.h"
> +#include "hw/ide/internal.h"
>   
>   #define TYPE_ISA_IDE "isa-ide"
>   OBJECT_DECLARE_SIMPLE_TYPE(ISAIDEState, ISA_IDE)
>   
>   ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int irqnum,
>                           DriveInfo *hd0, DriveInfo *hd1);
> +int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *isa,
> +                            int iobase, int iobase2);
>   
>   #endif

I have a similar, but opposite patch to this in one of my branches where I have a PCI 
IDE controller that can switch between legacy and native modes :)

 From my perspective the use of ide_init_ioport() in hw/ide/isa.c is the outlier 
here, because that is the only instance that works on ISADevice, all the other uses 
are PCIDevices. Hence I've gone the other way which is to inline the ISA ioport 
initialisation into isa_ide_realizefn(): 
https://github.com/mcayland/qemu/commit/e94b004d259e5831beadface100e6bb41beca92c.


ATB,

Mark.

Re: [PATCH v3 06/18] hw/ide/piix: Ensure IDE output IRQs are wired at realization

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Ensure both IDE output IRQ lines are wired.
> 
> We can remove the last use of isa_get_irq(NULL).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ide/piix.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index cb527553e2..91424e5249 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -134,14 +134,17 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>       static const struct {
>           int iobase;
>           int iobase2;
> -        int isairq;
>       } port_info[] = {
> -        {0x1f0, 0x3f6, 14},
> -        {0x170, 0x376, 15},
> +        {0x1f0, 0x3f6},
> +        {0x170, 0x376},
>       };
>       int ret;
>   
> -    qemu_irq irq_out = d->isa_irq[i] ? : isa_get_irq(NULL, port_info[i].isairq);
> +    if (!d->isa_irq[i]) {
> +        error_setg(errp, "output IDE IRQ %u not connected", i);
> +        return false;
> +    }
> +
>       ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>       ret = ide_bus_init_ioport_isa(&d->bus[i], NULL, port_info[i].iobase,
>                                     port_info[i].iobase2);
> @@ -150,7 +153,7 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
>                            object_get_typename(OBJECT(d)), i);
>           return false;
>       }
> -    ide_bus_init_output_irq(&d->bus[i], irq_out);
> +    ide_bus_init_output_irq(&d->bus[i], d->isa_irq[i]);
>   
>       bmdma_init(&d->bus[i], &d->bmdma[i], d);
>       d->bmdma[i].bus = &d->bus[i];

I'm not sure I agree with this, since an unwired IRQ/gpio is normally a no-op rather 
than an error - do we really want to change this just for one case? Plus wiring with 
legacy_irqs you should already have eliminated isa_get_irq() in the device itself.


ATB,

Mark.

Re: [PATCH v3 07/18] hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Last commit removed the last use of isa_get_irq(NULL).
> Add an assertion to ensure we won't use that hack again.
> Deprecate in favor of the BUS API: isa_bus_get_irq().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/isa/isa-bus.c     | 6 +++---
>   include/hw/isa/isa.h | 3 ++-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index a289eccfb1..081bac18ee 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -85,10 +85,10 @@ qemu_irq isa_bus_get_irq(ISABus *bus, unsigned irqnum)
>    * This function is only for special cases such as the 'ferr', and
>    * temporary use for normal devices until they are converted to qdev.
>    */
> -qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq)
> +qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum)
>   {
> -    assert(!dev || ISA_BUS(qdev_get_parent_bus(DEVICE(dev))) == isabus);
> -    return isa_bus_get_irq(isabus, isairq);
> +    assert(dev);
> +    return isa_bus_get_irq(ISA_BUS(qdev_get_parent_bus(DEVICE(dev))), irqnum);
>   }
>   
>   void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq)
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index 40d6224a4e..75fb620782 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -87,7 +87,8 @@ ISADevice *isa_create_simple(ISABus *bus, const char *name);
>   
>   ISADevice *isa_vga_init(ISABus *bus);
>   
> -qemu_irq isa_get_irq(ISADevice *dev, unsigned isairq);
> +/*  isa_get_irq() is deprecated, please use isa_bus_get_irq() instead. */
> +qemu_irq isa_get_irq(ISADevice *dev, unsigned irqnum);
>   void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, unsigned isairq);
>   MemoryRegion *isa_address_space(ISADevice *dev);
>   MemoryRegion *isa_address_space_io(ISADevice *dev);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 08/18] hw/ide: Introduce generic ide_init_ioport()

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Add ide_init_ioport() which is not restricted to the ISA bus.
> (Next commit will use it for a PCI device).
> 
> Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ide/ioport.c           | 12 ++++++++++--
>   include/hw/ide/internal.h |  2 ++
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index d869f8018a..ed7957dbae 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -46,8 +46,6 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
>   {
>       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");
>   
> @@ -58,3 +56,13 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
>   
>       return ret;
>   }
> +
> +void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
> +                         int iobase, int iobase2)
> +{
> +    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide");
> +    portio_list_add(&bus->portio_list, io, iobase);
> +
> +    portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus, "ide");
> +    portio_list_add(&bus->portio_list, io, iobase2);
> +}
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index d3b7fdc504..6967ca13e0 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -617,6 +617,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>                      uint32_t cylinders, uint32_t heads, uint32_t secs,
>                      int chs_trans, Error **errp);
>   void ide_exit(IDEState *s);
> +void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
> +                         int iobase, int iobase2);
>   void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
>   void ide_bus_set_irq(IDEBus *bus);
>   void ide_bus_register_restart_cb(IDEBus *bus);

Since I took the opposite approach with regard to the ISA IDE ioports, I have a very 
similar patch locally except that as all the remaining users are PCIDevices, I moved 
the logic into a function called pci_ide_register_legacy_ioports() in hw/ide/pci.c 
(see https://github.com/mcayland/qemu/commit/c2aa28fd6306eeeb60b3ec21be48dd9c8841e20b).

Thoughts?


ATB,

Mark.

Re: [PATCH v3 10/18] hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Previous commit removed the single call to isa_register_portio_list()
> with dev=NULL. To be sure we won't reintroduce such weird (ab)use,
> assert dev is non-NULL.
> 
> We can now calls isa_address_space_io() to get the device I/O region.
> 
> Note we can then remove the NULL check in isa_init_ioport() because
> it is only called in 2 places (and is static to this file):
> - isa_register_ioport() which first calls isa_address_space_io(),
>    itself asserting dev is not NULL.
> - isa_register_portio_list() which also asserts dev is not NULL
>    since the previous commit.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/isa/isa-bus.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 081bac18ee..9c8224afa5 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -113,7 +113,7 @@ IsaDma *isa_bus_get_dma(ISABus *bus, int nchan)
>   
>   static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
>   {
> -    if (dev && (dev->ioport_id == 0 || ioport < dev->ioport_id)) {
> +    if (dev->ioport_id == 0 || ioport < dev->ioport_id) {
>           dev->ioport_id = ioport;
>       }
>   }
> @@ -129,6 +129,7 @@ int isa_register_portio_list(ISADevice *dev,
>                                const MemoryRegionPortio *pio_start,
>                                void *opaque, const char *name)
>   {
> +    assert(dev);
>       assert(piolist && !piolist->owner);
>   
>       if (!isabus) {

Yes!

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 11/18] hw/isa: Simplify isa_address_space[_io]()

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> We don't have any caller passing a NULL device argument,
> so we can simplify, avoiding to access the global 'isabus'.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/isa/isa-bus.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 9c8224afa5..3036341d3b 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -252,20 +252,14 @@ 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;
> -    }
> -
> -    return isabus->address_space;
> +    assert(dev);
> +    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;
> -    }
> -
> -    return isabus->address_space_io;
> +    assert(dev);
> +    return isa_bus_from_device(dev)->address_space_io;
>   }
>   
>   type_init(isabus_register_types)

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 12/18] hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Previous commit ensured when entering isa_register_portio_list(),
> 'dev' is not NULL. Being a TYPE_ISA_DEVICE, the device must sit
> on a ISA bus. This means isa_bus_new() as already been called
> and 'isabus' can not be NULL.
> 
> Simplify by removing the 'isabus' NULL check in
> isa_register_portio_list(). 'isabus' is now only used in
> isa_bus_new(). Reduce its scope by only declaring it the
> function using it (this will allows us to create multiple
> ISA buses later).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   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 3036341d3b..8e3ca3785e 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -25,8 +25,6 @@
>   #include "sysemu/sysemu.h"
>   #include "hw/isa/isa.h"
>   
> -static ISABus *isabus;
> -
>   static char *isabus_get_fw_dev_path(DeviceState *dev);
>   
>   static void isa_bus_class_init(ObjectClass *klass, void *data)
> @@ -52,6 +50,8 @@ static const TypeInfo isa_bus_info = {
>   ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
>                       MemoryRegion *address_space_io, Error **errp)
>   {
> +    static ISABus *isabus;
> +
>       if (isabus) {
>           error_setg(errp, "Can't create a second ISA bus");
>           return NULL;
> @@ -132,10 +132,6 @@ int isa_register_portio_list(ISADevice *dev,
>       assert(dev);
>       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.  */

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 13/18] exec/ioport: Factor portio_list_register_flush_coalesced() out

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> We always follow the same pattern when registering
> coalesced portio:
> 
>    - portio_list_init()
>    - portio_list_set_flush_coalesced()
>    - portio_list_add()
> 
> Factor these 3 operations in a single helper named
> portio_list_register_flush_coalesced().
> 
> Drop portio_list_set_flush_coalesced() which is now
> inlined.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20230207234615.77300-2-philmd@linaro.org>
> ---
>   hw/display/qxl.c      |  7 +++----
>   hw/display/vga.c      |  5 ++---
>   include/exec/ioport.h |  5 ++++-
>   softmmu/ioport.c      | 27 ++++++++++++++++++++++-----
>   4 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index ec712d3ca2..2ecaa0643f 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2224,10 +2224,9 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
>       }
>       vga_init(vga, OBJECT(dev),
>                pci_address_space(dev), pci_address_space_io(dev), false);
> -    portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
> -                     vga, "vga");
> -    portio_list_set_flush_coalesced(&qxl->vga_port_list);
> -    portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
> +    portio_list_register_flush_coalesced(&qxl->vga_port_list, OBJECT(dev),
> +                                         qxl_vga_portio_list, vga, "vga",
> +                                         pci_address_space_io(dev), 0x3b0);
>       qxl->have_vga = true;
>   
>       vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 7a5fdff649..98d644922e 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2309,9 +2309,8 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>                                           1);
>       memory_region_set_coalescing(vga_io_memory);
>       if (init_vga_ports) {
> -        portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
> -        portio_list_set_flush_coalesced(&s->vga_port_list);
> -        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
> +        portio_list_register_flush_coalesced(&s->vga_port_list, obj, vga_ports,
> +                                             s, "vga", address_space_io, 0x3b0);
>       }
>       if (vbe_ports) {
>           portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index e34f668998..eb9882a3ee 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -65,7 +65,10 @@ typedef struct PortioList {
>   void portio_list_init(PortioList *piolist, Object *owner,
>                         const struct MemoryRegionPortio *callbacks,
>                         void *opaque, const char *name);
> -void portio_list_set_flush_coalesced(PortioList *piolist);
> +void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
> +                                          const MemoryRegionPortio *callbacks,
> +                                          void *opaque, const char *name,
> +                                          MemoryRegion *mr, uint32_t offset);
>   void portio_list_destroy(PortioList *piolist);
>   void portio_list_add(PortioList *piolist,
>                        struct MemoryRegion *address_space,
> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> index cb8adb0b93..be0c920c5c 100644
> --- a/softmmu/ioport.c
> +++ b/softmmu/ioport.c
> @@ -124,6 +124,7 @@ void portio_list_init(PortioList *piolist,
>           ++n;
>       }
>   
> +    assert(owner);
>       piolist->ports = callbacks;
>       piolist->nr = 0;
>       piolist->regions = g_new0(MemoryRegion *, n);
> @@ -134,11 +135,6 @@ void portio_list_init(PortioList *piolist,
>       piolist->flush_coalesced_mmio = false;
>   }
>   
> -void portio_list_set_flush_coalesced(PortioList *piolist)
> -{
> -    piolist->flush_coalesced_mmio = true;
> -}
> -
>   void portio_list_destroy(PortioList *piolist)
>   {
>       MemoryRegionPortioList *mrpio;
> @@ -297,3 +293,24 @@ void portio_list_del(PortioList *piolist)
>           memory_region_del_subregion(piolist->address_space, &mrpio->mr);
>       }
>   }
> +
> +static void do_portio_list_register(PortioList *piolist, Object *owner,
> +                                    const MemoryRegionPortio *callbacks,
> +                                    void *opaque, const char *name,
> +                                    MemoryRegion *mr, uint32_t offset,
> +                                    bool flush_coalesced_mmio)
> +{
> +    assert(piolist && !piolist->owner);
> +    portio_list_init(piolist, owner, callbacks, opaque, name);
> +    piolist->flush_coalesced_mmio = flush_coalesced_mmio;
> +    portio_list_add(piolist, mr, offset);
> +}
> +
> +void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
> +                                          const MemoryRegionPortio *callbacks,
> +                                          void *opaque, const char *name,
> +                                          MemoryRegion *mr, uint32_t offset)
> +{
> +    do_portio_list_register(piolist, owner, callbacks,
> +                            opaque, name, mr, offset, true);
> +}

Would it be better to have a portio_list_register() that calls portio_list_init() 
followed by portio_list_add() and still keep portio_list_set_flush_coalesced() 
separate? Then it seems that other portio users can benefit in a slight reduction in 
line count given that there only appear to be 2 users of 
portio_list_set_flush_coalesced().


ATB,

Mark.

Re: [PATCH v3 14/18] exec/ioport: Factor portio_list_register() out

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> We always follow the same pattern when registering
> non-coalesced portio:
> 
>    - portio_list_init()
>    - portio_list_add()
> 
> Factor these 2 operations in a single helper named
> portio_list_register(). Since both calls become local
> to ioport.c, reduce their scope by declaring them static.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20230207234615.77300-3-philmd@linaro.org>
> ---
>   hw/audio/adlib.c        |  4 ++--
>   hw/display/vga.c        |  4 ++--
>   hw/dma/i82374.c         |  7 +++----
>   hw/ide/ioport.c         |  9 ++++-----
>   hw/isa/isa-bus.c        |  5 ++---
>   hw/watchdog/wdt_ib700.c |  4 ++--
>   include/exec/ioport.h   | 10 ++++------
>   softmmu/ioport.c        | 21 ++++++++++++++-------
>   8 files changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/audio/adlib.c b/hw/audio/adlib.c
> index 5f979b1487..cc03c99306 100644
> --- a/hw/audio/adlib.c
> +++ b/hw/audio/adlib.c
> @@ -291,8 +291,8 @@ static void adlib_realizefn (DeviceState *dev, Error **errp)
>   
>       adlib_portio_list[0].offset = s->port;
>       adlib_portio_list[1].offset = s->port + 8;
> -    portio_list_init (&s->port_list, OBJECT(s), adlib_portio_list, s, "adlib");
> -    portio_list_add (&s->port_list, isa_address_space_io(&s->parent_obj), 0);
> +    portio_list_register(&s->port_list, OBJECT(s), adlib_portio_list, s,
> +                         "adlib", isa_address_space_io(&s->parent_obj), 0);
>   }
>   
>   static Property adlib_properties[] = {
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 98d644922e..aa899fddc3 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2313,7 +2313,7 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>                                                s, "vga", address_space_io, 0x3b0);
>       }
>       if (vbe_ports) {
> -        portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
> -        portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce);
> +        portio_list_register(&s->vbe_port_list, obj, vbe_ports, s,
> +                             "vbe", address_space_io, 0x1ce);
>       }
>   }
> diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
> index 63734c22c9..aeca0e8323 100644
> --- a/hw/dma/i82374.c
> +++ b/hw/dma/i82374.c
> @@ -131,10 +131,9 @@ static void i82374_realize(DeviceState *dev, Error **errp)
>       }
>       i8257_dma_init(isa_bus, true);
>   
> -    portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
> -                     "i82374");
> -    portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
> -                    s->iobase);
> +    portio_list_register(&s->port_list, OBJECT(s), i82374_portio_list, s,
> +                         "i82374", isa_address_space_io(&s->parent_obj),
> +                         s->iobase);
>   
>       memset(s->commands, 0, sizeof(s->commands));
>   }
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> index ed7957dbae..7a6f29955f 100644
> --- a/hw/ide/ioport.c
> +++ b/hw/ide/ioport.c
> @@ -60,9 +60,8 @@ int ide_bus_init_ioport_isa(IDEBus *bus, ISADevice *dev,
>   void ide_bus_init_ioport(IDEBus *bus, Object *owner, MemoryRegion *io,
>                            int iobase, int iobase2)
>   {
> -    portio_list_init(&bus->portio_list, owner, ide_portio_list, bus, "ide");
> -    portio_list_add(&bus->portio_list, io, iobase);
> -
> -    portio_list_init(&bus->portio2_list, owner, ide_portio2_list, bus, "ide");
> -    portio_list_add(&bus->portio_list, io, iobase2);
> +    portio_list_register(&bus->portio_list, owner, ide_portio_list,
> +                         bus, "ide", io, iobase);
> +    portio_list_register(&bus->portio2_list, owner, ide_portio2_list,
> +                         bus, "ide", io, iobase2);
>   }
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 8e3ca3785e..087293108e 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -130,15 +130,14 @@ int isa_register_portio_list(ISADevice *dev,
>                                void *opaque, const char *name)
>   {
>       assert(dev);
> -    assert(piolist && !piolist->owner);
>   
>       /* 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.  */
>       isa_init_ioport(dev, start);
>   
> -    portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name);
> -    portio_list_add(piolist, isa_address_space_io(dev), start);
> +    portio_list_register(piolist, OBJECT(dev), pio_start, opaque, name,
> +                         isa_address_space_io(dev), start);
>   
>       return 0;
>   }
> diff --git a/hw/watchdog/wdt_ib700.c b/hw/watchdog/wdt_ib700.c
> index b116c3a3aa..ac4f0be7d8 100644
> --- a/hw/watchdog/wdt_ib700.c
> +++ b/hw/watchdog/wdt_ib700.c
> @@ -115,8 +115,8 @@ static void wdt_ib700_realize(DeviceState *dev, Error **errp)
>   
>       s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ib700_timer_expired, s);
>   
> -    portio_list_init(&s->port_list, OBJECT(s), wdt_portio_list, s, "ib700");
> -    portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj), 0);
> +    portio_list_register(&s->port_list, OBJECT(s), wdt_portio_list, s,
> +                         "ib700", isa_address_space_io(&s->parent_obj), 0);
>   }
>   
>   static void wdt_ib700_reset(DeviceState *dev)
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index eb9882a3ee..ca44f269ea 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -62,17 +62,15 @@ typedef struct PortioList {
>       bool flush_coalesced_mmio;
>   } PortioList;
>   
> -void portio_list_init(PortioList *piolist, Object *owner,
> -                      const struct MemoryRegionPortio *callbacks,
> -                      void *opaque, const char *name);
> +void portio_list_register(PortioList *piolist, Object *owner,
> +                          const MemoryRegionPortio *callbacks,
> +                          void *opaque, const char *name,
> +                          MemoryRegion *mr, uint32_t offset);
>   void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
>                                             const MemoryRegionPortio *callbacks,
>                                             void *opaque, const char *name,
>                                             MemoryRegion *mr, uint32_t offset);
>   void portio_list_destroy(PortioList *piolist);
> -void portio_list_add(PortioList *piolist,
> -                     struct MemoryRegion *address_space,
> -                     uint32_t addr);
>   void portio_list_del(PortioList *piolist);
>   
>   #endif /* IOPORT_H */
> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> index be0c920c5c..42d43f8b27 100644
> --- a/softmmu/ioport.c
> +++ b/softmmu/ioport.c
> @@ -113,10 +113,9 @@ uint32_t cpu_inl(uint32_t addr)
>       return val;
>   }
>   
> -void portio_list_init(PortioList *piolist,
> -                      Object *owner,
> -                      const MemoryRegionPortio *callbacks,
> -                      void *opaque, const char *name)
> +static void portio_list_init(PortioList *piolist, Object *owner,
> +                             const MemoryRegionPortio *callbacks,
> +                             void *opaque, const char *name)
>   {
>       unsigned n = 0;
>   
> @@ -246,9 +245,8 @@ static void portio_list_add_1(PortioList *piolist,
>       ++piolist->nr;
>   }
>   
> -void portio_list_add(PortioList *piolist,
> -                     MemoryRegion *address_space,
> -                     uint32_t start)
> +static void portio_list_add(PortioList *piolist, MemoryRegion *address_space,
> +                            uint32_t start)
>   {
>       const MemoryRegionPortio *pio, *pio_start = piolist->ports;
>       unsigned int off_low, off_high, off_last, count;
> @@ -314,3 +312,12 @@ void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
>       do_portio_list_register(piolist, owner, callbacks,
>                               opaque, name, mr, offset, true);
>   }
> +
> +void portio_list_register(PortioList *piolist, Object *owner,
> +                          const MemoryRegionPortio *callbacks,
> +                          void *opaque, const char *name,
> +                          MemoryRegion *mr, uint32_t offset)
> +{
> +    do_portio_list_register(piolist, owner, callbacks,
> +                            opaque, name, mr, offset, false);
> +}

Ah, and you beat me to it :)  I'm not 100% sold on the previous patch, but this makes 
sense so:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 15/18] hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Manually convert to OBJECT_DECLARE_SIMPLE_TYPE() macro,
> similarly to automatic conversion from commit 8063396bf3
> ("Use OBJECT_DECLARE_SIMPLE_TYPE when possible").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/southbridge/piix.h | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 0bf48e936d..a58bf13a41 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -29,7 +29,7 @@
>   #define PIIX_NUM_PIC_IRQS       16      /* i8259 * 2 */
>   #define PIIX_NUM_PIRQS          4ULL    /* PIRQ[A-D] */
>   
> -struct PIIXState {
> +struct PIIX3State {
>       PCIDevice dev;
>   
>       /*
> @@ -57,14 +57,12 @@ struct PIIXState {
>       /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
>       MemoryRegion rcr_mem;
>   };
> -typedef struct PIIXState PIIX3State;
>   
>   #define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
> -DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
> -                         TYPE_PIIX3_PCI_DEVICE)
> -
>   #define TYPE_PIIX3_DEVICE "PIIX3"
>   #define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
>   #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
>   
> +OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_PCI_DEVICE)
> +
>   #endif

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 16/18] hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> See rationale in commit 38b5d79b2e ("qom: add helper
> macro DEFINE_TYPES()").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/isa/piix3.c | 53 +++++++++++++++++++++-----------------------------
>   1 file changed, 22 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index a9cb39bf21..0ee94a2313 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -346,19 +346,6 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
>       adevc->build_dev_aml = build_pci_isa_aml;
>   }
>   
> -static const TypeInfo piix3_pci_type_info = {
> -    .name = TYPE_PIIX3_PCI_DEVICE,
> -    .parent = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PIIX3State),
> -    .abstract = true,
> -    .class_init = pci_piix3_class_init,
> -    .interfaces = (InterfaceInfo[]) {
> -        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> -        { TYPE_ACPI_DEV_AML_IF },
> -        { },
> -    },
> -};
> -
>   static void piix3_realize(PCIDevice *dev, Error **errp)
>   {
>       ERRP_GUARD();
> @@ -382,12 +369,6 @@ static void piix3_class_init(ObjectClass *klass, void *data)
>       k->realize = piix3_realize;
>   }
>   
> -static const TypeInfo piix3_info = {
> -    .name          = TYPE_PIIX3_DEVICE,
> -    .parent        = TYPE_PIIX3_PCI_DEVICE,
> -    .class_init    = piix3_class_init,
> -};
> -
>   static void piix3_xen_realize(PCIDevice *dev, Error **errp)
>   {
>       ERRP_GUARD();
> @@ -416,17 +397,27 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
>       k->realize = piix3_xen_realize;
>   }
>   
> -static const TypeInfo piix3_xen_info = {
> -    .name          = TYPE_PIIX3_XEN_DEVICE,
> -    .parent        = TYPE_PIIX3_PCI_DEVICE,
> -    .class_init    = piix3_xen_class_init,
> +static const TypeInfo piix_isa_types[] = {
> +    {
> +        .name           = TYPE_PIIX3_PCI_DEVICE,
> +        .parent         = TYPE_PCI_DEVICE,
> +        .instance_size  = sizeof(PIIX3State),
> +        .class_init     = pci_piix3_class_init,
> +        .abstract       = true,
> +        .interfaces = (InterfaceInfo[]) {
> +            { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +            { TYPE_ACPI_DEV_AML_IF },
> +            { },
> +        },
> +    }, {
> +        .name           = TYPE_PIIX3_DEVICE,
> +        .parent         = TYPE_PIIX3_PCI_DEVICE,
> +        .class_init     = piix3_class_init,
> +    }, {
> +        .name           = TYPE_PIIX3_XEN_DEVICE,
> +        .parent         = TYPE_PIIX3_PCI_DEVICE,
> +        .class_init     = piix3_xen_class_init,
> +    }
>   };
>   
> -static void piix3_register_types(void)
> -{
> -    type_register_static(&piix3_pci_type_info);
> -    type_register_static(&piix3_info);
> -    type_register_static(&piix3_xen_info);
> -}
> -
> -type_init(piix3_register_types)
> +DEFINE_TYPES(piix_isa_types)

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 17/18] hw/isa/piix: Unify QOM type name of PIIX ISA function

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Mechanical change doing:
> 
>    $ sed -i -e 's/PIIX4_PCI_DEVICE/PIIX4_ISA/g' $(git grep -l PIIX4_PCI_DEVICE)
>    $ sed -i -e 's/PIIX3_XEN_DEVICE/PIIX3_ISA_XEN/g' $(git grep -l PIIX3_XEN_DEVICE)
>    $ sed -i -e 's/PIIX3_DEVICE/PIIX3_ISA/g' $(git grep -l PIIX3_DEVICE)
>    $ sed -i -e 's/PIIX3_PCI_DEVICE/PIIX_ISA/g' $(git grep -l PIIX3_PCI_DEVICE)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c             |  5 ++---
>   hw/isa/piix3.c                | 20 ++++++++++----------
>   hw/isa/piix4.c                | 10 +++++-----
>   hw/mips/malta.c               |  2 +-
>   include/hw/southbridge/piix.h | 10 +++++-----
>   5 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1e90b9ff0d..c887b27009 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -221,8 +221,7 @@ static void pc_init1(MachineState *machine,
>       if (pcmc->pci_enabled) {
>           PIIX3State *piix3;
>           PCIDevice *pci_dev;
> -        const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
> -                                         : TYPE_PIIX3_DEVICE;
> +        const char *type = xen_enabled() ? TYPE_PIIX3_ISA_XEN : TYPE_PIIX3_ISA;
>   
>           pci_bus = i440fx_init(pci_type,
>                                 i440fx_host,
> @@ -236,7 +235,7 @@ static void pc_init1(MachineState *machine,
>           pcms->bus = pci_bus;
>   
>           pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
> -        piix3 = PIIX3_PCI_DEVICE(pci_dev);
> +        piix3 = PIIX3_ISA(pci_dev);
>           piix3->pic = x86ms->gsi;
>           piix3_devfn = piix3->dev.devfn;
>           isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
> index 0ee94a2313..38e0c269ae 100644
> --- a/hw/isa/piix3.c
> +++ b/hw/isa/piix3.c
> @@ -112,7 +112,7 @@ static void piix3_write_config(PCIDevice *dev,
>   {
>       pci_default_write_config(dev, address, val, len);
>       if (ranges_overlap(address, len, PIIX_PIRQCA, 4)) {
> -        PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
> +        PIIX3State *piix3 = PIIX3_ISA(dev);
>           int pic_irq;
>   
>           pci_bus_fire_intx_routing_notifier(pci_get_bus(&piix3->dev));
> @@ -145,7 +145,7 @@ static void piix3_write_config_xen(PCIDevice *dev,
>   
>   static void piix3_reset(DeviceState *dev)
>   {
> -    PIIX3State *d = PIIX3_PCI_DEVICE(dev);
> +    PIIX3State *d = PIIX3_ISA(dev);
>       uint8_t *pci_conf = d->dev.config;
>   
>       pci_conf[0x04] = 0x07; /* master, memory and I/O */
> @@ -286,7 +286,7 @@ static const MemoryRegionOps rcr_ops = {
>   
>   static void pci_piix3_realize(PCIDevice *dev, Error **errp)
>   {
> -    PIIX3State *d = PIIX3_PCI_DEVICE(dev);
> +    PIIX3State *d = PIIX3_ISA(dev);
>       ISABus *isa_bus;
>   
>       isa_bus = isa_bus_new(DEVICE(d), pci_address_space(dev),
> @@ -349,7 +349,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
>   static void piix3_realize(PCIDevice *dev, Error **errp)
>   {
>       ERRP_GUARD();
> -    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
> +    PIIX3State *piix3 = PIIX3_ISA(dev);
>       PCIBus *pci_bus = pci_get_bus(dev);
>   
>       pci_piix3_realize(dev, errp);
> @@ -372,7 +372,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
>   static void piix3_xen_realize(PCIDevice *dev, Error **errp)
>   {
>       ERRP_GUARD();
> -    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
> +    PIIX3State *piix3 = PIIX3_ISA(dev);
>       PCIBus *pci_bus = pci_get_bus(dev);
>   
>       pci_piix3_realize(dev, errp);
> @@ -399,7 +399,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
>   
>   static const TypeInfo piix_isa_types[] = {
>       {
> -        .name           = TYPE_PIIX3_PCI_DEVICE,
> +        .name           = TYPE_PIIX_ISA,
>           .parent         = TYPE_PCI_DEVICE,
>           .instance_size  = sizeof(PIIX3State),
>           .class_init     = pci_piix3_class_init,
> @@ -410,12 +410,12 @@ static const TypeInfo piix_isa_types[] = {
>               { },
>           },
>       }, {
> -        .name           = TYPE_PIIX3_DEVICE,
> -        .parent         = TYPE_PIIX3_PCI_DEVICE,
> +        .name           = TYPE_PIIX3_ISA,
> +        .parent         = TYPE_PIIX_ISA,
>           .class_init     = piix3_class_init,
>       }, {
> -        .name           = TYPE_PIIX3_XEN_DEVICE,
> -        .parent         = TYPE_PIIX3_PCI_DEVICE,
> +        .name           = TYPE_PIIX3_ISA_XEN,
> +        .parent         = TYPE_PIIX_ISA,
>           .class_init     = piix3_xen_class_init,
>       }
>   };
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 702b458a3e..90e19a4c37 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -56,7 +56,7 @@ struct PIIX4State {
>       uint8_t rcr;
>   };
>   
> -OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
> +OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_ISA)
>   
>   static void piix4_set_irq(void *opaque, int irq_num, int level)
>   {
> @@ -81,7 +81,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>   
>   static void piix4_isa_reset(DeviceState *dev)
>   {
> -    PIIX4State *d = PIIX4_PCI_DEVICE(dev);
> +    PIIX4State *d = PIIX4_ISA(dev);
>       uint8_t *pci_conf = d->dev.config;
>   
>       pci_conf[0x04] = 0x07; // master, memory and I/O
> @@ -186,7 +186,7 @@ static const MemoryRegionOps piix4_rcr_ops = {
>   
>   static void piix4_realize(PCIDevice *dev, Error **errp)
>   {
> -    PIIX4State *s = PIIX4_PCI_DEVICE(dev);
> +    PIIX4State *s = PIIX4_ISA(dev);
>       PCIBus *pci_bus = pci_get_bus(dev);
>       ISABus *isa_bus;
>       qemu_irq *i8259_out_irq;
> @@ -253,7 +253,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>   
>   static void piix4_init(Object *obj)
>   {
> -    PIIX4State *s = PIIX4_PCI_DEVICE(obj);
> +    PIIX4State *s = PIIX4_ISA(obj);
>   
>       object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>       object_initialize_child(obj, "ide", &s->ide, TYPE_PIIX4_IDE);
> @@ -285,7 +285,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
>   }
>   
>   static const TypeInfo piix4_info = {
> -    .name          = TYPE_PIIX4_PCI_DEVICE,
> +    .name          = TYPE_PIIX4_ISA,
>       .parent        = TYPE_PCI_DEVICE,
>       .instance_size = sizeof(PIIX4State),
>       .instance_init = piix4_init,
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index ec172b111a..5aefeba581 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -1255,7 +1255,7 @@ void mips_malta_init(MachineState *machine)
>   
>       /* Southbridge */
>       piix4 = pci_create_simple_multifunction(pci_bus, PIIX4_PCI_DEVFN, true,
> -                                            TYPE_PIIX4_PCI_DEVICE);
> +                                            TYPE_PIIX4_ISA);
>       isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix4), "isa.0"));
>   
>       dev = DEVICE(object_resolve_path_component(OBJECT(piix4), "ide"));
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index a58bf13a41..71a82ef266 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -58,11 +58,11 @@ struct PIIX3State {
>       MemoryRegion rcr_mem;
>   };
>   
> -#define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
> -#define TYPE_PIIX3_DEVICE "PIIX3"
> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> -#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> +#define TYPE_PIIX_ISA       "pci-piix3"
> +#define TYPE_PIIX3_ISA      "PIIX3"
> +#define TYPE_PIIX3_ISA_XEN  "PIIX3-xen"
> +#define TYPE_PIIX4_ISA      "piix4-isa"
>   
> -OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_PCI_DEVICE)
> +OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_ISA)
>   
>   #endif

I think this patch makes things more confusing: here you've got a type with a parent 
of PCI_DEVICE which is then using a PIIX3_ISA QOM macro. I'd prefer to leave this, or 
rename it to something that better represents what it is i.e. PIIX3_PCI_ISA_BRIDGE.


ATB,

Mark.

Re: [PATCH v3 18/18] hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases

[Mark Cave-Ayland]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> Unify PIIX ISA (PCI function #0) as:
> 
>   pci-piix3 -> piix-isa       (abstract base class)
>   PIIX3     -> piix3-isa      (PIIX3 implementation)
>   PIIX3-xen -> piix3-isa-xen  (PIIX3 implementation with Xen extensions)
>   piix4-isa -> piix4-isa      (PIIX4 implementation)
> 
> Alias previous names in the QDevAlias table.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/southbridge/piix.h | 6 +++---
>   softmmu/qdev-monitor.c        | 3 +++
>   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 71a82ef266..cce65e8f44 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -58,9 +58,9 @@ struct PIIX3State {
>       MemoryRegion rcr_mem;
>   };
>   
> -#define TYPE_PIIX_ISA       "pci-piix3"
> -#define TYPE_PIIX3_ISA      "PIIX3"
> -#define TYPE_PIIX3_ISA_XEN  "PIIX3-xen"
> +#define TYPE_PIIX_ISA       "piix-isa"
> +#define TYPE_PIIX3_ISA      "piix3-isa"
> +#define TYPE_PIIX3_ISA_XEN  "piix3-isa-xen"
>   #define TYPE_PIIX4_ISA      "piix4-isa"
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(PIIX3State, PIIX3_ISA)
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index b8d2c4dadd..820e7f52ad 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -72,6 +72,9 @@ static const QDevAlias qdev_alias_table[] = {
>       { "ES1370", "es1370" }, /* -soundhw name */
>       { "ich9-ahci", "ahci" },
>       { "lsi53c895a", "lsi" },
> +    { "piix-isa", "pci-piix3" },
> +    { "piix3-isa", "PIIX3" },
> +    { "piix3-isa-xen", "PIIX3-xen" },
>       { "virtio-9p-device", "virtio-9p", QEMU_ARCH_VIRTIO_MMIO },
>       { "virtio-9p-ccw", "virtio-9p", QEMU_ARCH_VIRTIO_CCW },
>       { "virtio-9p-pci", "virtio-9p", QEMU_ARCH_VIRTIO_PCI },

Same comment here re: naming: I completely agree that the existing name is confusing, 
however I don't find the replacements that less confusing either :/


ATB,

Mark.

Re: [PATCH v3 00/18] hw/ide: Untangle ISA/PCI abuses of ide_init_ioport()

[Mark Cave-Ayland]

On 21/04/2023 09:25, Michael S. Tsirkin wrote:

> On Thu, Mar 02, 2023 at 11:40:40PM +0100, Philippe Mathieu-Daudé wrote:
>> Since v2: rebased
>>
>> I'm posting this series as it to not block Bernhard's PIIX
>> cleanup work. I don't have code change planned, but eventually
>> reword / improve commit descriptions.
> 
>> Tested commit after commit to be sure it is bisectable. Sadly
>> this was before Zoltan & Thomas report a problem with commit
>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder").
> 
> I'm not sure what this implies, or how do you want to
> resolve the conflicts with Bernhard's work.
> 
> did my best to review, series:
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Phil: I revisited this series after our discussion earlier in the week re: device 
IRQs, and I realised that there is some overlap with this series, Bernhard's latest 
series, and some of my local patches for switching between legacy/native PCI IDE IRQ 
routing.

I've now gone through both series and replied where I think we should now be going 
with this, but I can see that the final series will likely pull from all 3 sources.

How do you think we should best move forward from here? Once you've replied to the 
various comments I can potentially pull everything together into an updated series if 
that helps?

>> Background thread:
>> https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/
>>
>> Philippe Mathieu-Daudé (18):
>>    hw/ide/piix: Expose output IRQ as properties for late object
>>      population
>>    hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function
>>    hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15
>>    hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15
>>    hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa
>>    hw/ide/piix: Ensure IDE output IRQs are wired at realization
>>    hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq()
>>    hw/ide: Introduce generic ide_init_ioport()
>>    hw/ide/piix: Use generic ide_bus_init_ioport()
>>    hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device
>>    hw/isa: Simplify isa_address_space[_io]()
>>    hw/isa: Reduce 'isabus' singleton scope to isa_bus_new()
>>    exec/ioport: Factor portio_list_register_flush_coalesced() out
>>    exec/ioport: Factor portio_list_register() out
>>    hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro
>>    hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro
>>    hw/isa/piix: Unify QOM type name of PIIX ISA function
>>    hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases
>>
>>   hw/audio/adlib.c              |  4 +--
>>   hw/display/qxl.c              |  7 ++--
>>   hw/display/vga.c              |  9 +++--
>>   hw/dma/i82374.c               |  7 ++--
>>   hw/i386/pc_piix.c             | 13 +++++---
>>   hw/ide/ioport.c               | 15 +++++++--
>>   hw/ide/isa.c                  |  2 +-
>>   hw/ide/piix.c                 | 54 +++++++++++++++++++++++-------
>>   hw/isa/isa-bus.c              | 36 ++++++++------------
>>   hw/isa/piix3.c                | 63 +++++++++++++++--------------------
>>   hw/isa/piix4.c                | 12 ++++---
>>   hw/mips/malta.c               |  2 +-
>>   hw/watchdog/wdt_ib700.c       |  4 +--
>>   include/exec/ioport.h         | 15 +++++----
>>   include/hw/ide/internal.h     |  3 +-
>>   include/hw/ide/isa.h          |  3 ++
>>   include/hw/ide/piix.h         |  4 +++
>>   include/hw/isa/isa.h          |  3 +-
>>   include/hw/southbridge/piix.h | 14 ++++----
>>   softmmu/ioport.c              | 48 +++++++++++++++++++-------
>>   softmmu/qdev-monitor.c        |  3 ++
>>   21 files changed, 190 insertions(+), 131 deletions(-)
>>
>> -- 
>> 2.38.1


ATB,

Mark.

Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Bernhard Beschow]

Am 26. April 2023 12:50:08 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:
>
>> Since pc_init1() has access to the ISABus*, retrieve the
>> ISA IRQs with isa_bus_get_irq().
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/pc_piix.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 126b6c11df..1e90b9ff0d 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
>>       if (pcmc->pci_enabled) {
>>           PCIDevice *dev;
>>   -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, TYPE_PIIX3_IDE);
>> +        dev = pci_new_multifunction(piix3_devfn + 1, false, TYPE_PIIX3_IDE);
>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
>> +                                    isa_bus_get_irq(isa_bus, 14));
>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
>> +                                    isa_bus_get_irq(isa_bus, 15));
>> +        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");
>
>Another reason this probably isn't a good idea: you're having to call qdev_connect_gpio_*() before realizing the device :(

Just curious: Resources like memory regions are assigned before realization, see e.g. i440fx or q35. Interrupts are also resources. What makes them special?

I'm asking since this issue seems to be the main blocker for the piix consolidation to be merged.

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.

Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Bernhard Beschow]

Am 26. April 2023 12:50:08 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:
>
>> Since pc_init1() has access to the ISABus*, retrieve the
>> ISA IRQs with isa_bus_get_irq().
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/pc_piix.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 126b6c11df..1e90b9ff0d 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
>>       if (pcmc->pci_enabled) {
>>           PCIDevice *dev;
>>   -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, TYPE_PIIX3_IDE);
>> +        dev = pci_new_multifunction(piix3_devfn + 1, false, TYPE_PIIX3_IDE);
>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
>> +                                    isa_bus_get_irq(isa_bus, 14));
>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
>> +                                    isa_bus_get_irq(isa_bus, 15));
>> +        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");
>
>Another reason this probably isn't a good idea: you're having to call qdev_connect_gpio_*() before realizing the device :(

Just curious: Resources like memory regions are assigned before realization, see e.g. i440fx or q35. Interrupts are also resources. What makes them special?

I'm asking since this issue seems to be the main blocker for the piix consolidation to be merged.

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.

Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Mark Cave-Ayland]

On 27/04/2023 08:58, Bernhard Beschow wrote:

> Am 26. April 2023 12:50:08 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:
>>
>>> Since pc_init1() has access to the ISABus*, retrieve the
>>> ISA IRQs with isa_bus_get_irq().
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/i386/pc_piix.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 126b6c11df..1e90b9ff0d 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
>>>        if (pcmc->pci_enabled) {
>>>            PCIDevice *dev;
>>>    -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, TYPE_PIIX3_IDE);
>>> +        dev = pci_new_multifunction(piix3_devfn + 1, false, TYPE_PIIX3_IDE);
>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
>>> +                                    isa_bus_get_irq(isa_bus, 14));
>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
>>> +                                    isa_bus_get_irq(isa_bus, 15));
>>> +        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");
>>
>> Another reason this probably isn't a good idea: you're having to call qdev_connect_gpio_*() before realizing the device :(
> 
> Just curious: Resources like memory regions are assigned before realization, see e.g. i440fx or q35. Interrupts are also resources. What makes them special?

I think I've covered the .instance_init() vs. realize() part in my reply to Zoltan, 
but in terms of qdev_connect_gpio_out_named() the reasoning here is that a device 
shouldn't change it's internal behaviour depending upon how it is wired. In other 
words a standalone device will behave exactly the same as a device connected into a 
machine.

> I'm asking since this issue seems to be the main blocker for the piix consolidation to be merged.

I did have a brief catch-up with Phil at the start of the week, and he's tagged your 
series for review but he is really busy at the moment. As before I repeat my offer to 
help out if needed as I think it's a good series, but for now we're waiting for Phil 
to let us know what the next steps are...


ATB,

Mark.

Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3422 bytes --]

On Thu, 27 Apr 2023, Mark Cave-Ayland wrote:
> On 27/04/2023 08:58, Bernhard Beschow wrote:
>> Am 26. April 2023 12:50:08 UTC schrieb Mark Cave-Ayland 
>> <mark.cave-ayland@ilande.co.uk>:
>>> On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:
>>> 
>>>> Since pc_init1() has access to the ISABus*, retrieve the
>>>> ISA IRQs with isa_bus_get_irq().
>>>> 
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    hw/i386/pc_piix.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index 126b6c11df..1e90b9ff0d 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
>>>>        if (pcmc->pci_enabled) {
>>>>            PCIDevice *dev;
>>>>    -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, 
>>>> TYPE_PIIX3_IDE);
>>>> +        dev = pci_new_multifunction(piix3_devfn + 1, false, 
>>>> TYPE_PIIX3_IDE);
>>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
>>>> +                                    isa_bus_get_irq(isa_bus, 14));
>>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
>>>> +                                    isa_bus_get_irq(isa_bus, 15));
>>>> +        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");
>>> 
>>> Another reason this probably isn't a good idea: you're having to call 
>>> qdev_connect_gpio_*() before realizing the device :(
>> 
>> Just curious: Resources like memory regions are assigned before 
>> realization, see e.g. i440fx or q35. Interrupts are also resources. What 
>> makes them special?
>
> I think I've covered the .instance_init() vs. realize() part in my reply to 
> Zoltan, but in terms of qdev_connect_gpio_out_named() the reasoning here is

Well, not quite I'm afaid as I still have questions as it's not clear what 
should be in init and in realize.

I've looked at i440fx and it has no init just realize which does what 
init method shoulc do anf the i440fx_init there is a legacy init function 
which should probably be realize so this does not look to be a good 
example. The q35 model is more complex and I proably don't understand it 
fully but still has a realize where most of the memory regions are created 
and an init method where some tweaks are done that are mentioned in a 
comment as needed but not the norm so it may also not be the best example 
so I'm not even getting why Bernhard's cited these in the first place.

Maybe some QOM/qdev experts could give some advice here.

Regards,
BALATON Zoltan

> that a device shouldn't change it's internal behaviour depending upon how it 
> is wired. In other words a standalone device will behave exactly the same as 
> a device connected into a machine.
>
>> I'm asking since this issue seems to be the main blocker for the piix 
>> consolidation to be merged.
>
> I did have a brief catch-up with Phil at the start of the week, and he's 
> tagged your series for review but he is really busy at the moment. As before 
> I repeat my offer to help out if needed as I think it's a good series, but 
> for now we're waiting for Phil to let us know what the next steps are...
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH v3 03/18] hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15

[Bernhard Beschow]

Am 27. April 2023 13:04:15 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Thu, 27 Apr 2023, Mark Cave-Ayland wrote:
>> On 27/04/2023 08:58, Bernhard Beschow wrote:
>>> Am 26. April 2023 12:50:08 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>>> On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:
>>>> 
>>>>> Since pc_init1() has access to the ISABus*, retrieve the
>>>>> ISA IRQs with isa_bus_get_irq().
>>>>> 
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>    hw/i386/pc_piix.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>> index 126b6c11df..1e90b9ff0d 100644
>>>>> --- a/hw/i386/pc_piix.c
>>>>> +++ b/hw/i386/pc_piix.c
>>>>> @@ -277,7 +277,13 @@ static void pc_init1(MachineState *machine,
>>>>>        if (pcmc->pci_enabled) {
>>>>>            PCIDevice *dev;
>>>>>    -        dev = pci_create_simple(pci_bus, piix3_devfn + 1, TYPE_PIIX3_IDE);
>>>>> +        dev = pci_new_multifunction(piix3_devfn + 1, false, TYPE_PIIX3_IDE);
>>>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 0,
>>>>> +                                    isa_bus_get_irq(isa_bus, 14));
>>>>> +        qdev_connect_gpio_out_named(DEVICE(dev), "ide-irq", 1,
>>>>> +                                    isa_bus_get_irq(isa_bus, 15));
>>>>> +        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");
>>>> 
>>>> Another reason this probably isn't a good idea: you're having to call qdev_connect_gpio_*() before realizing the device :(
>>> 
>>> Just curious: Resources like memory regions are assigned before realization, see e.g. i440fx or q35. Interrupts are also resources. What makes them special?
>> 
>> I think I've covered the .instance_init() vs. realize() part in my reply to Zoltan, but in terms of qdev_connect_gpio_out_named() the reasoning here is
>
>Well, not quite I'm afaid as I still have questions as it's not clear what should be in init and in realize.
>
>I've looked at i440fx and it has no init just realize which does what init method shoulc do anf the i440fx_init there is a legacy init function which should probably be realize so this does not look to be a good example. The q35 model is more complex and I proably don't understand it fully but still has a realize where most of the memory regions are created and an init method where some tweaks are done that are mentioned in a comment as needed but not the norm so it may also not be the best example so I'm not even getting why Bernhard's cited these in the first place.

Let's not confuse the topics ".instance_init() vs. .realize()" and "resources". I440fx seems to be very old code -- so old that it still uses legacy init methods (not to be confused with .instance_init()" methods). I've chosen the examples in context of the "resources" topic.

Best regards,
Bernhard

>
>Maybe some QOM/qdev experts could give some advice here.
>
>Regards,
>BALATON Zoltan
>
>> that a device shouldn't change it's internal behaviour depending upon how it is wired. In other words a standalone device will behave exactly the same as a device connected into a machine.
>> 
>>> I'm asking since this issue seems to be the main blocker for the piix consolidation to be merged.
>> 
>> I did have a brief catch-up with Phil at the start of the week, and he's tagged your series for review but he is really busy at the moment. As before I repeat my offer to help out if needed as I think it's a good series, but for now we're waiting for Phil to let us know what the next steps are...
>> 
>> 
>> ATB,
>> 
>> Mark.
>> 
>>