[PATCH 00/12] hw: Strengthen SysBus & QBus API

[PATCH 00/12] hw: Strengthen SysBus & QBus API

[Philippe Mathieu-Daudé]

Hi,

This series ensure:

- qbus_new() and sysbus_init_mmio() are called *before*
  a device is realized,
- sysbus_mmio_map() is called *after* it is realized.

First we fix some abuse, then we enforce in qdev/sysbus
core code.

Philippe Mathieu-Daudé (12):
  hw/i386/amd_iommu: Do not use SysBus API to map local MMIO region
  hw/i386/intel_iommu: Do not use SysBus API to map local MMIO region
  hw/misc/allwinner-dramc: Move sysbus_mmio_map call from init ->
    realize
  hw/misc/allwinner-dramc: Do not use SysBus API to map local MMIO
    region
  hw/pci-host/bonito: Do not use SysBus API to map local MMIO region
  hw/acpi: Realize ACPI_GED sysbus device before accessing it
  hw/arm/virt: Realize ARM_GICV2M sysbus device before accessing it
  hw/isa: Realize ISA BUS sysbus device before accessing it
  hw/s390x/css-bridge: Realize sysbus device before accessing it
  hw/qdev: Ensure parent device is not realized before adding bus
  hw/sysbus: Ensure device is not realized before adding MMIO region
  hw/sysbus: Ensure device is realized before mapping it

 hw/arm/virt.c                 |  5 ++---
 hw/core/bus.c                 |  7 +++++++
 hw/core/sysbus.c              | 13 +++++++++++++
 hw/i386/amd_iommu.c           |  5 ++---
 hw/i386/intel_iommu.c         |  5 ++---
 hw/i386/microvm.c             |  2 +-
 hw/isa/isa-bus.c              | 11 +++++++++--
 hw/loongarch/virt.c           |  2 +-
 hw/misc/allwinner-r40-dramc.c | 20 +++++++++-----------
 hw/pci-host/bonito.c          | 29 ++++++++++++++---------------
 hw/s390x/css-bridge.c         |  7 +++----
 11 files changed, 63 insertions(+), 43 deletions(-)

-- 
2.41.0

[PATCH 01/12] hw/i386/amd_iommu: Do not use SysBus API to map local MMIO region

[Philippe Mathieu-Daudé]

There is no point in exposing an internal MMIO region via
SysBus and directly mapping it in the very same device.

Just map it without using the SysBus API.

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

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 8d0f2f99dd..7965415b47 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1579,9 +1579,8 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     /* set up MMIO */
     memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
                           AMDVI_MMIO_SIZE);
-
-    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
-    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
+    memory_region_add_subregion(get_system_memory(), AMDVI_BASE_ADDR,
+                                &s->mmio);
     pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
     amdvi_init(s);
 }
-- 
2.41.0

[PATCH 02/12] hw/i386/intel_iommu: Do not use SysBus API to map local MMIO region

[Philippe Mathieu-Daudé]

There is no point in exposing an internal MMIO region via
SysBus and directly mapping it in the very same device.

Just map it without using the SysBus API.

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

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 2c832ab68b..e4f6cedcb1 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4134,6 +4134,8 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     qemu_mutex_init(&s->iommu_lock);
     memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
                           "intel_iommu", DMAR_REG_SIZE);
+    memory_region_add_subregion(get_system_memory(),
+                                Q35_HOST_BRIDGE_IOMMU_ADDR, &s->csrmem);
 
     /* Create the shared memory regions by all devices */
     memory_region_init(&s->mr_nodmar, OBJECT(s), "vtd-nodmar",
@@ -4148,15 +4150,12 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion_overlap(&s->mr_nodmar,
                                         VTD_INTERRUPT_ADDR_FIRST,
                                         &s->mr_ir, 1);
-
-    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->csrmem);
     /* No corresponding destroy */
     s->iotlb = g_hash_table_new_full(vtd_iotlb_hash, vtd_iotlb_equal,
                                      g_free, g_free);
     s->vtd_address_spaces = g_hash_table_new_full(vtd_as_hash, vtd_as_equal,
                                       g_free, g_free);
     vtd_init(s);
-    sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
     pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
     /* Pseudo address space under root PCI bus. */
     x86ms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
-- 
2.41.0

[PATCH 03/12] hw/misc/allwinner-dramc: Move sysbus_mmio_map call from init -> realize

[Philippe Mathieu-Daudé]

In order to make the next commit trivial, move the sysbus_init_mmio()
call in allwinner_r40_dramc_init() just before the corresponding
sysbus_mmio_map_overlap() call in allwinner_r40_dramc_realize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/allwinner-r40-dramc.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/misc/allwinner-r40-dramc.c b/hw/misc/allwinner-r40-dramc.c
index 6944f84455..2cc0254a55 100644
--- a/hw/misc/allwinner-r40-dramc.c
+++ b/hw/misc/allwinner-r40-dramc.c
@@ -414,6 +414,7 @@ static void allwinner_r40_dramc_reset(DeviceState *dev)
 static void allwinner_r40_dramc_realize(DeviceState *dev, Error **errp)
 {
     AwR40DramCtlState *s = AW_R40_DRAMC(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
     if (!get_match_ddr(s->ram_size)) {
         error_report("%s: ram-size %u MiB is not supported",
@@ -421,8 +422,12 @@ static void allwinner_r40_dramc_realize(DeviceState *dev, Error **errp)
         exit(1);
     }
 
-    /* detect_cells */
-    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(s), 3, s->ram_addr, 10);
+    /* R40 support max 2G memory but we only support up to 1G now. index 3 */
+    memory_region_init_io(&s->detect_cells, OBJECT(s),
+                          &allwinner_r40_detect_ops, s,
+                          "DRAMCELLS", 1 * GiB);
+    sysbus_init_mmio(sbd, &s->detect_cells);
+    sysbus_mmio_map_overlap(sbd, 3, s->ram_addr, 10);
     memory_region_set_enabled(&s->detect_cells, false);
 
     /*
@@ -458,12 +463,6 @@ static void allwinner_r40_dramc_init(Object *obj)
                           &allwinner_r40_dramphy_ops, s,
                           "DRAMPHY", 4 * KiB);
     sysbus_init_mmio(sbd, &s->dramphy_iomem);
-
-    /* R40 support max 2G memory but we only support up to 1G now. index 3 */
-    memory_region_init_io(&s->detect_cells, OBJECT(s),
-                          &allwinner_r40_detect_ops, s,
-                          "DRAMCELLS", 1 * GiB);
-    sysbus_init_mmio(sbd, &s->detect_cells);
 }
 
 static Property allwinner_r40_dramc_properties[] = {
-- 
2.41.0

[PATCH 04/12] hw/misc/allwinner-dramc: Do not use SysBus API to map local MMIO region

[Philippe Mathieu-Daudé]

There is no point in exposing an internal MMIO region via
SysBus and directly mapping it in the very same device.

Just map it without using the SysBus API.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/allwinner-r40-dramc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/misc/allwinner-r40-dramc.c b/hw/misc/allwinner-r40-dramc.c
index 2cc0254a55..3d81ddb2e1 100644
--- a/hw/misc/allwinner-r40-dramc.c
+++ b/hw/misc/allwinner-r40-dramc.c
@@ -414,7 +414,6 @@ static void allwinner_r40_dramc_reset(DeviceState *dev)
 static void allwinner_r40_dramc_realize(DeviceState *dev, Error **errp)
 {
     AwR40DramCtlState *s = AW_R40_DRAMC(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
     if (!get_match_ddr(s->ram_size)) {
         error_report("%s: ram-size %u MiB is not supported",
@@ -422,23 +421,23 @@ static void allwinner_r40_dramc_realize(DeviceState *dev, Error **errp)
         exit(1);
     }
 
-    /* R40 support max 2G memory but we only support up to 1G now. index 3 */
+    /* R40 support max 2G memory but we only support up to 1G now. */
     memory_region_init_io(&s->detect_cells, OBJECT(s),
                           &allwinner_r40_detect_ops, s,
                           "DRAMCELLS", 1 * GiB);
-    sysbus_init_mmio(sbd, &s->detect_cells);
-    sysbus_mmio_map_overlap(sbd, 3, s->ram_addr, 10);
+    memory_region_add_subregion_overlap(get_system_memory(), s->ram_addr,
+                                        &s->detect_cells, 10);
     memory_region_set_enabled(&s->detect_cells, false);
 
     /*
      * We only support DRAM size up to 1G now, so prepare a high memory page
-     * after 1G for dualrank detect. index = 4
+     * after 1G for dualrank detect.
      */
     memory_region_init_io(&s->dram_high, OBJECT(s),
                             &allwinner_r40_dualrank_detect_ops, s,
                             "DRAMHIGH", KiB);
-    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->dram_high);
-    sysbus_mmio_map(SYS_BUS_DEVICE(s), 4, s->ram_addr + GiB);
+    memory_region_add_subregion(get_system_memory(), s->ram_addr + GiB,
+                                &s->dram_high);
 }
 
 static void allwinner_r40_dramc_init(Object *obj)
-- 
2.41.0

[PATCH 05/12] hw/pci-host/bonito: Do not use SysBus API to map local MMIO region

[Philippe Mathieu-Daudé]

There is no point in exposing an internal MMIO region via
SysBus and directly mapping it in the very same device.

Just map it without using the SysBus API.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/bonito.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
index ee6cb85e97..3b803bcad3 100644
--- a/hw/pci-host/bonito.c
+++ b/hw/pci-host/bonito.c
@@ -654,7 +654,6 @@ static void bonito_host_realize(DeviceState *dev, Error **errp)
 static void bonito_pci_realize(PCIDevice *dev, Error **errp)
 {
     PCIBonitoState *s = PCI_BONITO(dev);
-    SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
     PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
     BonitoState *bs = s->pcihost;
     MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
@@ -668,48 +667,48 @@ static void bonito_pci_realize(PCIDevice *dev, Error **errp)
     /* set the north bridge register mapping */
     memory_region_init_io(&s->iomem, OBJECT(s), &bonito_ops, s,
                           "north-bridge-register", BONITO_INTERNAL_REG_SIZE);
-    sysbus_init_mmio(sysbus, &s->iomem);
-    sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_INTERNAL_REG_BASE,
+                                &s->iomem);
 
     /* set the north bridge pci configure  mapping */
     memory_region_init_io(&phb->conf_mem, OBJECT(s), &bonito_pciconf_ops, s,
                           "north-bridge-pci-config", BONITO_PCICONFIG_SIZE);
-    sysbus_init_mmio(sysbus, &phb->conf_mem);
-    sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_PCICONFIG_BASE,
+                                &phb->conf_mem);
 
     /* set the south bridge pci configure  mapping */
     memory_region_init_io(&phb->data_mem, OBJECT(s), &bonito_spciconf_ops, s,
                           "south-bridge-pci-config", BONITO_SPCICONFIG_SIZE);
-    sysbus_init_mmio(sysbus, &phb->data_mem);
-    sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_SPCICONFIG_BASE,
+                                &phb->data_mem);
 
     create_unimplemented_device("bonito", BONITO_REG_BASE, BONITO_REG_SIZE);
 
     memory_region_init_io(&s->iomem_ldma, OBJECT(s), &bonito_ldma_ops, s,
                           "ldma", 0x100);
-    sysbus_init_mmio(sysbus, &s->iomem_ldma);
-    sysbus_mmio_map(sysbus, 3, 0x1fe00200);
+    memory_region_add_subregion(get_system_memory(), 0x1fe00200,
+                                &s->iomem_ldma);
 
     /* PCI copier */
     memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
                           "cop", 0x100);
-    sysbus_init_mmio(sysbus, &s->iomem_cop);
-    sysbus_mmio_map(sysbus, 4, 0x1fe00300);
+    memory_region_add_subregion(get_system_memory(), 0x1fe00300,
+                                &s->iomem_cop);
 
     create_unimplemented_device("ROMCS", BONITO_FLASH_BASE, 60 * MiB);
 
     /* Map PCI IO Space  0x1fd0 0000 - 0x1fd1 0000 */
     memory_region_init_alias(&s->bonito_pciio, OBJECT(s), "isa_mmio",
                              get_system_io(), 0, BONITO_PCIIO_SIZE);
-    sysbus_init_mmio(sysbus, &s->bonito_pciio);
-    sysbus_mmio_map(sysbus, 5, BONITO_PCIIO_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_PCIIO_BASE,
+                                &s->bonito_pciio);
 
     /* add pci local io mapping */
 
     memory_region_init_alias(&s->bonito_localio, OBJECT(s), "IOCS[0]",
                              get_system_io(), 0, 256 * KiB);
-    sysbus_init_mmio(sysbus, &s->bonito_localio);
-    sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
+    memory_region_add_subregion(get_system_memory(), BONITO_DEV_BASE,
+                                &s->bonito_localio);
     create_unimplemented_device("IOCS[1]", BONITO_DEV_BASE + 1 * 256 * KiB,
                                 256 * KiB);
     create_unimplemented_device("IOCS[2]", BONITO_DEV_BASE + 2 * 256 * KiB,
-- 
2.41.0

[PATCH 06/12] hw/acpi: Realize ACPI_GED sysbus device before accessing it

[Philippe Mathieu-Daudé]

sysbus_mmio_map() should not be called on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/virt.c       | 3 +--
 hw/i386/microvm.c   | 2 +-
 hw/loongarch/virt.c | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 15e74249f9..02c7a7ff3c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -647,13 +647,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
 
     dev = qdev_new(TYPE_ACPI_GED);
     qdev_prop_set_uint32(dev, "ged-event", event);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_ACPI_GED].base);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, vms->memmap[VIRT_PCDIMM_ACPI].base);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(vms->gic, irq));
 
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-
     return dev;
 }
 
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index b9c93039e2..ca55aecc3b 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -206,12 +206,12 @@ static void microvm_devices_init(MicrovmMachineState *mms)
     if (x86_machine_is_acpi_enabled(x86ms)) {
         DeviceState *dev = qdev_new(TYPE_ACPI_GED);
         qdev_prop_set_uint32(dev, "ged-event", ACPI_GED_PWR_DOWN_EVT);
+        sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, GED_MMIO_BASE);
         /* sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, GED_MMIO_BASE_MEMHP); */
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, GED_MMIO_BASE_REGS);
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
                            x86ms->gsi[GED_MMIO_IRQ]);
-        sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
         x86ms->acpi_dev = HOTPLUG_HANDLER(dev);
     }
 
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 2952fe452e..4b7dc67a2d 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -412,6 +412,7 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic, LoongArchMachineState
     }
     dev = qdev_new(TYPE_ACPI_GED);
     qdev_prop_set_uint32(dev, "ged-event", event);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     /* ged event */
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, VIRT_GED_EVT_ADDR);
@@ -422,7 +423,6 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic, LoongArchMachineState
 
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
                        qdev_get_gpio_in(pch_pic, VIRT_SCI_IRQ - VIRT_GSI_BASE));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     return dev;
 }
 
-- 
2.41.0

[PATCH 07/12] hw/arm/virt: Realize ARM_GICV2M sysbus device before accessing it

[Philippe Mathieu-Daudé]

sysbus_mmio_map() should not be called on unrealized device.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 02c7a7ff3c..5b08a98f07 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -690,10 +690,10 @@ static void create_v2m(VirtMachineState *vms)
     DeviceState *dev;
 
     dev = qdev_new("arm-gicv2m");
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_V2M].base);
     qdev_prop_set_uint32(dev, "base-spi", irq);
     qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_V2M].base);
 
     for (i = 0; i < NUM_GICV2M_SPIS; i++) {
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
-- 
2.41.0

[PATCH 08/12] hw/isa: Realize ISA BUS sysbus device before accessing it

[Philippe Mathieu-Daudé]

sysbus_mmio_map() should not be called on unrealized device.

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

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index a289eccfb1..f1e0f14007 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -52,18 +52,25 @@ static const TypeInfo isa_bus_info = {
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
                     MemoryRegion *address_space_io, Error **errp)
 {
+    DeviceState *bridge = NULL;
+
     if (isabus) {
         error_setg(errp, "Can't create a second ISA bus");
         return NULL;
     }
     if (!dev) {
-        dev = qdev_new("isabus-bridge");
-        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        bridge = qdev_new("isabus-bridge");
+        dev = bridge;
     }
 
     isabus = ISA_BUS(qbus_new(TYPE_ISA_BUS, dev, NULL));
     isabus->address_space = address_space;
     isabus->address_space_io = address_space_io;
+
+    if (bridge) {
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(bridge), &error_fatal);
+    }
+
     return isabus;
 }
 
-- 
2.41.0

[PATCH 09/12] hw/s390x/css-bridge: Realize sysbus device before accessing it

[Philippe Mathieu-Daudé]

sysbus_mmio_map() should not be called on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/s390x/css-bridge.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
index 4017081d49..15d26efc95 100644
--- a/hw/s390x/css-bridge.c
+++ b/hw/s390x/css-bridge.c
@@ -95,7 +95,6 @@ static const TypeInfo virtual_css_bus_info = {
 
 VirtualCssBus *virtual_css_bus_init(void)
 {
-    VirtualCssBus *cbus;
     BusState *bus;
     DeviceState *dev;
 
@@ -103,19 +102,19 @@ VirtualCssBus *virtual_css_bus_init(void)
     dev = qdev_new(TYPE_VIRTUAL_CSS_BRIDGE);
     object_property_add_child(qdev_get_machine(), TYPE_VIRTUAL_CSS_BRIDGE,
                               OBJECT(dev));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     /* Create bus on bridge device */
     bus = qbus_new(TYPE_VIRTUAL_CSS_BUS, dev, "virtual-css");
-    cbus = VIRTUAL_CSS_BUS(bus);
 
     /* Enable hotplugging */
     qbus_set_hotplug_handler(bus, OBJECT(dev));
 
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
     css_register_io_adapters(CSS_IO_ADAPTER_VIRTIO, true, false,
                              0, &error_abort);
 
-    return cbus;
+    return VIRTUAL_CSS_BUS(bus);
  }
 
 /***************** Virtual-css Bus Bridge Device ********************/
-- 
2.41.0

[PATCH 10/12] hw/qdev: Ensure parent device is not realized before adding bus

[Philippe Mathieu-Daudé]

qbus_new() should not be called on realized device.

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

diff --git a/hw/core/bus.c b/hw/core/bus.c
index c7831b5293..c92d07667b 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -21,6 +21,7 @@
 #include "hw/qdev-properties.h"
 #include "qemu/ctype.h"
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 
 void qbus_set_hotplug_handler(BusState *bus, Object *handler)
@@ -163,6 +164,12 @@ BusState *qbus_new(const char *typename, DeviceState *parent, const char *name)
 {
     BusState *bus;
 
+    if (parent->realized) {
+        error_report("qbus_new(type:%s parent:%s, name:%s) but parent realized",
+                     typename, object_get_typename(OBJECT(parent)), name);
+        abort();
+    }
+
     bus = BUS(object_new(typename));
     qbus_init_internal(bus, parent, name);
 
-- 
2.41.0

[PATCH 11/12] hw/sysbus: Ensure device is not realized before adding MMIO region

[Philippe Mathieu-Daudé]

sysbus_init_mmio() should not be called on realized device.

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

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 35f902b582..ce54e2c416 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -192,6 +192,11 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
     int n;
 
     assert(dev->num_mmio < QDEV_MAX_MMIO);
+    if (DEVICE(dev)->realized) {
+        error_report("sysbus_init_mmio(type:%s) but object is realized",
+                     object_get_typename(OBJECT(dev)));
+        abort();
+    }
     n = dev->num_mmio++;
     dev->mmio[n].addr = -1;
     dev->mmio[n].memory = memory;
-- 
2.41.0

[PATCH 12/12] hw/sysbus: Ensure device is realized before mapping it

[Philippe Mathieu-Daudé]

sysbus_mmio_map() should not be called on unrealized device.

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

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index ce54e2c416..a46828a808 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 #include "hw/sysbus.h"
 #include "monitor/monitor.h"
 #include "exec/address-spaces.h"
@@ -132,6 +133,13 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
 {
     assert(n >= 0 && n < dev->num_mmio);
 
+    if (!DEVICE(dev)->realized) {
+        error_report("sysbus_mmio_map(type:%s, index:%d, addr:0x%"HWADDR_PRIx","
+                     " prio:%d) but object is not realized",
+                     object_get_typename(OBJECT(dev)), n, addr, priority);
+        abort();
+    }
+
     if (dev->mmio[n].addr == addr) {
         /* ??? region already mapped here.  */
         return;
-- 
2.41.0

Re: [PATCH 09/12] hw/s390x/css-bridge: Realize sysbus device before accessing it

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> sysbus_mmio_map() should not be called on unrealized device.

Can you elaborate? I don't see a sysbus_mmio_map() in this code here...?

  Thomas


> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/s390x/css-bridge.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> index 4017081d49..15d26efc95 100644
> --- a/hw/s390x/css-bridge.c
> +++ b/hw/s390x/css-bridge.c
> @@ -95,7 +95,6 @@ static const TypeInfo virtual_css_bus_info = {
>   
>   VirtualCssBus *virtual_css_bus_init(void)
>   {
> -    VirtualCssBus *cbus;
>       BusState *bus;
>       DeviceState *dev;
>   
> @@ -103,19 +102,19 @@ VirtualCssBus *virtual_css_bus_init(void)
>       dev = qdev_new(TYPE_VIRTUAL_CSS_BRIDGE);
>       object_property_add_child(qdev_get_machine(), TYPE_VIRTUAL_CSS_BRIDGE,
>                                 OBJECT(dev));
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>   
>       /* Create bus on bridge device */
>       bus = qbus_new(TYPE_VIRTUAL_CSS_BUS, dev, "virtual-css");
> -    cbus = VIRTUAL_CSS_BUS(bus);
>   
>       /* Enable hotplugging */
>       qbus_set_hotplug_handler(bus, OBJECT(dev));
>   
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +
>       css_register_io_adapters(CSS_IO_ADAPTER_VIRTIO, true, false,
>                                0, &error_abort);
>   
> -    return cbus;
> +    return VIRTUAL_CSS_BUS(bus);
>    }
>   
>   /***************** Virtual-css Bus Bridge Device ********************/

Re: [PATCH 08/12] hw/isa: Realize ISA BUS sysbus device before accessing it

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> sysbus_mmio_map() should not be called on unrealized device.

I also cannot spot a sysbus_mmio_map() here ... do you mean qdev_new() instead?

  Thomas

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/isa/isa-bus.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index a289eccfb1..f1e0f14007 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -52,18 +52,25 @@ static const TypeInfo isa_bus_info = {
>   ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
>                       MemoryRegion *address_space_io, Error **errp)
>   {
> +    DeviceState *bridge = NULL;
> +
>       if (isabus) {
>           error_setg(errp, "Can't create a second ISA bus");
>           return NULL;
>       }
>       if (!dev) {
> -        dev = qdev_new("isabus-bridge");
> -        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +        bridge = qdev_new("isabus-bridge");
> +        dev = bridge;
>       }
>   
>       isabus = ISA_BUS(qbus_new(TYPE_ISA_BUS, dev, NULL));
>       isabus->address_space = address_space;
>       isabus->address_space_io = address_space_io;
> +
> +    if (bridge) {
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(bridge), &error_fatal);
> +    }
> +
>       return isabus;
>   }
>   

Re: [PATCH 07/12] hw/arm/virt: Realize ARM_GICV2M sysbus device before accessing it

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> sysbus_mmio_map() should not be called on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/virt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 02c7a7ff3c..5b08a98f07 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -690,10 +690,10 @@ static void create_v2m(VirtMachineState *vms)
>       DeviceState *dev;
>   
>       dev = qdev_new("arm-gicv2m");
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_V2M].base);
>       qdev_prop_set_uint32(dev, "base-spi", irq);
>       qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_V2M].base);
>   
>       for (i = 0; i < NUM_GICV2M_SPIS; i++) {
>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 06/12] hw/acpi: Realize ACPI_GED sysbus device before accessing it

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> sysbus_mmio_map() should not be called on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/virt.c       | 3 +--
>   hw/i386/microvm.c   | 2 +-
>   hw/loongarch/virt.c | 2 +-
>   3 files changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 05/12] hw/pci-host/bonito: Do not use SysBus API to map local MMIO region

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> There is no point in exposing an internal MMIO region via
> SysBus and directly mapping it in the very same device.
> 
> Just map it without using the SysBus API.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/bonito.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index ee6cb85e97..3b803bcad3 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -654,7 +654,6 @@ static void bonito_host_realize(DeviceState *dev, Error **errp)
>   static void bonito_pci_realize(PCIDevice *dev, Error **errp)
>   {
>       PCIBonitoState *s = PCI_BONITO(dev);
> -    SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
>       PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
>       BonitoState *bs = s->pcihost;
>       MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
> @@ -668,48 +667,48 @@ static void bonito_pci_realize(PCIDevice *dev, Error **errp)
>       /* set the north bridge register mapping */
>       memory_region_init_io(&s->iomem, OBJECT(s), &bonito_ops, s,
>                             "north-bridge-register", BONITO_INTERNAL_REG_SIZE);
> -    sysbus_init_mmio(sysbus, &s->iomem);
> -    sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_INTERNAL_REG_BASE,
> +                                &s->iomem);
>   
>       /* set the north bridge pci configure  mapping */
>       memory_region_init_io(&phb->conf_mem, OBJECT(s), &bonito_pciconf_ops, s,
>                             "north-bridge-pci-config", BONITO_PCICONFIG_SIZE);
> -    sysbus_init_mmio(sysbus, &phb->conf_mem);
> -    sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_PCICONFIG_BASE,
> +                                &phb->conf_mem);
>   
>       /* set the south bridge pci configure  mapping */
>       memory_region_init_io(&phb->data_mem, OBJECT(s), &bonito_spciconf_ops, s,
>                             "south-bridge-pci-config", BONITO_SPCICONFIG_SIZE);
> -    sysbus_init_mmio(sysbus, &phb->data_mem);
> -    sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_SPCICONFIG_BASE,
> +                                &phb->data_mem);
>   
>       create_unimplemented_device("bonito", BONITO_REG_BASE, BONITO_REG_SIZE);
>   
>       memory_region_init_io(&s->iomem_ldma, OBJECT(s), &bonito_ldma_ops, s,
>                             "ldma", 0x100);
> -    sysbus_init_mmio(sysbus, &s->iomem_ldma);
> -    sysbus_mmio_map(sysbus, 3, 0x1fe00200);
> +    memory_region_add_subregion(get_system_memory(), 0x1fe00200,
> +                                &s->iomem_ldma);
>   
>       /* PCI copier */
>       memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
>                             "cop", 0x100);
> -    sysbus_init_mmio(sysbus, &s->iomem_cop);
> -    sysbus_mmio_map(sysbus, 4, 0x1fe00300);
> +    memory_region_add_subregion(get_system_memory(), 0x1fe00300,
> +                                &s->iomem_cop);
>   
>       create_unimplemented_device("ROMCS", BONITO_FLASH_BASE, 60 * MiB);
>   
>       /* Map PCI IO Space  0x1fd0 0000 - 0x1fd1 0000 */
>       memory_region_init_alias(&s->bonito_pciio, OBJECT(s), "isa_mmio",
>                                get_system_io(), 0, BONITO_PCIIO_SIZE);
> -    sysbus_init_mmio(sysbus, &s->bonito_pciio);
> -    sysbus_mmio_map(sysbus, 5, BONITO_PCIIO_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_PCIIO_BASE,
> +                                &s->bonito_pciio);
>   
>       /* add pci local io mapping */
>   
>       memory_region_init_alias(&s->bonito_localio, OBJECT(s), "IOCS[0]",
>                                get_system_io(), 0, 256 * KiB);
> -    sysbus_init_mmio(sysbus, &s->bonito_localio);
> -    sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
> +    memory_region_add_subregion(get_system_memory(), BONITO_DEV_BASE,
> +                                &s->bonito_localio);
>       create_unimplemented_device("IOCS[1]", BONITO_DEV_BASE + 1 * 256 * KiB,
>                                   256 * KiB);
>       create_unimplemented_device("IOCS[2]", BONITO_DEV_BASE + 2 * 256 * KiB,

Would it make sense to cache the return value of get_system_memory() in a 
local variable instead of calling it again and again ...?

  Thomas

Re: [PATCH 10/12] hw/qdev: Ensure parent device is not realized before adding bus

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> qbus_new() should not be called on realized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/core/bus.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index c7831b5293..c92d07667b 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -21,6 +21,7 @@
>   #include "hw/qdev-properties.h"
>   #include "qemu/ctype.h"
>   #include "qemu/module.h"
> +#include "qemu/error-report.h"
>   #include "qapi/error.h"
>   
>   void qbus_set_hotplug_handler(BusState *bus, Object *handler)
> @@ -163,6 +164,12 @@ BusState *qbus_new(const char *typename, DeviceState *parent, const char *name)
>   {
>       BusState *bus;
>   
> +    if (parent->realized) {
> +        error_report("qbus_new(type:%s parent:%s, name:%s) but parent realized",
> +                     typename, object_get_typename(OBJECT(parent)), name);
> +        abort();
> +    }
> +
>       bus = BUS(object_new(typename));
>       qbus_init_internal(bus, parent, name);
>   

Sounds like a good idea!

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 11/12] hw/sysbus: Ensure device is not realized before adding MMIO region

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> sysbus_init_mmio() should not be called on realized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/core/sysbus.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 35f902b582..ce54e2c416 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -192,6 +192,11 @@ void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
>       int n;
>   
>       assert(dev->num_mmio < QDEV_MAX_MMIO);
> +    if (DEVICE(dev)->realized) {
> +        error_report("sysbus_init_mmio(type:%s) but object is realized",
> +                     object_get_typename(OBJECT(dev)));
> +        abort();
> +    }
>       n = dev->num_mmio++;
>       dev->mmio[n].addr = -1;
>       dev->mmio[n].memory = memory;

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 00/12] hw: Strengthen SysBus & QBus API

[Michael S. Tsirkin]

On Wed, Oct 18, 2023 at 04:11:38PM +0200, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series ensure:
> 
> - qbus_new() and sysbus_init_mmio() are called *before*
>   a device is realized,
> - sysbus_mmio_map() is called *after* it is realized.
> 
> First we fix some abuse, then we enforce in qdev/sysbus
> core code.


pc things:

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

> Philippe Mathieu-Daudé (12):
>   hw/i386/amd_iommu: Do not use SysBus API to map local MMIO region
>   hw/i386/intel_iommu: Do not use SysBus API to map local MMIO region
>   hw/misc/allwinner-dramc: Move sysbus_mmio_map call from init ->
>     realize
>   hw/misc/allwinner-dramc: Do not use SysBus API to map local MMIO
>     region
>   hw/pci-host/bonito: Do not use SysBus API to map local MMIO region
>   hw/acpi: Realize ACPI_GED sysbus device before accessing it
>   hw/arm/virt: Realize ARM_GICV2M sysbus device before accessing it
>   hw/isa: Realize ISA BUS sysbus device before accessing it
>   hw/s390x/css-bridge: Realize sysbus device before accessing it
>   hw/qdev: Ensure parent device is not realized before adding bus
>   hw/sysbus: Ensure device is not realized before adding MMIO region
>   hw/sysbus: Ensure device is realized before mapping it
> 
>  hw/arm/virt.c                 |  5 ++---
>  hw/core/bus.c                 |  7 +++++++
>  hw/core/sysbus.c              | 13 +++++++++++++
>  hw/i386/amd_iommu.c           |  5 ++---
>  hw/i386/intel_iommu.c         |  5 ++---
>  hw/i386/microvm.c             |  2 +-
>  hw/isa/isa-bus.c              | 11 +++++++++--
>  hw/loongarch/virt.c           |  2 +-
>  hw/misc/allwinner-r40-dramc.c | 20 +++++++++-----------
>  hw/pci-host/bonito.c          | 29 ++++++++++++++---------------
>  hw/s390x/css-bridge.c         |  7 +++----
>  11 files changed, 63 insertions(+), 43 deletions(-)
> 
> -- 
> 2.41.0

Re: [PATCH 12/12] hw/sysbus: Ensure device is realized before mapping it

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> sysbus_mmio_map() should not be called on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/core/sysbus.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index ce54e2c416..a46828a808 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -20,6 +20,7 @@
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> +#include "qemu/error-report.h"

I assume this hunk should go into the previous patch?

>   #include "hw/sysbus.h"
>   #include "monitor/monitor.h"
>   #include "exec/address-spaces.h"
> @@ -132,6 +133,13 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
>   {
>       assert(n >= 0 && n < dev->num_mmio);
>   
> +    if (!DEVICE(dev)->realized) {
> +        error_report("sysbus_mmio_map(type:%s, index:%d, addr:0x%"HWADDR_PRIx","
> +                     " prio:%d) but object is not realized",
> +                     object_get_typename(OBJECT(dev)), n, addr, priority);
> +        abort();
> +    }

With the hunk moved (or the order of the patches reversed):

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 00/12] hw: Strengthen SysBus & QBus API

[Thomas Huth]

On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series ensure:
> 
> - qbus_new() and sysbus_init_mmio() are called *before*
>    a device is realized,
> - sysbus_mmio_map() is called *after* it is realized.
> 
> First we fix some abuse, then we enforce in qdev/sysbus
> core code.

I like the idea, and just had a try with "make check-qtest" with the patches 
here, but seems like there are more spots that need attention:

  10/433 qemu:qtest+qtest-ppc64 / qtest-ppc64/qom-test 
              ERROR           0.72s   killed by signal 6 SIGABRT
 >>> MALLOC_PERTURB_=217 QTEST_QEMU_IMG=./qemu-img 
G_TEST_DBUS_DAEMON=/home/thuth/devel/qemu/tests/dbus-vmstate-daemon.sh 
PYTHON=/home/thuth/tmp/qemu-build/pyvenv/bin/python3 
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
QTEST_QEMU_BINARY=./qemu-system-ppc64 
/home/thuth/tmp/qemu-build/tests/qtest/qom-test --tap -k
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ 
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-ppc64: sysbus_mmio_map(type:power9_v2.2-pnv-chip, index:0, 
addr:0x603fc00000000, prio:0) but object is not realized
Broken pipe
../../devel/qemu/tests/qtest/libqtest.c:203: kill_qemu() detected QEMU death 
from signal 6 (Aborted) (core dumped)

(test program exited with status code -6)

TAP parsing error: Too few tests run (expected 17, got 0)
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

...

  24/433 qemu:qtest+qtest-aarch64 / qtest-aarch64/qom-test 
              ERROR           5.94s   killed by signal 6 SIGABRT
 >>> QTEST_QEMU_BINARY=./qemu-system-aarch64 
G_TEST_DBUS_DAEMON=/home/thuth/devel/qemu/tests/dbus-vmstate-daemon.sh 
QTEST_QEMU_IMG=./qemu-img 
PYTHON=/home/thuth/tmp/qemu-build/pyvenv/bin/python3 
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
MALLOC_PERTURB_=105 /home/thuth/tmp/qemu-build/tests/qtest/qom-test --tap -k
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ 
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-aarch64: sysbus_init_mmio(type:pxa2xx_pic) but object is realized
Broken pipe
../../devel/qemu/tests/qtest/libqtest.c:203: kill_qemu() detected QEMU death 
from signal 6 (Aborted) (core dumped)

(test program exited with status code -6)

TAP parsing error: Too few tests run (expected 95, got 3)
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

...

  73/433 qemu:qtest+qtest-ppc64 / qtest-ppc64/boot-serial-test 
              ERROR           2.65s   killed by signal 6 SIGABRT
 >>> QTEST_QEMU_IMG=./qemu-img 
G_TEST_DBUS_DAEMON=/home/thuth/devel/qemu/tests/dbus-vmstate-daemon.sh 
PYTHON=/home/thuth/tmp/qemu-build/pyvenv/bin/python3 
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
MALLOC_PERTURB_=129 QTEST_QEMU_BINARY=./qemu-system-ppc64 
/home/thuth/tmp/qemu-build/tests/qtest/boot-serial-test --tap -k
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ 
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-ppc64: sysbus_mmio_map(type:spapr-xive, index:0, 
addr:0x6010000000000, prio:0) but object is not realized
Broken pipe
../../devel/qemu/tests/qtest/libqtest.c:203: kill_qemu() detected QEMU death 
from signal 6 (Aborted) (core dumped)

(test program exited with status code -6)

TAP parsing error: Too few tests run (expected 7, got 3)
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

...

270/433 qemu:qtest+qtest-ppc64 / qtest-ppc64/cpu-plug-test 
             ERROR           0.40s   killed by signal 6 SIGABRT
 >>> QTEST_QEMU_IMG=./qemu-img 
G_TEST_DBUS_DAEMON=/home/thuth/devel/qemu/tests/dbus-vmstate-daemon.sh 
PYTHON=/home/thuth/tmp/qemu-build/pyvenv/bin/python3 
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
MALLOC_PERTURB_=175 QTEST_QEMU_BINARY=./qemu-system-ppc64 
/home/thuth/tmp/qemu-build/tests/qtest/cpu-plug-test --tap -k
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ 
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-ppc64: qbus_new(type:spapr-vio-bus parent:spapr-vio-bridge, 
name:spapr-vio) but parent realized
Broken pipe
../../devel/qemu/tests/qtest/libqtest.c:203: kill_qemu() detected QEMU death 
from signal 6 (Aborted) (core dumped)

(test program exited with status code -6)

TAP parsing error: Too few tests run (expected 3, got 0)
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

...

  HTH,
   Thomas

Re: [PATCH 05/12] hw/pci-host/bonito: Do not use SysBus API to map local MMIO region

[Philippe Mathieu-Daudé]

On 18/10/23 18:05, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> There is no point in exposing an internal MMIO region via
>> SysBus and directly mapping it in the very same device.
>>
>> Just map it without using the SysBus API.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/pci-host/bonito.c | 29 ++++++++++++++---------------
>>   1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
>> index ee6cb85e97..3b803bcad3 100644
>> --- a/hw/pci-host/bonito.c
>> +++ b/hw/pci-host/bonito.c
>> @@ -654,7 +654,6 @@ static void bonito_host_realize(DeviceState *dev, 
>> Error **errp)
>>   static void bonito_pci_realize(PCIDevice *dev, Error **errp)
>>   {
>>       PCIBonitoState *s = PCI_BONITO(dev);
>> -    SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
>>       PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
>>       BonitoState *bs = s->pcihost;
>>       MemoryRegion *pcimem_alias = g_new(MemoryRegion, 1);
>> @@ -668,48 +667,48 @@ static void bonito_pci_realize(PCIDevice *dev, 
>> Error **errp)
>>       /* set the north bridge register mapping */
>>       memory_region_init_io(&s->iomem, OBJECT(s), &bonito_ops, s,
>>                             "north-bridge-register", 
>> BONITO_INTERNAL_REG_SIZE);
>> -    sysbus_init_mmio(sysbus, &s->iomem);
>> -    sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_INTERNAL_REG_BASE,
>> +                                &s->iomem);
>>       /* set the north bridge pci configure  mapping */
>>       memory_region_init_io(&phb->conf_mem, OBJECT(s), 
>> &bonito_pciconf_ops, s,
>>                             "north-bridge-pci-config", 
>> BONITO_PCICONFIG_SIZE);
>> -    sysbus_init_mmio(sysbus, &phb->conf_mem);
>> -    sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_PCICONFIG_BASE,
>> +                                &phb->conf_mem);
>>       /* set the south bridge pci configure  mapping */
>>       memory_region_init_io(&phb->data_mem, OBJECT(s), 
>> &bonito_spciconf_ops, s,
>>                             "south-bridge-pci-config", 
>> BONITO_SPCICONFIG_SIZE);
>> -    sysbus_init_mmio(sysbus, &phb->data_mem);
>> -    sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
>> +    memory_region_add_subregion(get_system_memory(), 
>> BONITO_SPCICONFIG_BASE,
>> +                                &phb->data_mem);
>>       create_unimplemented_device("bonito", BONITO_REG_BASE, 
>> BONITO_REG_SIZE);
>>       memory_region_init_io(&s->iomem_ldma, OBJECT(s), 
>> &bonito_ldma_ops, s,
>>                             "ldma", 0x100);
>> -    sysbus_init_mmio(sysbus, &s->iomem_ldma);
>> -    sysbus_mmio_map(sysbus, 3, 0x1fe00200);
>> +    memory_region_add_subregion(get_system_memory(), 0x1fe00200,
>> +                                &s->iomem_ldma);
>>       /* PCI copier */
>>       memory_region_init_io(&s->iomem_cop, OBJECT(s), &bonito_cop_ops, s,
>>                             "cop", 0x100);
>> -    sysbus_init_mmio(sysbus, &s->iomem_cop);
>> -    sysbus_mmio_map(sysbus, 4, 0x1fe00300);
>> +    memory_region_add_subregion(get_system_memory(), 0x1fe00300,
>> +                                &s->iomem_cop);
>>       create_unimplemented_device("ROMCS", BONITO_FLASH_BASE, 60 * MiB);
>>       /* Map PCI IO Space  0x1fd0 0000 - 0x1fd1 0000 */
>>       memory_region_init_alias(&s->bonito_pciio, OBJECT(s), "isa_mmio",
>>                                get_system_io(), 0, BONITO_PCIIO_SIZE);
>> -    sysbus_init_mmio(sysbus, &s->bonito_pciio);
>> -    sysbus_mmio_map(sysbus, 5, BONITO_PCIIO_BASE);
>> +    memory_region_add_subregion(get_system_memory(), BONITO_PCIIO_BASE,
>> +                                &s->bonito_pciio);
>>       /* add pci local io mapping */
>>       memory_region_init_alias(&s->bonito_localio, OBJECT(s), "IOCS[0]",
>>                                get_system_io(), 0, 256 * KiB);
>> -    sysbus_init_mmio(sysbus, &s->bonito_localio);
>> -    sysbus_mmio_map(sysbus, 6, BONITO_DEV_BASE);
>> +    memory_region_add_subregion(get_system_memory(), BONITO_DEV_BASE,
>> +                                &s->bonito_localio);
>>       create_unimplemented_device("IOCS[1]", BONITO_DEV_BASE + 1 * 256 
>> * KiB,
>>                                   256 * KiB);
>>       create_unimplemented_device("IOCS[2]", BONITO_DEV_BASE + 2 * 256 
>> * KiB,
> 
> Would it make sense to cache the return value of get_system_memory() in 
> a local variable instead of calling it again and again ...?


This is done later, at this point I'm just doing mechanical
transformation with:

     @@
     expression sbdev;
     expression index;
     expression addr;
     expression subregion;
     @@
     -    sysbus_init_mmio(sbdev, subregion);
          ... when != sbdev
     -    sysbus_mmio_map(sbdev, index, addr);
     +    memory_region_add_subregion(get_system_memory(), addr, subregion);

     @@
     expression sbdev;
     expression index;
     expression addr;
     expression subregion;
     expression priority;
     @@
     -    sysbus_init_mmio(sbdev, subregion);
          ... when != sbdev
     -    sysbus_mmio_map_overlap(sbdev, index, addr, priority);
     +    memory_region_add_subregion_overlap(get_system_memory(), addr,
     +                                        subregion, priority);

Re: [PATCH 12/12] hw/sysbus: Ensure device is realized before mapping it

[Philippe Mathieu-Daudé]

On 18/10/23 18:13, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> sysbus_mmio_map() should not be called on unrealized device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/core/sysbus.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index ce54e2c416..a46828a808 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -20,6 +20,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qapi/error.h"
>>   #include "qemu/module.h"
>> +#include "qemu/error-report.h"
> 
> I assume this hunk should go into the previous patch?

Oops indeed...


> With the hunk moved (or the order of the patches reversed):
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!

Re: [PATCH 00/12] hw: Strengthen SysBus & QBus API

[Philippe Mathieu-Daudé]

On 18/10/23 18:24, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> This series ensure:
>>
>> - qbus_new() and sysbus_init_mmio() are called *before*
>>    a device is realized,
>> - sysbus_mmio_map() is called *after* it is realized.
>>
>> First we fix some abuse, then we enforce in qdev/sysbus
>> core code.
> 
> I like the idea, and just had a try with "make check-qtest" with the 
> patches here, but seems like there are more spots that need attention:
> 
>   10/433 qemu:qtest+qtest-ppc64 / qtest-ppc64/qom-test              
> ERROR           0.72s   killed by signal 6 SIGABRT
>  >>> MALLOC_PERTURB_=217 QTEST_QEMU_IMG=./qemu-img 
> G_TEST_DBUS_DAEMON=/home/thuth/devel/qemu/tests/dbus-vmstate-daemon.sh 
> PYTHON=/home/thuth/tmp/qemu-build/pyvenv/bin/python3 
> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
> QTEST_QEMU_BINARY=./qemu-system-ppc64 
> /home/thuth/tmp/qemu-build/tests/qtest/qom-test --tap -k
> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― 
> ✀ ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> stderr:
> qemu-system-ppc64: sysbus_mmio_map(type:power9_v2.2-pnv-chip, index:0, 
> addr:0x603fc00000000, prio:0) but object is not realized
> Broken pipe
> ../../devel/qemu/tests/qtest/libqtest.c:203: kill_qemu() detected QEMU 
> death from signal 6 (Aborted) (core dumped)
> 
> (test program exited with status code -6)
> 
> TAP parsing error: Too few tests run (expected 17, got 0)

Sorry, I forgot to add:

Based-on: <20231018133059.85765-1-philmd@linaro.org>
           "hw/ppc: SysBus simplifications"
https://lore.kernel.org/qemu-devel/20231018133059.85765-1-philmd@linaro.org/

Re: [PATCH 08/12] hw/isa: Realize ISA BUS sysbus device before accessing it

[Philippe Mathieu-Daudé]

On 18/10/23 17:57, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> sysbus_mmio_map() should not be called on unrealized device.
> 
> I also cannot spot a sysbus_mmio_map() here ... do you mean qdev_new() 
> instead?

Yeah, bad copy/paste :/

> 
>   Thomas
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/isa/isa-bus.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
>> index a289eccfb1..f1e0f14007 100644
>> --- a/hw/isa/isa-bus.c
>> +++ b/hw/isa/isa-bus.c
>> @@ -52,18 +52,25 @@ static const TypeInfo isa_bus_info = {
>>   ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
>>                       MemoryRegion *address_space_io, Error **errp)
>>   {
>> +    DeviceState *bridge = NULL;
>> +
>>       if (isabus) {
>>           error_setg(errp, "Can't create a second ISA bus");
>>           return NULL;
>>       }
>>       if (!dev) {
>> -        dev = qdev_new("isabus-bridge");
>> -        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +        bridge = qdev_new("isabus-bridge");
>> +        dev = bridge;
>>       }
>>       isabus = ISA_BUS(qbus_new(TYPE_ISA_BUS, dev, NULL));
>>       isabus->address_space = address_space;
>>       isabus->address_space_io = address_space_io;
>> +
>> +    if (bridge) {
>> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(bridge), &error_fatal);
>> +    }
>> +
>>       return isabus;
>>   }
> 

Re: [PATCH 09/12] hw/s390x/css-bridge: Realize sysbus device before accessing it

[Philippe Mathieu-Daudé]

On 18/10/23 17:06, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> sysbus_mmio_map() should not be called on unrealized device.
> 
> Can you elaborate? I don't see a sysbus_mmio_map() in this code here...?

I meant 's/sysbus_mmio_map/qbus_new'. Sorry, long day.

> 
>   Thomas
> 
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/s390x/css-bridge.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
>> index 4017081d49..15d26efc95 100644
>> --- a/hw/s390x/css-bridge.c
>> +++ b/hw/s390x/css-bridge.c
>> @@ -95,7 +95,6 @@ static const TypeInfo virtual_css_bus_info = {
>>   VirtualCssBus *virtual_css_bus_init(void)
>>   {
>> -    VirtualCssBus *cbus;
>>       BusState *bus;
>>       DeviceState *dev;
>> @@ -103,19 +102,19 @@ VirtualCssBus *virtual_css_bus_init(void)
>>       dev = qdev_new(TYPE_VIRTUAL_CSS_BRIDGE);
>>       object_property_add_child(qdev_get_machine(), 
>> TYPE_VIRTUAL_CSS_BRIDGE,
>>                                 OBJECT(dev));
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>       /* Create bus on bridge device */
>>       bus = qbus_new(TYPE_VIRTUAL_CSS_BUS, dev, "virtual-css");
>> -    cbus = VIRTUAL_CSS_BUS(bus);
>>       /* Enable hotplugging */
>>       qbus_set_hotplug_handler(bus, OBJECT(dev));
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +
>>       css_register_io_adapters(CSS_IO_ADAPTER_VIRTIO, true, false,
>>                                0, &error_abort);
>> -    return cbus;
>> +    return VIRTUAL_CSS_BUS(bus);
>>    }
>>   /***************** Virtual-css Bus Bridge Device ********************/
> 

Re: [PATCH 00/12] hw: Strengthen SysBus & QBus API

[Philippe Mathieu-Daudé]

On 18/10/23 18:24, Thomas Huth wrote:
> On 18/10/2023 16.11, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> This series ensure:
>>
>> - qbus_new() and sysbus_init_mmio() are called *before*
>>    a device is realized,
>> - sysbus_mmio_map() is called *after* it is realized.
>>
>> First we fix some abuse, then we enforce in qdev/sysbus
>> core code.
> 
> I like the idea, and just had a try with "make check-qtest" with the 
> patches here, but seems like there are more spots that need attention:


>   24/433 qemu:qtest+qtest-aarch64 / qtest-aarch64/qom-test              
> ERROR           5.94s   killed by signal 6 SIGABRT
>  >>> QTEST_QEMU_BINARY=./qemu-system-aarch64 
> G_TEST_DBUS_DAEMON=/home/thuth/devel/qemu/tests/dbus-vmstate-daemon.sh 
> QTEST_QEMU_IMG=./qemu-img 
> PYTHON=/home/thuth/tmp/qemu-build/pyvenv/bin/python3 
> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
> MALLOC_PERTURB_=105 /home/thuth/tmp/qemu-build/tests/qtest/qom-test 
> --tap -k
> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― 
> ✀ ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> stderr:
> qemu-system-aarch64: sysbus_init_mmio(type:pxa2xx_pic) but object is 
> realized
> Broken pipe
> ../../devel/qemu/tests/qtest/libqtest.c:203: kill_qemu() detected QEMU 
> death from signal 6 (Aborted) (core dumped)

I neglected to mention that in the cover letter. This is mentioned here
https://lore.kernel.org/qemu-devel/1b159c7a-f52c-3705-8757-c2b80a04965b@linaro.org/

I ping'd Peter on IRC because I'm not sure how to fix this PXA2xx code.
Apparently it cames from commit 3f6c925f37 ("Use i2c_slave_init() to
allocate the PXA (dummy) I2C slave"), which I presume was how to model
slave<->master transactions *before* I2C bus modelling.

Regards,

Phil.