[PATCH v5 00/13] hw/pci-host/raven clean ups

[PATCH v5 00/13] hw/pci-host/raven clean ups

[BALATON Zoltan]

Hello,

This series cleans up and simplifies the raven model which does some
strange stuff that no other pci-host is doing and does it in a
convoluted way and also has some legacy bits that can be removed.
Apart from making the model much more readable this also fixes the
non-contiguous IO control bit which was there but did not work as it
was not connected but apparently it's not really used by any guest so
that wasn't noticed.

Regards,
BALATON Zoltan

Link to previous version:
https://patchew.org/QEMU/cover.1760795082.git.balaton@eik.bme.hu/

v5:
- rebased on master
- split patch 1 (Philippe)

V4:
- added R-b tags from Mark and address some of his review comments
(other comments not addressed were answered on the list explaining why
I did not add them to this version)
- added new patches to fix creation and reset of prep-systemio
- rebased on master

V3:
- rebase on master
- Fix issue with device-crash-test reported by Akihiko Odaki: make
sure device is correctly used by adding assert and making it not user
creatable in patch 14.

v2:
- rebase on master
- add R-b tags from Philippe

BALATON Zoltan (13):
  hw/pci-host/raven: Simplify creating PCI facing part
  hw/pci-host/raven: Simplify PCI facing part
  hw/pci-host/raven: Simplify host bridge type declaration
  hw/pci-host/raven: Use DEFINE_TYPES macro
  hw/pci-host/raven: Simplify PCI bus creation
  hw/pci-host/raven: Simplify PCI interrupt routing
  hw/pci-host/raven: Do not use parent object for mmcfg region
  hw/pci-host/raven: Fix PCI config direct access region
  hw/pci-host/raven: Simpify discontiguous IO access
  hw/pci-host/raven: Move bus master address space creation to one place
  hw/pci-host/raven: Do not map regions in init method
  hw/ppc/prep: Fix non-contiguous IO control bit
  hw/ppc/prep: Add reset method to prep-systemio

 hw/pci-host/raven.c       | 292 ++++++++++++--------------------------
 hw/ppc/prep.c             |  18 ++-
 hw/ppc/prep_systemio.c    |  26 +++-
 include/hw/pci/pci_host.h |   1 -
 4 files changed, 123 insertions(+), 214 deletions(-)

-- 
2.41.3

[PATCH v5 01/13] hw/pci-host/raven: Simplify creating PCI facing part

[BALATON Zoltan]

There is no need to init and realize the PCI facing part of the host
bridge separately as it does not expose any properties that need to be
available before realize. It can be simpilfied using pci_create_simple.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index eacffc86d8..c0492d1456 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -65,7 +65,6 @@ struct PRePPCIState {
     MemoryRegion bm_ram_alias;
     MemoryRegion bm_pci_memory_alias;
     AddressSpace bm_as;
-    RavenPCIState pci_dev;
 
     int contiguous_map;
 };
@@ -260,8 +259,7 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
                           "pci-intack", 1);
     memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
 
-    /* TODO Remove once realize propagates to child devices. */
-    qdev_realize(DEVICE(&s->pci_dev), BUS(&s->pci_bus), errp);
+    pci_create_simple(&s->pci_bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
 }
 
 static void raven_pcihost_initfn(Object *obj)
@@ -269,7 +267,6 @@ static void raven_pcihost_initfn(Object *obj)
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
     MemoryRegion *address_space_mem = get_system_memory();
-    DeviceState *pci_dev;
 
     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
@@ -306,12 +303,6 @@ static void raven_pcihost_initfn(Object *obj)
     pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
 
     h->bus = &s->pci_bus;
-
-    object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_RAVEN_PCI_DEVICE);
-    pci_dev = DEVICE(&s->pci_dev);
-    object_property_set_int(OBJECT(&s->pci_dev), "addr", PCI_DEVFN(0, 0),
-                            NULL);
-    qdev_prop_set_bit(pci_dev, "multifunction", false);
 }
 
 static void raven_realize(PCIDevice *d, Error **errp)
-- 
2.41.3

Re: [PATCH v5 01/13] hw/pci-host/raven: Simplify creating PCI facing part

[Mark Cave-Ayland]

On 23/10/2025 16:26, BALATON Zoltan wrote:

> There is no need to init and realize the PCI facing part of the host
> bridge separately as it does not expose any properties that need to be
> available before realize. It can be simpilfied using pci_create_simple.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index eacffc86d8..c0492d1456 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -65,7 +65,6 @@ struct PRePPCIState {
>       MemoryRegion bm_ram_alias;
>       MemoryRegion bm_pci_memory_alias;
>       AddressSpace bm_as;
> -    RavenPCIState pci_dev;
>   
>       int contiguous_map;
>   };
> @@ -260,8 +259,7 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>                             "pci-intack", 1);
>       memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
>   
> -    /* TODO Remove once realize propagates to child devices. */
> -    qdev_realize(DEVICE(&s->pci_dev), BUS(&s->pci_bus), errp);
> +    pci_create_simple(&s->pci_bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
>   }
>   
>   static void raven_pcihost_initfn(Object *obj)
> @@ -269,7 +267,6 @@ static void raven_pcihost_initfn(Object *obj)
>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>       PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>       MemoryRegion *address_space_mem = get_system_memory();
> -    DeviceState *pci_dev;
>   
>       memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>       memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
> @@ -306,12 +303,6 @@ static void raven_pcihost_initfn(Object *obj)
>       pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
>   
>       h->bus = &s->pci_bus;
> -
> -    object_initialize(&s->pci_dev, sizeof(s->pci_dev), TYPE_RAVEN_PCI_DEVICE);
> -    pci_dev = DEVICE(&s->pci_dev);
> -    object_property_set_int(OBJECT(&s->pci_dev), "addr", PCI_DEVFN(0, 0),
> -                            NULL);
> -    qdev_prop_set_bit(pci_dev, "multifunction", false);
>   }
>   
>   static void raven_realize(PCIDevice *d, Error **errp)

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


ATB,

Mark.

[PATCH v5 02/13] hw/pci-host/raven: Simplify PCI facing part

[BALATON Zoltan]

The raven PCI device does not need a state struct as it has no data to
store there any more, so we can remove that to simplify code.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index c0492d1456..fa76e5170c 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -31,7 +31,6 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
 #include "hw/qdev-properties.h"
-#include "migration/vmstate.h"
 #include "hw/intc/i8259.h"
 #include "hw/irq.h"
 #include "hw/or-irq.h"
@@ -40,12 +39,6 @@
 #define TYPE_RAVEN_PCI_DEVICE "raven"
 #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
 
-OBJECT_DECLARE_SIMPLE_TYPE(RavenPCIState, RAVEN_PCI_DEVICE)
-
-struct RavenPCIState {
-    PCIDevice dev;
-};
-
 typedef struct PRePPCIState PREPPCIState;
 DECLARE_INSTANCE_CHECKER(PREPPCIState, RAVEN_PCI_HOST_BRIDGE,
                          TYPE_RAVEN_PCI_HOST_BRIDGE)
@@ -312,16 +305,6 @@ static void raven_realize(PCIDevice *d, Error **errp)
     d->config[PCI_CAPABILITY_LIST] = 0x00;
 }
 
-static const VMStateDescription vmstate_raven = {
-    .name = "raven",
-    .version_id = 0,
-    .minimum_version_id = 0,
-    .fields = (const VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, RavenPCIState),
-        VMSTATE_END_OF_LIST()
-    },
-};
-
 static void raven_class_init(ObjectClass *klass, const void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
@@ -333,7 +316,6 @@ static void raven_class_init(ObjectClass *klass, const void *data)
     k->revision = 0x00;
     k->class_id = PCI_CLASS_BRIDGE_HOST;
     dc->desc = "PReP Host Bridge - Motorola Raven";
-    dc->vmsd = &vmstate_raven;
     /*
      * Reason: PCI-facing part of the host bridge, not usable without
      * the host-facing part, which can't be device_add'ed, yet.
@@ -344,7 +326,6 @@ static void raven_class_init(ObjectClass *klass, const void *data)
 static const TypeInfo raven_info = {
     .name = TYPE_RAVEN_PCI_DEVICE,
     .parent = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(RavenPCIState),
     .class_init = raven_class_init,
     .interfaces = (const InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-- 
2.41.3

Re: [PATCH v5 02/13] hw/pci-host/raven: Simplify PCI facing part

[Mark Cave-Ayland]

On 23/10/2025 16:26, BALATON Zoltan wrote:

> The raven PCI device does not need a state struct as it has no data to
> store there any more, so we can remove that to simplify code.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 19 -------------------
>   1 file changed, 19 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index c0492d1456..fa76e5170c 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -31,7 +31,6 @@
>   #include "hw/pci/pci_bus.h"
>   #include "hw/pci/pci_host.h"
>   #include "hw/qdev-properties.h"
> -#include "migration/vmstate.h"
>   #include "hw/intc/i8259.h"
>   #include "hw/irq.h"
>   #include "hw/or-irq.h"
> @@ -40,12 +39,6 @@
>   #define TYPE_RAVEN_PCI_DEVICE "raven"
>   #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
>   
> -OBJECT_DECLARE_SIMPLE_TYPE(RavenPCIState, RAVEN_PCI_DEVICE)
> -
> -struct RavenPCIState {
> -    PCIDevice dev;
> -};
> -
>   typedef struct PRePPCIState PREPPCIState;
>   DECLARE_INSTANCE_CHECKER(PREPPCIState, RAVEN_PCI_HOST_BRIDGE,
>                            TYPE_RAVEN_PCI_HOST_BRIDGE)
> @@ -312,16 +305,6 @@ static void raven_realize(PCIDevice *d, Error **errp)
>       d->config[PCI_CAPABILITY_LIST] = 0x00;
>   }
>   
> -static const VMStateDescription vmstate_raven = {
> -    .name = "raven",
> -    .version_id = 0,
> -    .minimum_version_id = 0,
> -    .fields = (const VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(dev, RavenPCIState),
> -        VMSTATE_END_OF_LIST()
> -    },
> -};
> -
>   static void raven_class_init(ObjectClass *klass, const void *data)
>   {
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> @@ -333,7 +316,6 @@ static void raven_class_init(ObjectClass *klass, const void *data)
>       k->revision = 0x00;
>       k->class_id = PCI_CLASS_BRIDGE_HOST;
>       dc->desc = "PReP Host Bridge - Motorola Raven";
> -    dc->vmsd = &vmstate_raven;
>       /*
>        * Reason: PCI-facing part of the host bridge, not usable without
>        * the host-facing part, which can't be device_add'ed, yet.
> @@ -344,7 +326,6 @@ static void raven_class_init(ObjectClass *klass, const void *data)
>   static const TypeInfo raven_info = {
>       .name = TYPE_RAVEN_PCI_DEVICE,
>       .parent = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(RavenPCIState),
>       .class_init = raven_class_init,
>       .interfaces = (const InterfaceInfo[]) {
>           { INTERFACE_CONVENTIONAL_PCI_DEVICE },

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


ATB,

Mark.

[PATCH v5 03/13] hw/pci-host/raven: Simplify host bridge type declaration

[BALATON Zoltan]

Use OBJECT_DECLARE_SIMPLE_TYPE macro instead of open coding it and
change state struct name to match the previous typedef.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/raven.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index fa76e5170c..ea5ad69547 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -39,11 +39,9 @@
 #define TYPE_RAVEN_PCI_DEVICE "raven"
 #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
 
-typedef struct PRePPCIState PREPPCIState;
-DECLARE_INSTANCE_CHECKER(PREPPCIState, RAVEN_PCI_HOST_BRIDGE,
-                         TYPE_RAVEN_PCI_HOST_BRIDGE)
+OBJECT_DECLARE_SIMPLE_TYPE(PREPPCIState, RAVEN_PCI_HOST_BRIDGE)
 
-struct PRePPCIState {
+struct PREPPCIState {
     PCIHostState parent_obj;
 
     OrIRQState *or_irq;
-- 
2.41.3

[PATCH v5 04/13] hw/pci-host/raven: Use DEFINE_TYPES macro

[BALATON Zoltan]

Convert to using DEFINE_TYPES macro and move raven_pcihost_class_init
so methods of each object are grouped together.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/raven.c | 57 +++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index ea5ad69547..1d09a28bff 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -296,6 +296,15 @@ static void raven_pcihost_initfn(Object *obj)
     h->bus = &s->pci_bus;
 }
 
+static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    dc->realize = raven_pcihost_realizefn;
+    dc->fw_name = "pci";
+}
+
 static void raven_realize(PCIDevice *d, Error **errp)
 {
     d->config[PCI_CACHE_LINE_SIZE] = 0x08;
@@ -321,37 +330,23 @@ static void raven_class_init(ObjectClass *klass, const void *data)
     dc->user_creatable = false;
 }
 
-static const TypeInfo raven_info = {
-    .name = TYPE_RAVEN_PCI_DEVICE,
-    .parent = TYPE_PCI_DEVICE,
-    .class_init = raven_class_init,
-    .interfaces = (const InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
+static const TypeInfo raven_types[] = {
+    {
+        .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
+        .parent = TYPE_PCI_HOST_BRIDGE,
+        .instance_size = sizeof(PREPPCIState),
+        .instance_init = raven_pcihost_initfn,
+        .class_init = raven_pcihost_class_init,
+    },
+    {
+        .name = TYPE_RAVEN_PCI_DEVICE,
+        .parent = TYPE_PCI_DEVICE,
+        .class_init = raven_class_init,
+        .interfaces = (const InterfaceInfo[]) {
+            { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+            { },
+        },
     },
 };
 
-static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->realize = raven_pcihost_realizefn;
-    dc->fw_name = "pci";
-}
-
-static const TypeInfo raven_pcihost_info = {
-    .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
-    .parent = TYPE_PCI_HOST_BRIDGE,
-    .instance_size = sizeof(PREPPCIState),
-    .instance_init = raven_pcihost_initfn,
-    .class_init = raven_pcihost_class_init,
-};
-
-static void raven_register_types(void)
-{
-    type_register_static(&raven_pcihost_info);
-    type_register_static(&raven_info);
-}
-
-type_init(raven_register_types)
+DEFINE_TYPES(raven_types)
-- 
2.41.3

[PATCH v5 05/13] hw/pci-host/raven: Simplify PCI bus creation

[BALATON Zoltan]

Instead of doing it manually use pci_register_root_bus() to create and
register the PCI bus. Also drop pci_bus from PREPPCIState and use the
existing bus field in the parent PCIHostState.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/raven.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 1d09a28bff..22ad244eb6 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -46,7 +46,6 @@ struct PREPPCIState {
 
     OrIRQState *or_irq;
     qemu_irq pci_irqs[PCI_NUM_PINS];
-    PCIBus pci_bus;
     AddressSpace pci_io_as;
     MemoryRegion pci_io;
     MemoryRegion pci_io_non_contiguous;
@@ -231,8 +230,9 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 
     qdev_init_gpio_in(d, raven_change_gpio, 1);
 
-    pci_bus_irqs(&s->pci_bus, raven_set_irq, s, PCI_NUM_PINS);
-    pci_bus_map_irqs(&s->pci_bus, raven_map_irq);
+    h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,
+                                   s, &s->pci_memory, &s->pci_io, 0, 4,
+                                   TYPE_PCI_BUS);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
                           "pci-conf-idx", 4);
@@ -250,12 +250,14 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
                           "pci-intack", 1);
     memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
 
-    pci_create_simple(&s->pci_bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
+    pci_create_simple(h->bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
+
+    address_space_init(&s->bm_as, &s->bm, "raven-bm");
+    pci_setup_iommu(h->bus, &raven_iommu_ops, s);
 }
 
 static void raven_pcihost_initfn(Object *obj)
 {
-    PCIHostState *h = PCI_HOST_BRIDGE(obj);
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
     MemoryRegion *address_space_mem = get_system_memory();
 
@@ -278,8 +280,6 @@ static void raven_pcihost_initfn(Object *obj)
     memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
                                         &s->pci_io_non_contiguous, 1);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-    pci_root_bus_init(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
-                      &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
 
     /* Bus master address space */
     memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
@@ -290,10 +290,6 @@ static void raven_pcihost_initfn(Object *obj)
                              get_system_memory(), 0, 0x80000000);
     memory_region_add_subregion(&s->bm, 0         , &s->bm_pci_memory_alias);
     memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
-    address_space_init(&s->bm_as, &s->bm, "raven-bm");
-    pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
-
-    h->bus = &s->pci_bus;
 }
 
 static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
-- 
2.41.3

[PATCH v5 06/13] hw/pci-host/raven: Simplify PCI interrupt routing

[BALATON Zoltan]

No need to use an or-irq to map interrupt lines to a single IRQ as the
PCI code can handle this internally so simplify by dropping the or-irq
similar to how ppc440_pcix.c does it.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c | 39 +++++++++++++++------------------------
 hw/ppc/prep.c       |  5 ++++-
 2 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 22ad244eb6..2057a1869f 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -30,11 +30,8 @@
 #include "hw/pci/pci_device.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
-#include "hw/qdev-properties.h"
 #include "hw/intc/i8259.h"
 #include "hw/irq.h"
-#include "hw/or-irq.h"
-#include "qom/object.h"
 
 #define TYPE_RAVEN_PCI_DEVICE "raven"
 #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
@@ -44,8 +41,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PREPPCIState, RAVEN_PCI_HOST_BRIDGE)
 struct PREPPCIState {
     PCIHostState parent_obj;
 
-    OrIRQState *or_irq;
-    qemu_irq pci_irqs[PCI_NUM_PINS];
+    qemu_irq irq;
     AddressSpace pci_io_as;
     MemoryRegion pci_io;
     MemoryRegion pci_io_non_contiguous;
@@ -175,16 +171,25 @@ static const MemoryRegionOps raven_io_ops = {
     .valid.unaligned = true,
 };
 
+/*
+ * All four IRQ[ABCD] pins from all slots are tied to a single board
+ * IRQ, so our mapping function here maps everything to IRQ 0.
+ * The code in pci_change_irq_level() tracks the number of times
+ * the mapped IRQ is asserted and deasserted, so if multiple devices
+ * assert an IRQ at the same time the behaviour is correct.
+ *
+ * This may need further refactoring for boards that use multiple IRQ lines.
+ */
 static int raven_map_irq(PCIDevice *pci_dev, int irq_num)
 {
-    return (irq_num + (pci_dev->devfn >> 3)) & 1;
+    return 0;
 }
 
 static void raven_set_irq(void *opaque, int irq_num, int level)
 {
-    PREPPCIState *s = opaque;
+    qemu_irq *irq = opaque;
 
-    qemu_set_irq(s->pci_irqs[irq_num], level);
+    qemu_set_irq(*irq, level);
 }
 
 static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
@@ -212,26 +217,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
     MemoryRegion *address_space_mem = get_system_memory();
-    int i;
-
-    /*
-     * According to PReP specification section 6.1.6 "System Interrupt
-     * Assignments", all PCI interrupts are routed via IRQ 15
-     */
-    s->or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
-    object_property_set_int(OBJECT(s->or_irq), "num-lines", PCI_NUM_PINS,
-                            &error_fatal);
-    qdev_realize(DEVICE(s->or_irq), NULL, &error_fatal);
-    sysbus_init_irq(dev, &s->or_irq->out_irq);
-
-    for (i = 0; i < PCI_NUM_PINS; i++) {
-        s->pci_irqs[i] = qdev_get_gpio_in(DEVICE(s->or_irq), i);
-    }
 
     qdev_init_gpio_in(d, raven_change_gpio, 1);
 
+    sysbus_init_irq(dev, &s->irq);
     h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,
-                                   s, &s->pci_memory, &s->pci_io, 0, 4,
+                                   &s->irq, &s->pci_memory, &s->pci_io, 0, 1,
                                    TYPE_PCI_BUS);
 
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index c730cb3429..816455d289 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -304,7 +304,10 @@ static void ibm_40p_init(MachineState *machine)
     qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
     qdev_connect_gpio_out(i82378_dev, 0,
                           qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
-
+    /*
+     * According to PReP specification section 6.1.6 "System Interrupt
+     * Assignments", all PCI interrupts are routed via IRQ 15
+     */
     sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15));
     isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
 
-- 
2.41.3

Re: [PATCH v5 06/13] hw/pci-host/raven: Simplify PCI interrupt routing

[Mark Cave-Ayland]

On 23/10/2025 16:26, BALATON Zoltan wrote:

> No need to use an or-irq to map interrupt lines to a single IRQ as the
> PCI code can handle this internally so simplify by dropping the or-irq
> similar to how ppc440_pcix.c does it.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 39 +++++++++++++++------------------------
>   hw/ppc/prep.c       |  5 ++++-
>   2 files changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 22ad244eb6..2057a1869f 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -30,11 +30,8 @@
>   #include "hw/pci/pci_device.h"
>   #include "hw/pci/pci_bus.h"
>   #include "hw/pci/pci_host.h"
> -#include "hw/qdev-properties.h"
>   #include "hw/intc/i8259.h"
>   #include "hw/irq.h"
> -#include "hw/or-irq.h"
> -#include "qom/object.h"
>   
>   #define TYPE_RAVEN_PCI_DEVICE "raven"
>   #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
> @@ -44,8 +41,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PREPPCIState, RAVEN_PCI_HOST_BRIDGE)
>   struct PREPPCIState {
>       PCIHostState parent_obj;
>   
> -    OrIRQState *or_irq;
> -    qemu_irq pci_irqs[PCI_NUM_PINS];
> +    qemu_irq irq;
>       AddressSpace pci_io_as;
>       MemoryRegion pci_io;
>       MemoryRegion pci_io_non_contiguous;
> @@ -175,16 +171,25 @@ static const MemoryRegionOps raven_io_ops = {
>       .valid.unaligned = true,
>   };
>   
> +/*
> + * All four IRQ[ABCD] pins from all slots are tied to a single board
> + * IRQ, so our mapping function here maps everything to IRQ 0.
> + * The code in pci_change_irq_level() tracks the number of times
> + * the mapped IRQ is asserted and deasserted, so if multiple devices
> + * assert an IRQ at the same time the behaviour is correct.
> + *
> + * This may need further refactoring for boards that use multiple IRQ lines.
> + */
>   static int raven_map_irq(PCIDevice *pci_dev, int irq_num)
>   {
> -    return (irq_num + (pci_dev->devfn >> 3)) & 1;
> +    return 0;
>   }
>   
>   static void raven_set_irq(void *opaque, int irq_num, int level)
>   {
> -    PREPPCIState *s = opaque;
> +    qemu_irq *irq = opaque;
>   
> -    qemu_set_irq(s->pci_irqs[irq_num], level);
> +    qemu_set_irq(*irq, level);
>   }
>   
>   static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
> @@ -212,26 +217,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>       PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
>       MemoryRegion *address_space_mem = get_system_memory();
> -    int i;
> -
> -    /*
> -     * According to PReP specification section 6.1.6 "System Interrupt
> -     * Assignments", all PCI interrupts are routed via IRQ 15
> -     */
> -    s->or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
> -    object_property_set_int(OBJECT(s->or_irq), "num-lines", PCI_NUM_PINS,
> -                            &error_fatal);
> -    qdev_realize(DEVICE(s->or_irq), NULL, &error_fatal);
> -    sysbus_init_irq(dev, &s->or_irq->out_irq);
> -
> -    for (i = 0; i < PCI_NUM_PINS; i++) {
> -        s->pci_irqs[i] = qdev_get_gpio_in(DEVICE(s->or_irq), i);
> -    }
>   
>       qdev_init_gpio_in(d, raven_change_gpio, 1);
>   
> +    sysbus_init_irq(dev, &s->irq);
>       h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,
> -                                   s, &s->pci_memory, &s->pci_io, 0, 4,
> +                                   &s->irq, &s->pci_memory, &s->pci_io, 0, 1,
>                                      TYPE_PCI_BUS);
>   
>       memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index c730cb3429..816455d289 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -304,7 +304,10 @@ static void ibm_40p_init(MachineState *machine)
>       qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
>       qdev_connect_gpio_out(i82378_dev, 0,
>                             qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_INT));
> -
> +    /*
> +     * According to PReP specification section 6.1.6 "System Interrupt
> +     * Assignments", all PCI interrupts are routed via IRQ 15
> +     */
>       sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15));
>       isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
>   

 From a PCI bus perspective this is not correct: a PCI bus always has 4 IRQ lines, 
and so removing them from the model is the wrong thing to do. In fact in more general 
terms, any device that creates a PCI bus that doesn't have 4 IRQs is not doing the 
right thing (this is likely an artifact of conversion from older APIs).


ATB,

Mark.

Re: [PATCH v5 06/13] hw/pci-host/raven: Simplify PCI interrupt routing

[BALATON Zoltan]

On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
> On 23/10/2025 16:26, BALATON Zoltan wrote:
>> No need to use an or-irq to map interrupt lines to a single IRQ as the
>> PCI code can handle this internally so simplify by dropping the or-irq
>> similar to how ppc440_pcix.c does it.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/pci-host/raven.c | 39 +++++++++++++++------------------------
>>   hw/ppc/prep.c       |  5 ++++-
>>   2 files changed, 19 insertions(+), 25 deletions(-)
>> 
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index 22ad244eb6..2057a1869f 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -30,11 +30,8 @@
>>   #include "hw/pci/pci_device.h"
>>   #include "hw/pci/pci_bus.h"
>>   #include "hw/pci/pci_host.h"
>> -#include "hw/qdev-properties.h"
>>   #include "hw/intc/i8259.h"
>>   #include "hw/irq.h"
>> -#include "hw/or-irq.h"
>> -#include "qom/object.h"
>>     #define TYPE_RAVEN_PCI_DEVICE "raven"
>>   #define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
>> @@ -44,8 +41,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PREPPCIState, 
>> RAVEN_PCI_HOST_BRIDGE)
>>   struct PREPPCIState {
>>       PCIHostState parent_obj;
>>   -    OrIRQState *or_irq;
>> -    qemu_irq pci_irqs[PCI_NUM_PINS];
>> +    qemu_irq irq;
>>       AddressSpace pci_io_as;
>>       MemoryRegion pci_io;
>>       MemoryRegion pci_io_non_contiguous;
>> @@ -175,16 +171,25 @@ static const MemoryRegionOps raven_io_ops = {
>>       .valid.unaligned = true,
>>   };
>>   +/*
>> + * All four IRQ[ABCD] pins from all slots are tied to a single board
>> + * IRQ, so our mapping function here maps everything to IRQ 0.
>> + * The code in pci_change_irq_level() tracks the number of times
>> + * the mapped IRQ is asserted and deasserted, so if multiple devices
>> + * assert an IRQ at the same time the behaviour is correct.
>> + *
>> + * This may need further refactoring for boards that use multiple IRQ 
>> lines.
>> + */
>>   static int raven_map_irq(PCIDevice *pci_dev, int irq_num)
>>   {
>> -    return (irq_num + (pci_dev->devfn >> 3)) & 1;
>> +    return 0;
>>   }
>>     static void raven_set_irq(void *opaque, int irq_num, int level)
>>   {
>> -    PREPPCIState *s = opaque;
>> +    qemu_irq *irq = opaque;
>>   -    qemu_set_irq(s->pci_irqs[irq_num], level);
>> +    qemu_set_irq(*irq, level);
>>   }
>>     static AddressSpace *raven_pcihost_set_iommu(PCIBus *bus, void *opaque,
>> @@ -212,26 +217,12 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>> Error **errp)
>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>       PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
>>       MemoryRegion *address_space_mem = get_system_memory();
>> -    int i;
>> -
>> -    /*
>> -     * According to PReP specification section 6.1.6 "System Interrupt
>> -     * Assignments", all PCI interrupts are routed via IRQ 15
>> -     */
>> -    s->or_irq = OR_IRQ(object_new(TYPE_OR_IRQ));
>> -    object_property_set_int(OBJECT(s->or_irq), "num-lines", PCI_NUM_PINS,
>> -                            &error_fatal);
>> -    qdev_realize(DEVICE(s->or_irq), NULL, &error_fatal);
>> -    sysbus_init_irq(dev, &s->or_irq->out_irq);
>> -
>> -    for (i = 0; i < PCI_NUM_PINS; i++) {
>> -        s->pci_irqs[i] = qdev_get_gpio_in(DEVICE(s->or_irq), i);
>> -    }
>>         qdev_init_gpio_in(d, raven_change_gpio, 1);
>>   +    sysbus_init_irq(dev, &s->irq);
>>       h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,
>> -                                   s, &s->pci_memory, &s->pci_io, 0, 4,
>> +                                   &s->irq, &s->pci_memory, &s->pci_io, 0, 
>> 1,
>>                                      TYPE_PCI_BUS);
>>         memory_region_init_io(&h->conf_mem, OBJECT(h), 
>> &pci_host_conf_le_ops, s,
>> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
>> index c730cb3429..816455d289 100644
>> --- a/hw/ppc/prep.c
>> +++ b/hw/ppc/prep.c
>> @@ -304,7 +304,10 @@ static void ibm_40p_init(MachineState *machine)
>>       qdev_realize_and_unref(i82378_dev, BUS(pci_bus), &error_fatal);
>>       qdev_connect_gpio_out(i82378_dev, 0,
>>                             qdev_get_gpio_in(DEVICE(cpu), 
>> PPC6xx_INPUT_INT));
>> -
>> +    /*
>> +     * According to PReP specification section 6.1.6 "System Interrupt
>> +     * Assignments", all PCI interrupts are routed via IRQ 15
>> +     */
>>       sysbus_connect_irq(pcihost, 0, qdev_get_gpio_in(i82378_dev, 15));
>>       isa_bus = ISA_BUS(qdev_get_child_bus(i82378_dev, "isa.0"));
>> 
>
> From a PCI bus perspective this is not correct: a PCI bus always has 4 IRQ 
> lines, and so removing them from the model is the wrong thing to do. In fact 
> in more general terms, any device that creates a PCI bus that doesn't have 4 
> IRQs is not doing the right thing (this is likely an artifact of conversion 
> from older APIs).

We have 4 PCI interrupts which is handled by the PCI model of QEMU (cards 
still can use any of those lines) but we need to connect all of them to a 
single IRQ. We export the 4 lines and put an or-irq somewhere to achieve 
this but it's not needed as the QEMU PCI model can do this simpler by 
mapping all interrupts to one line and only exporting that. I've quoted 
the messages in my previous reply where we ended up with this solution for 
the ppc440 where first we wanted to use or-irq but concluded that it's an 
unneeded complication. If it was acceptable there it should be here too 
and achieves the same with less code. Since this is only used for 40p and 
nothing else no need to model something that nothing needs.

Regards,
BALATON Zoltan

[PATCH v5 07/13] hw/pci-host/raven: Do not use parent object for mmcfg region

[BALATON Zoltan]

The mmcfg field in PCIHostState is only used by raven for the PCI
config direct access but is not actually needed as the memory region
lifetime can be managed by the object given during init so use that
and remove the unused field from PCIHostState.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c       | 7 ++++---
 include/hw/pci/pci_host.h | 1 -
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 2057a1869f..23020fd09f 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -216,7 +216,7 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
-    MemoryRegion *address_space_mem = get_system_memory();
+    MemoryRegion *mr, *address_space_mem = get_system_memory();
 
     qdev_init_gpio_in(d, raven_change_gpio, 1);
 
@@ -233,9 +233,10 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
                           "pci-conf-data", 4);
     memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
 
-    memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
+    mr = g_new0(MemoryRegion, 1);
+    memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
                           "pci-mmcfg", 0x00400000);
-    memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
+    memory_region_add_subregion(address_space_mem, 0x80800000, mr);
 
     memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
                           "pci-intack", 1);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 954dd446fa..a13f879872 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -41,7 +41,6 @@ struct PCIHostState {
 
     MemoryRegion conf_mem;
     MemoryRegion data_mem;
-    MemoryRegion mmcfg;
     uint32_t config_reg;
     bool mig_enabled;
     PCIBus *bus;
-- 
2.41.3

Re: [PATCH v5 07/13] hw/pci-host/raven: Do not use parent object for mmcfg region

[Mark Cave-Ayland]

On 23/10/2025 16:26, BALATON Zoltan wrote:

> The mmcfg field in PCIHostState is only used by raven for the PCI
> config direct access but is not actually needed as the memory region
> lifetime can be managed by the object given during init so use that
> and remove the unused field from PCIHostState.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c       | 7 ++++---
>   include/hw/pci/pci_host.h | 1 -
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 2057a1869f..23020fd09f 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -216,7 +216,7 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>       PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
> -    MemoryRegion *address_space_mem = get_system_memory();
> +    MemoryRegion *mr, *address_space_mem = get_system_memory();
>   
>       qdev_init_gpio_in(d, raven_change_gpio, 1);
>   
> @@ -233,9 +233,10 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>                             "pci-conf-data", 4);
>       memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>   
> -    memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
> +    mr = g_new0(MemoryRegion, 1);
> +    memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
>                             "pci-mmcfg", 0x00400000);
> -    memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
> +    memory_region_add_subregion(address_space_mem, 0x80800000, mr);
>   
>       memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
>                             "pci-intack", 1);
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index 954dd446fa..a13f879872 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -41,7 +41,6 @@ struct PCIHostState {
>   
>       MemoryRegion conf_mem;
>       MemoryRegion data_mem;
> -    MemoryRegion mmcfg;
>       uint32_t config_reg;
>       bool mig_enabled;
>       PCIBus *bus;

Looking back at this patch in respect of patch 2, should RavenPCIState actually be 
kept and mmcfg moved there instead? It feels like the wrong approach simply because 
it isn't possible to access the MR directly from the device state when debugging.


ATB,

Mark.

Re: [PATCH v5 07/13] hw/pci-host/raven: Do not use parent object for mmcfg region

[BALATON Zoltan]

On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
> On 23/10/2025 16:26, BALATON Zoltan wrote:
>> The mmcfg field in PCIHostState is only used by raven for the PCI
>> config direct access but is not actually needed as the memory region
>> lifetime can be managed by the object given during init so use that
>> and remove the unused field from PCIHostState.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/pci-host/raven.c       | 7 ++++---
>>   include/hw/pci/pci_host.h | 1 -
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index 2057a1869f..23020fd09f 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -216,7 +216,7 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>> Error **errp)
>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>       PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
>> -    MemoryRegion *address_space_mem = get_system_memory();
>> +    MemoryRegion *mr, *address_space_mem = get_system_memory();
>>         qdev_init_gpio_in(d, raven_change_gpio, 1);
>>   @@ -233,9 +233,10 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>> Error **errp)
>>                             "pci-conf-data", 4);
>>       memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>>   -    memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, 
>> h->bus,
>> +    mr = g_new0(MemoryRegion, 1);
>> +    memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
>>                             "pci-mmcfg", 0x00400000);
>> -    memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
>> +    memory_region_add_subregion(address_space_mem, 0x80800000, mr);
>>         memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, 
>> s,
>>                             "pci-intack", 1);
>> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
>> index 954dd446fa..a13f879872 100644
>> --- a/include/hw/pci/pci_host.h
>> +++ b/include/hw/pci/pci_host.h
>> @@ -41,7 +41,6 @@ struct PCIHostState {
>>         MemoryRegion conf_mem;
>>       MemoryRegion data_mem;
>> -    MemoryRegion mmcfg;
>>       uint32_t config_reg;
>>       bool mig_enabled;
>>       PCIBus *bus;
>
> Looking back at this patch in respect of patch 2, should RavenPCIState 
> actually be kept and mmcfg moved there instead? It feels like the wrong 
> approach simply because it isn't possible to access the MR directly from the 
> device state when debugging.

No, that's not even the same object. What I removed in patch 2 is an empty 
subclass for the PCI facing part of the host bridge which is a subclass of 
PCIDevice and this is part of the sysbus facing part (PCIHostState) that 
already has a state struct but we don't need to store this memory region 
there. Not even for debugging, because all it does is just forwarding to 
PCI (look at the read/write ops) so you can fully debug it by -trace 
enable="pci*" or add debug logs to the read/write ops if you want. We also 
don't need to put it in a state struct to free it as 
memory_region_init_io(mr, OBJECT...) takes care of that and frees it with 
the OBJECT so I don't think what you say is a real concern or should block 
this patch.

Regards,
BALATON Zoltan

[PATCH v5 08/13] hw/pci-host/raven: Fix PCI config direct access region

[BALATON Zoltan]

The PCI configuration direct access region occupies 8 MiB at offset
0x800000 in PCI IO space so model that accordingly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/raven.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 23020fd09f..bb0be40eb4 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -235,8 +235,8 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 
     mr = g_new0(MemoryRegion, 1);
     memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
-                          "pci-mmcfg", 0x00400000);
-    memory_region_add_subregion(address_space_mem, 0x80800000, mr);
+                          "pci-mmcfg", 8 * MiB);
+    memory_region_add_subregion(&s->pci_io, 0x800000, mr);
 
     memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
                           "pci-intack", 1);
-- 
2.41.3

[PATCH v5 09/13] hw/pci-host/raven: Simpify discontiguous IO access

[BALATON Zoltan]

PREP allows remapping of the 64k ISA IO addresses from the normal
contiguous IO space into a discontiguous 8MB region and can switch
between the two modes. We can implement this in a simpler way than is
done currently using an io region that forwards access to the
contiguous pci_io region and enabling/disabling the discontiguous
region as needed.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c | 88 ++++++++++++---------------------------------
 1 file changed, 22 insertions(+), 66 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index bb0be40eb4..bf4f4b7f71 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -42,17 +42,14 @@ struct PREPPCIState {
     PCIHostState parent_obj;
 
     qemu_irq irq;
-    AddressSpace pci_io_as;
     MemoryRegion pci_io;
-    MemoryRegion pci_io_non_contiguous;
+    MemoryRegion pci_discontiguous_io;
     MemoryRegion pci_memory;
     MemoryRegion pci_intack;
     MemoryRegion bm;
     MemoryRegion bm_ram_alias;
     MemoryRegion bm_pci_memory_alias;
     AddressSpace bm_as;
-
-    int contiguous_map;
 };
 
 #define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
@@ -103,63 +100,28 @@ static const MemoryRegionOps raven_intack_ops = {
     },
 };
 
-static inline hwaddr raven_io_address(PREPPCIState *s,
-                                      hwaddr addr)
+/* Convert 8 MB non-contiguous address to 64k ISA IO address */
+static inline hwaddr raven_io_addr(hwaddr addr)
 {
-    if (s->contiguous_map == 0) {
-        /* 64 KB contiguous space for IOs */
-        addr &= 0xFFFF;
-    } else {
-        /* 8 MB non-contiguous space for IOs */
-        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
-    }
-
-    /* FIXME: handle endianness switch */
-
-    return addr;
+    return ((addr & 0x007FFF000) >> 7) | (addr & 0x1F);
 }
 
-static uint64_t raven_io_read(void *opaque, hwaddr addr,
-                              unsigned int size)
+static uint64_t raven_io_read(void *opaque, hwaddr addr, unsigned int size)
 {
-    PREPPCIState *s = opaque;
-    uint8_t buf[4];
-
-    addr = raven_io_address(s, addr);
-    address_space_read(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
-                       MEMTXATTRS_UNSPECIFIED, buf, size);
-
-    if (size == 1) {
-        return buf[0];
-    } else if (size == 2) {
-        return lduw_le_p(buf);
-    } else if (size == 4) {
-        return ldl_le_p(buf);
-    } else {
-        g_assert_not_reached();
-    }
+    uint64_t val = 0xffffffffULL;
+
+    memory_region_dispatch_read(opaque, raven_io_addr(addr), &val,
+                                size_memop(size) | MO_LE,
+                                MEMTXATTRS_UNSPECIFIED);
+    return val;
 }
 
-static void raven_io_write(void *opaque, hwaddr addr,
-                           uint64_t val, unsigned int size)
+static void raven_io_write(void *opaque, hwaddr addr, uint64_t val,
+                           unsigned int size)
 {
-    PREPPCIState *s = opaque;
-    uint8_t buf[4];
-
-    addr = raven_io_address(s, addr);
-
-    if (size == 1) {
-        buf[0] = val;
-    } else if (size == 2) {
-        stw_le_p(buf, val);
-    } else if (size == 4) {
-        stl_le_p(buf, val);
-    } else {
-        g_assert_not_reached();
-    }
-
-    address_space_write(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
-                        MEMTXATTRS_UNSPECIFIED, buf, size);
+    memory_region_dispatch_write(opaque, raven_io_addr(addr), val,
+                                 size_memop(size) | MO_LE,
+                                 MEMTXATTRS_UNSPECIFIED);
 }
 
 static const MemoryRegionOps raven_io_ops = {
@@ -208,7 +170,7 @@ static void raven_change_gpio(void *opaque, int n, int level)
 {
     PREPPCIState *s = opaque;
 
-    s->contiguous_map = level;
+    memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
 }
 
 static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
@@ -254,23 +216,17 @@ static void raven_pcihost_initfn(Object *obj)
     MemoryRegion *address_space_mem = get_system_memory();
 
     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
-    memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
-                          "pci-io-non-contiguous", 0x00800000);
+    memory_region_init_io(&s->pci_discontiguous_io, obj,
+                          &raven_io_ops, &s->pci_io,
+                          "pci-discontiguous-io", 8 * MiB);
     memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
-    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
-
-    /*
-     * Raven's raven_io_ops use the address-space API to access pci-conf-idx
-     * (which is also owned by the raven device). As such, mark the
-     * pci_io_non_contiguous as re-entrancy safe.
-     */
-    s->pci_io_non_contiguous.disable_reentrancy_guard = true;
 
     /* CPU address space */
     memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
                                 &s->pci_io);
     memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
-                                        &s->pci_io_non_contiguous, 1);
+                                        &s->pci_discontiguous_io, 1);
+    memory_region_set_enabled(&s->pci_discontiguous_io, false);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
 
     /* Bus master address space */
-- 
2.41.3

Re: [PATCH v5 09/13] hw/pci-host/raven: Simpify discontiguous IO access

[Mark Cave-Ayland]

On 23/10/2025 16:26, BALATON Zoltan wrote:

> PREP allows remapping of the 64k ISA IO addresses from the normal
> contiguous IO space into a discontiguous 8MB region and can switch
> between the two modes. We can implement this in a simpler way than is
> done currently using an io region that forwards access to the
> contiguous pci_io region and enabling/disabling the discontiguous
> region as needed.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 88 ++++++++++++---------------------------------
>   1 file changed, 22 insertions(+), 66 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index bb0be40eb4..bf4f4b7f71 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -42,17 +42,14 @@ struct PREPPCIState {
>       PCIHostState parent_obj;
>   
>       qemu_irq irq;
> -    AddressSpace pci_io_as;
>       MemoryRegion pci_io;
> -    MemoryRegion pci_io_non_contiguous;
> +    MemoryRegion pci_discontiguous_io;
>       MemoryRegion pci_memory;
>       MemoryRegion pci_intack;
>       MemoryRegion bm;
>       MemoryRegion bm_ram_alias;
>       MemoryRegion bm_pci_memory_alias;
>       AddressSpace bm_as;
> -
> -    int contiguous_map;
>   };
>   
>   #define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
> @@ -103,63 +100,28 @@ static const MemoryRegionOps raven_intack_ops = {
>       },
>   };
>   
> -static inline hwaddr raven_io_address(PREPPCIState *s,
> -                                      hwaddr addr)
> +/* Convert 8 MB non-contiguous address to 64k ISA IO address */
> +static inline hwaddr raven_io_addr(hwaddr addr)
>   {
> -    if (s->contiguous_map == 0) {
> -        /* 64 KB contiguous space for IOs */
> -        addr &= 0xFFFF;
> -    } else {
> -        /* 8 MB non-contiguous space for IOs */
> -        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
> -    }
> -
> -    /* FIXME: handle endianness switch */
> -
> -    return addr;
> +    return ((addr & 0x007FFF000) >> 7) | (addr & 0x1F);
>   }
>   
> -static uint64_t raven_io_read(void *opaque, hwaddr addr,
> -                              unsigned int size)
> +static uint64_t raven_io_read(void *opaque, hwaddr addr, unsigned int size)
>   {
> -    PREPPCIState *s = opaque;
> -    uint8_t buf[4];
> -
> -    addr = raven_io_address(s, addr);
> -    address_space_read(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
> -                       MEMTXATTRS_UNSPECIFIED, buf, size);
> -
> -    if (size == 1) {
> -        return buf[0];
> -    } else if (size == 2) {
> -        return lduw_le_p(buf);
> -    } else if (size == 4) {
> -        return ldl_le_p(buf);
> -    } else {
> -        g_assert_not_reached();
> -    }
> +    uint64_t val = 0xffffffffULL;
> +
> +    memory_region_dispatch_read(opaque, raven_io_addr(addr), &val,
> +                                size_memop(size) | MO_LE,
> +                                MEMTXATTRS_UNSPECIFIED);
> +    return val;
>   }
>   
> -static void raven_io_write(void *opaque, hwaddr addr,
> -                           uint64_t val, unsigned int size)
> +static void raven_io_write(void *opaque, hwaddr addr, uint64_t val,
> +                           unsigned int size)
>   {
> -    PREPPCIState *s = opaque;
> -    uint8_t buf[4];
> -
> -    addr = raven_io_address(s, addr);
> -
> -    if (size == 1) {
> -        buf[0] = val;
> -    } else if (size == 2) {
> -        stw_le_p(buf, val);
> -    } else if (size == 4) {
> -        stl_le_p(buf, val);
> -    } else {
> -        g_assert_not_reached();
> -    }
> -
> -    address_space_write(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
> -                        MEMTXATTRS_UNSPECIFIED, buf, size);
> +    memory_region_dispatch_write(opaque, raven_io_addr(addr), val,
> +                                 size_memop(size) | MO_LE,
> +                                 MEMTXATTRS_UNSPECIFIED);
>   }
>   
>   static const MemoryRegionOps raven_io_ops = {
> @@ -208,7 +170,7 @@ static void raven_change_gpio(void *opaque, int n, int level)
>   {
>       PREPPCIState *s = opaque;
>   
> -    s->contiguous_map = level;
> +    memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
>   }
>   
>   static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
> @@ -254,23 +216,17 @@ static void raven_pcihost_initfn(Object *obj)
>       MemoryRegion *address_space_mem = get_system_memory();
>   
>       memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
> -    memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
> -                          "pci-io-non-contiguous", 0x00800000);
> +    memory_region_init_io(&s->pci_discontiguous_io, obj,
> +                          &raven_io_ops, &s->pci_io,
> +                          "pci-discontiguous-io", 8 * MiB);
>       memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
> -    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
> -
> -    /*
> -     * Raven's raven_io_ops use the address-space API to access pci-conf-idx
> -     * (which is also owned by the raven device). As such, mark the
> -     * pci_io_non_contiguous as re-entrancy safe.
> -     */
> -    s->pci_io_non_contiguous.disable_reentrancy_guard = true;
>   
>       /* CPU address space */
>       memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
>                                   &s->pci_io);
>       memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
> -                                        &s->pci_io_non_contiguous, 1);
> +                                        &s->pci_discontiguous_io, 1);
> +    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>       memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
>   
>       /* Bus master address space */

I'm still not convinced by this one switching to use 
memory_region_{read,write}_dispatch() functions which do not handle all accesses 
correctly over the preferred address_space_{read,write} APIs. There might be some 
merit in using the address_space_ld*() and address_space_st*() versions though.


ATB,

Mark.

Re: [PATCH v5 09/13] hw/pci-host/raven: Simpify discontiguous IO access

[BALATON Zoltan]

On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
> On 23/10/2025 16:26, BALATON Zoltan wrote:
>> PREP allows remapping of the 64k ISA IO addresses from the normal
>> contiguous IO space into a discontiguous 8MB region and can switch
>> between the two modes. We can implement this in a simpler way than is
>> done currently using an io region that forwards access to the
>> contiguous pci_io region and enabling/disabling the discontiguous
>> region as needed.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/pci-host/raven.c | 88 ++++++++++++---------------------------------
>>   1 file changed, 22 insertions(+), 66 deletions(-)
>> 
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index bb0be40eb4..bf4f4b7f71 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -42,17 +42,14 @@ struct PREPPCIState {
>>       PCIHostState parent_obj;
>>         qemu_irq irq;
>> -    AddressSpace pci_io_as;
>>       MemoryRegion pci_io;
>> -    MemoryRegion pci_io_non_contiguous;
>> +    MemoryRegion pci_discontiguous_io;
>>       MemoryRegion pci_memory;
>>       MemoryRegion pci_intack;
>>       MemoryRegion bm;
>>       MemoryRegion bm_ram_alias;
>>       MemoryRegion bm_pci_memory_alias;
>>       AddressSpace bm_as;
>> -
>> -    int contiguous_map;
>>   };
>>     #define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus 
>> */
>> @@ -103,63 +100,28 @@ static const MemoryRegionOps raven_intack_ops = {
>>       },
>>   };
>>   -static inline hwaddr raven_io_address(PREPPCIState *s,
>> -                                      hwaddr addr)
>> +/* Convert 8 MB non-contiguous address to 64k ISA IO address */
>> +static inline hwaddr raven_io_addr(hwaddr addr)
>>   {
>> -    if (s->contiguous_map == 0) {
>> -        /* 64 KB contiguous space for IOs */
>> -        addr &= 0xFFFF;
>> -    } else {
>> -        /* 8 MB non-contiguous space for IOs */
>> -        addr = (addr & 0x1F) | ((addr & 0x007FFF000) >> 7);
>> -    }
>> -
>> -    /* FIXME: handle endianness switch */
>> -
>> -    return addr;
>> +    return ((addr & 0x007FFF000) >> 7) | (addr & 0x1F);
>>   }
>>   -static uint64_t raven_io_read(void *opaque, hwaddr addr,
>> -                              unsigned int size)
>> +static uint64_t raven_io_read(void *opaque, hwaddr addr, unsigned int 
>> size)
>>   {
>> -    PREPPCIState *s = opaque;
>> -    uint8_t buf[4];
>> -
>> -    addr = raven_io_address(s, addr);
>> -    address_space_read(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
>> -                       MEMTXATTRS_UNSPECIFIED, buf, size);
>> -
>> -    if (size == 1) {
>> -        return buf[0];
>> -    } else if (size == 2) {
>> -        return lduw_le_p(buf);
>> -    } else if (size == 4) {
>> -        return ldl_le_p(buf);
>> -    } else {
>> -        g_assert_not_reached();
>> -    }
>> +    uint64_t val = 0xffffffffULL;
>> +
>> +    memory_region_dispatch_read(opaque, raven_io_addr(addr), &val,
>> +                                size_memop(size) | MO_LE,
>> +                                MEMTXATTRS_UNSPECIFIED);
>> +    return val;
>>   }
>>   -static void raven_io_write(void *opaque, hwaddr addr,
>> -                           uint64_t val, unsigned int size)
>> +static void raven_io_write(void *opaque, hwaddr addr, uint64_t val,
>> +                           unsigned int size)
>>   {
>> -    PREPPCIState *s = opaque;
>> -    uint8_t buf[4];
>> -
>> -    addr = raven_io_address(s, addr);
>> -
>> -    if (size == 1) {
>> -        buf[0] = val;
>> -    } else if (size == 2) {
>> -        stw_le_p(buf, val);
>> -    } else if (size == 4) {
>> -        stl_le_p(buf, val);
>> -    } else {
>> -        g_assert_not_reached();
>> -    }
>> -
>> -    address_space_write(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
>> -                        MEMTXATTRS_UNSPECIFIED, buf, size);
>> +    memory_region_dispatch_write(opaque, raven_io_addr(addr), val,
>> +                                 size_memop(size) | MO_LE,
>> +                                 MEMTXATTRS_UNSPECIFIED);
>>   }
>>     static const MemoryRegionOps raven_io_ops = {
>> @@ -208,7 +170,7 @@ static void raven_change_gpio(void *opaque, int n, int 
>> level)
>>   {
>>       PREPPCIState *s = opaque;
>>   -    s->contiguous_map = level;
>> +    memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
>>   }
>>     static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>> @@ -254,23 +216,17 @@ static void raven_pcihost_initfn(Object *obj)
>>       MemoryRegion *address_space_mem = get_system_memory();
>>         memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>> -    memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, 
>> s,
>> -                          "pci-io-non-contiguous", 0x00800000);
>> +    memory_region_init_io(&s->pci_discontiguous_io, obj,
>> +                          &raven_io_ops, &s->pci_io,
>> +                          "pci-discontiguous-io", 8 * MiB);
>>       memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
>> -    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
>> -
>> -    /*
>> -     * Raven's raven_io_ops use the address-space API to access 
>> pci-conf-idx
>> -     * (which is also owned by the raven device). As such, mark the
>> -     * pci_io_non_contiguous as re-entrancy safe.
>> -     */
>> -    s->pci_io_non_contiguous.disable_reentrancy_guard = true;
>>         /* CPU address space */
>>       memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
>>                                   &s->pci_io);
>>       memory_region_add_subregion_overlap(address_space_mem, 
>> PCI_IO_BASE_ADDR,
>> -                                        &s->pci_io_non_contiguous, 1);
>> +                                        &s->pci_discontiguous_io, 1);
>> +    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>>       memory_region_add_subregion(address_space_mem, 0xc0000000, 
>> &s->pci_memory);
>>         /* Bus master address space */
>
> I'm still not convinced by this one switching to use 
> memory_region_{read,write}_dispatch() functions which do not handle all 
> accesses correctly over the preferred address_space_{read,write} APIs. There 
> might be some merit in using the address_space_ld*() and address_space_st*() 
> versions though.

Which accesses does it not handle correctly and what makes address_space_* 
functions preferred over accessing the memory region?

Regards,
BALATON Zoltan

[PATCH v5 10/13] hw/pci-host/raven: Move bus master address space creation to one place

[BALATON Zoltan]

Move the lines related to creating the bus master address space
together and reduce the number of memory regions stored in the device
state. These are used once to create the address space and can be
tracked with their owner object so no need to keep track of them in
the device state. Keep only the address space that is used later in a
callback.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index bf4f4b7f71..ebf0c511dc 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -46,9 +46,6 @@ struct PREPPCIState {
     MemoryRegion pci_discontiguous_io;
     MemoryRegion pci_memory;
     MemoryRegion pci_intack;
-    MemoryRegion bm;
-    MemoryRegion bm_ram_alias;
-    MemoryRegion bm_pci_memory_alias;
     AddressSpace bm_as;
 };
 
@@ -178,7 +175,8 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
-    MemoryRegion *mr, *address_space_mem = get_system_memory();
+    Object *o = OBJECT(d);
+    MemoryRegion *mr, *bm, *address_space_mem = get_system_memory();
 
     qdev_init_gpio_in(d, raven_change_gpio, 1);
 
@@ -187,26 +185,37 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
                                    &s->irq, &s->pci_memory, &s->pci_io, 0, 1,
                                    TYPE_PCI_BUS);
 
-    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
+    memory_region_init_io(&h->conf_mem, o, &pci_host_conf_le_ops, s,
                           "pci-conf-idx", 4);
     memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
 
-    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
+    memory_region_init_io(&h->data_mem, o, &pci_host_data_le_ops, s,
                           "pci-conf-data", 4);
     memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
 
     mr = g_new0(MemoryRegion, 1);
-    memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
+    memory_region_init_io(mr, o, &raven_mmcfg_ops, h->bus,
                           "pci-mmcfg", 8 * MiB);
     memory_region_add_subregion(&s->pci_io, 0x800000, mr);
 
-    memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
+    memory_region_init_io(&s->pci_intack, o, &raven_intack_ops, s,
                           "pci-intack", 1);
     memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
 
     pci_create_simple(h->bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
 
-    address_space_init(&s->bm_as, &s->bm, "raven-bm");
+    /* Bus master address space */
+    bm = g_new0(MemoryRegion, 1);
+    memory_region_init(bm, o, "raven-bm", 4 * GiB);
+    mr = g_new0(MemoryRegion, 1);
+    memory_region_init_alias(mr, o, "bm-pci-memory", &s->pci_memory, 0,
+                             memory_region_size(&s->pci_memory));
+    memory_region_add_subregion(bm, 0, mr);
+    mr = g_new0(MemoryRegion, 1);
+    memory_region_init_alias(mr, o, "bm-system", get_system_memory(),
+                             0, 0x80000000);
+    memory_region_add_subregion(bm, 0x80000000, mr);
+    address_space_init(&s->bm_as, bm, "raven-bm-as");
     pci_setup_iommu(h->bus, &raven_iommu_ops, s);
 }
 
@@ -228,16 +237,6 @@ static void raven_pcihost_initfn(Object *obj)
                                         &s->pci_discontiguous_io, 1);
     memory_region_set_enabled(&s->pci_discontiguous_io, false);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-
-    /* Bus master address space */
-    memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
-    memory_region_init_alias(&s->bm_pci_memory_alias, obj, "bm-pci-memory",
-                             &s->pci_memory, 0,
-                             memory_region_size(&s->pci_memory));
-    memory_region_init_alias(&s->bm_ram_alias, obj, "bm-system",
-                             get_system_memory(), 0, 0x80000000);
-    memory_region_add_subregion(&s->bm, 0         , &s->bm_pci_memory_alias);
-    memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
 }
 
 static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
-- 
2.41.3

Re: [PATCH v5 10/13] hw/pci-host/raven: Move bus master address space creation to one place

[Mark Cave-Ayland]

On 23/10/2025 16:26, BALATON Zoltan wrote:

> Move the lines related to creating the bus master address space
> together and reduce the number of memory regions stored in the device
> state. These are used once to create the address space and can be
> tracked with their owner object so no need to keep track of them in
> the device state. Keep only the address space that is used later in a
> callback.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 37 ++++++++++++++++++-------------------
>   1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index bf4f4b7f71..ebf0c511dc 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -46,9 +46,6 @@ struct PREPPCIState {
>       MemoryRegion pci_discontiguous_io;
>       MemoryRegion pci_memory;
>       MemoryRegion pci_intack;
> -    MemoryRegion bm;
> -    MemoryRegion bm_ram_alias;
> -    MemoryRegion bm_pci_memory_alias;
>       AddressSpace bm_as;
>   };
>   
> @@ -178,7 +175,8 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>       PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
> -    MemoryRegion *mr, *address_space_mem = get_system_memory();
> +    Object *o = OBJECT(d);
> +    MemoryRegion *mr, *bm, *address_space_mem = get_system_memory();
>   
>       qdev_init_gpio_in(d, raven_change_gpio, 1);
>   
> @@ -187,26 +185,37 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>                                      &s->irq, &s->pci_memory, &s->pci_io, 0, 1,
>                                      TYPE_PCI_BUS);
>   
> -    memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
> +    memory_region_init_io(&h->conf_mem, o, &pci_host_conf_le_ops, s,
>                             "pci-conf-idx", 4);
>       memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
>   
> -    memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
> +    memory_region_init_io(&h->data_mem, o, &pci_host_data_le_ops, s,
>                             "pci-conf-data", 4);
>       memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>   
>       mr = g_new0(MemoryRegion, 1);
> -    memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
> +    memory_region_init_io(mr, o, &raven_mmcfg_ops, h->bus,
>                             "pci-mmcfg", 8 * MiB);
>       memory_region_add_subregion(&s->pci_io, 0x800000, mr);
>   
> -    memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
> +    memory_region_init_io(&s->pci_intack, o, &raven_intack_ops, s,
>                             "pci-intack", 1);
>       memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
>   
>       pci_create_simple(h->bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
>   
> -    address_space_init(&s->bm_as, &s->bm, "raven-bm");
> +    /* Bus master address space */
> +    bm = g_new0(MemoryRegion, 1);
> +    memory_region_init(bm, o, "raven-bm", 4 * GiB);
> +    mr = g_new0(MemoryRegion, 1);
> +    memory_region_init_alias(mr, o, "bm-pci-memory", &s->pci_memory, 0,
> +                             memory_region_size(&s->pci_memory));
> +    memory_region_add_subregion(bm, 0, mr);
> +    mr = g_new0(MemoryRegion, 1);
> +    memory_region_init_alias(mr, o, "bm-system", get_system_memory(),
> +                             0, 0x80000000);
> +    memory_region_add_subregion(bm, 0x80000000, mr);
> +    address_space_init(&s->bm_as, bm, "raven-bm-as");
>       pci_setup_iommu(h->bus, &raven_iommu_ops, s);
>   }
>   
> @@ -228,16 +237,6 @@ static void raven_pcihost_initfn(Object *obj)
>                                           &s->pci_discontiguous_io, 1);
>       memory_region_set_enabled(&s->pci_discontiguous_io, false);
>       memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
> -
> -    /* Bus master address space */
> -    memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
> -    memory_region_init_alias(&s->bm_pci_memory_alias, obj, "bm-pci-memory",
> -                             &s->pci_memory, 0,
> -                             memory_region_size(&s->pci_memory));
> -    memory_region_init_alias(&s->bm_ram_alias, obj, "bm-system",
> -                             get_system_memory(), 0, 0x80000000);
> -    memory_region_add_subregion(&s->bm, 0         , &s->bm_pci_memory_alias);
> -    memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
>   }
>   
>   static void raven_pcihost_class_init(ObjectClass *klass, const void *data)

Again I don't think this is helpful as per my comments on patch 7.


ATB,

Mark.

[PATCH v5 11/13] hw/pci-host/raven: Do not map regions in init method

[BALATON Zoltan]

Export memory regions as sysbus mmio regions and let the board code
map them similar to how it is done in grackle. While at it rename
raven_pcihost_realizefn to raven_pcihost_realize.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/raven.c | 38 +++++++++++++-------------------------
 hw/ppc/prep.c       | 10 ++++++++--
 2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index ebf0c511dc..1e36a637a6 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -49,8 +49,6 @@ struct PREPPCIState {
     AddressSpace bm_as;
 };
 
-#define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
-
 static inline uint32_t raven_idsel_to_addr(hwaddr addr)
 {
     return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
@@ -170,7 +168,7 @@ static void raven_change_gpio(void *opaque, int n, int level)
     memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
 }
 
-static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
+static void raven_pcihost_realize(DeviceState *d, Error **errp)
 {
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
@@ -180,7 +178,18 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
 
     qdev_init_gpio_in(d, raven_change_gpio, 1);
 
+    memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
+    memory_region_init_io(&s->pci_discontiguous_io, o,
+                          &raven_io_ops, &s->pci_io,
+                          "pci-discontiguous-io", 8 * MiB);
+    memory_region_set_enabled(&s->pci_discontiguous_io, false);
+    memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
+
+    sysbus_init_mmio(dev, &s->pci_io);
+    sysbus_init_mmio(dev, &s->pci_discontiguous_io);
+    sysbus_init_mmio(dev, &s->pci_memory);
     sysbus_init_irq(dev, &s->irq);
+
     h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,
                                    &s->irq, &s->pci_memory, &s->pci_io, 0, 1,
                                    TYPE_PCI_BUS);
@@ -219,32 +228,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     pci_setup_iommu(h->bus, &raven_iommu_ops, s);
 }
 
-static void raven_pcihost_initfn(Object *obj)
-{
-    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
-    MemoryRegion *address_space_mem = get_system_memory();
-
-    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
-    memory_region_init_io(&s->pci_discontiguous_io, obj,
-                          &raven_io_ops, &s->pci_io,
-                          "pci-discontiguous-io", 8 * MiB);
-    memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
-
-    /* CPU address space */
-    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
-                                &s->pci_io);
-    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
-                                        &s->pci_discontiguous_io, 1);
-    memory_region_set_enabled(&s->pci_discontiguous_io, false);
-    memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-}
-
 static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->realize = raven_pcihost_realizefn;
+    dc->realize = raven_pcihost_realize;
     dc->fw_name = "pci";
 }
 
@@ -278,7 +267,6 @@ static const TypeInfo raven_types[] = {
         .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
         .parent = TYPE_PCI_HOST_BRIDGE,
         .instance_size = sizeof(PREPPCIState),
-        .instance_init = raven_pcihost_initfn,
         .class_init = raven_pcihost_class_init,
     },
     {
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 816455d289..973d2fb7eb 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -53,8 +53,11 @@
 
 #define CFG_ADDR 0xf0000510
 
-#define KERNEL_LOAD_ADDR 0x01000000
-#define INITRD_LOAD_ADDR 0x01800000
+#define KERNEL_LOAD_ADDR  0x01000000
+#define INITRD_LOAD_ADDR  0x01800000
+
+#define PCI_IO_BASE_ADDR  0x80000000
+#define PCI_MEM_BASE_ADDR 0xc0000000
 
 #define BIOS_ADDR         0xfff00000
 #define BIOS_SIZE         (1 * MiB)
@@ -293,6 +296,9 @@ static void ibm_40p_init(MachineState *machine)
     pcihost = SYS_BUS_DEVICE(dev);
     object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev));
     sysbus_realize_and_unref(pcihost, &error_fatal);
+    sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
+    sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
+    sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
     pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
     if (!pci_bus) {
         error_report("could not create PCI host controller");
-- 
2.41.3

Re: [PATCH v5 11/13] hw/pci-host/raven: Do not map regions in init method

[Mark Cave-Ayland]

On 23/10/2025 16:26, BALATON Zoltan wrote:

> Export memory regions as sysbus mmio regions and let the board code
> map them similar to how it is done in grackle. While at it rename
> raven_pcihost_realizefn to raven_pcihost_realize.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/raven.c | 38 +++++++++++++-------------------------
>   hw/ppc/prep.c       | 10 ++++++++--
>   2 files changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index ebf0c511dc..1e36a637a6 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -49,8 +49,6 @@ struct PREPPCIState {
>       AddressSpace bm_as;
>   };
>   
> -#define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
> -
>   static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>   {
>       return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
> @@ -170,7 +168,7 @@ static void raven_change_gpio(void *opaque, int n, int level)
>       memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
>   }
>   
> -static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
> +static void raven_pcihost_realize(DeviceState *d, Error **errp)
>   {
>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
> @@ -180,7 +178,18 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>   
>       qdev_init_gpio_in(d, raven_change_gpio, 1);
>   
> +    memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
> +    memory_region_init_io(&s->pci_discontiguous_io, o,
> +                          &raven_io_ops, &s->pci_io,
> +                          "pci-discontiguous-io", 8 * MiB);
> +    memory_region_set_enabled(&s->pci_discontiguous_io, false);
> +    memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
> +
> +    sysbus_init_mmio(dev, &s->pci_io);
> +    sysbus_init_mmio(dev, &s->pci_discontiguous_io);
> +    sysbus_init_mmio(dev, &s->pci_memory);
>       sysbus_init_irq(dev, &s->irq);
> +
>       h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,
>                                      &s->irq, &s->pci_memory, &s->pci_io, 0, 1,
>                                      TYPE_PCI_BUS);
> @@ -219,32 +228,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>       pci_setup_iommu(h->bus, &raven_iommu_ops, s);
>   }
>   
> -static void raven_pcihost_initfn(Object *obj)
> -{
> -    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
> -    MemoryRegion *address_space_mem = get_system_memory();
> -
> -    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
> -    memory_region_init_io(&s->pci_discontiguous_io, obj,
> -                          &raven_io_ops, &s->pci_io,
> -                          "pci-discontiguous-io", 8 * MiB);
> -    memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
> -
> -    /* CPU address space */
> -    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
> -                                &s->pci_io);
> -    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
> -                                        &s->pci_discontiguous_io, 1);
> -    memory_region_set_enabled(&s->pci_discontiguous_io, false);
> -    memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
> -}
> -
>   static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    dc->realize = raven_pcihost_realizefn;
> +    dc->realize = raven_pcihost_realize;
>       dc->fw_name = "pci";
>   }
>   
> @@ -278,7 +267,6 @@ static const TypeInfo raven_types[] = {
>           .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
>           .parent = TYPE_PCI_HOST_BRIDGE,
>           .instance_size = sizeof(PREPPCIState),
> -        .instance_init = raven_pcihost_initfn,
>           .class_init = raven_pcihost_class_init,
>       },
>       {
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 816455d289..973d2fb7eb 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -53,8 +53,11 @@
>   
>   #define CFG_ADDR 0xf0000510
>   
> -#define KERNEL_LOAD_ADDR 0x01000000
> -#define INITRD_LOAD_ADDR 0x01800000
> +#define KERNEL_LOAD_ADDR  0x01000000
> +#define INITRD_LOAD_ADDR  0x01800000
> +
> +#define PCI_IO_BASE_ADDR  0x80000000
> +#define PCI_MEM_BASE_ADDR 0xc0000000
>   
>   #define BIOS_ADDR         0xfff00000
>   #define BIOS_SIZE         (1 * MiB)
> @@ -293,6 +296,9 @@ static void ibm_40p_init(MachineState *machine)
>       pcihost = SYS_BUS_DEVICE(dev);
>       object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev));
>       sysbus_realize_and_unref(pcihost, &error_fatal);
> +    sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
> +    sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
> +    sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>       if (!pci_bus) {
>           error_report("could not create PCI host controller");

In general the expectation is that memory regions should be initialised in the 
_init() function, unless they depend upon a property in which case they should be 
initialised in the _realize() function. Why do you feel this needs to be different?


ATB,

Mark.

Re: [PATCH v5 11/13] hw/pci-host/raven: Do not map regions in init method

[BALATON Zoltan]

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

On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
> On 23/10/2025 16:26, BALATON Zoltan wrote:
>> Export memory regions as sysbus mmio regions and let the board code
>> map them similar to how it is done in grackle. While at it rename
>> raven_pcihost_realizefn to raven_pcihost_realize.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/pci-host/raven.c | 38 +++++++++++++-------------------------
>>   hw/ppc/prep.c       | 10 ++++++++--
>>   2 files changed, 21 insertions(+), 27 deletions(-)
>> 
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index ebf0c511dc..1e36a637a6 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -49,8 +49,6 @@ struct PREPPCIState {
>>       AddressSpace bm_as;
>>   };
>>   -#define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus 
>> */
>> -
>>   static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>>   {
>>       return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
>> @@ -170,7 +168,7 @@ static void raven_change_gpio(void *opaque, int n, int 
>> level)
>>       memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
>>   }
>>   -static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>> +static void raven_pcihost_realize(DeviceState *d, Error **errp)
>>   {
>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>> @@ -180,7 +178,18 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>> Error **errp)
>>         qdev_init_gpio_in(d, raven_change_gpio, 1);
>>   +    memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
>> +    memory_region_init_io(&s->pci_discontiguous_io, o,
>> +                          &raven_io_ops, &s->pci_io,
>> +                          "pci-discontiguous-io", 8 * MiB);
>> +    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>> +    memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
>> +
>> +    sysbus_init_mmio(dev, &s->pci_io);
>> +    sysbus_init_mmio(dev, &s->pci_discontiguous_io);
>> +    sysbus_init_mmio(dev, &s->pci_memory);
>>       sysbus_init_irq(dev, &s->irq);
>> +
>>       h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,
>>                                      &s->irq, &s->pci_memory, &s->pci_io, 
>> 0, 1,
>>                                      TYPE_PCI_BUS);
>> @@ -219,32 +228,12 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>> Error **errp)
>>       pci_setup_iommu(h->bus, &raven_iommu_ops, s);
>>   }
>>   -static void raven_pcihost_initfn(Object *obj)
>> -{
>> -    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>> -    MemoryRegion *address_space_mem = get_system_memory();
>> -
>> -    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>> -    memory_region_init_io(&s->pci_discontiguous_io, obj,
>> -                          &raven_io_ops, &s->pci_io,
>> -                          "pci-discontiguous-io", 8 * MiB);
>> -    memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
>> -
>> -    /* CPU address space */
>> -    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
>> -                                &s->pci_io);
>> -    memory_region_add_subregion_overlap(address_space_mem, 
>> PCI_IO_BASE_ADDR,
>> -                                        &s->pci_discontiguous_io, 1);
>> -    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>> -    memory_region_add_subregion(address_space_mem, 0xc0000000, 
>> &s->pci_memory);
>> -}
>> -
>>   static void raven_pcihost_class_init(ObjectClass *klass, const void 
>> *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>         set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> -    dc->realize = raven_pcihost_realizefn;
>> +    dc->realize = raven_pcihost_realize;
>>       dc->fw_name = "pci";
>>   }
>>   @@ -278,7 +267,6 @@ static const TypeInfo raven_types[] = {
>>           .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
>>           .parent = TYPE_PCI_HOST_BRIDGE,
>>           .instance_size = sizeof(PREPPCIState),
>> -        .instance_init = raven_pcihost_initfn,
>>           .class_init = raven_pcihost_class_init,
>>       },
>>       {
>> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
>> index 816455d289..973d2fb7eb 100644
>> --- a/hw/ppc/prep.c
>> +++ b/hw/ppc/prep.c
>> @@ -53,8 +53,11 @@
>>     #define CFG_ADDR 0xf0000510
>>   -#define KERNEL_LOAD_ADDR 0x01000000
>> -#define INITRD_LOAD_ADDR 0x01800000
>> +#define KERNEL_LOAD_ADDR  0x01000000
>> +#define INITRD_LOAD_ADDR  0x01800000
>> +
>> +#define PCI_IO_BASE_ADDR  0x80000000
>> +#define PCI_MEM_BASE_ADDR 0xc0000000
>>     #define BIOS_ADDR         0xfff00000
>>   #define BIOS_SIZE         (1 * MiB)
>> @@ -293,6 +296,9 @@ static void ibm_40p_init(MachineState *machine)
>>       pcihost = SYS_BUS_DEVICE(dev);
>>       object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev));
>>       sysbus_realize_and_unref(pcihost, &error_fatal);
>> +    sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
>> +    sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
>> +    sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
>>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>>       if (!pci_bus) {
>>           error_report("could not create PCI host controller");
>
> In general the expectation is that memory regions should be initialised in 
> the _init() function, unless they depend upon a property in which case they 
> should be initialised in the _realize() function. Why do you feel this needs 
> to be different?

Is any of it needed before realize? If not why have an init method at all? 
As shown here this works perfectly without one and is more comprehensible 
that way for people reading it without deep knowledge about Qdev. In 
general I think simple devices only need a realize method and the init 
method is rarely needed, e.g. if there are some child objects that need to 
be init for passing properties that can be set before realize or similar 
unusual cases but for most classes init is not needed at all. I only want 
to keep what's necessary and remove everything that's not needed. I think 
that makes the device model easier to understand.

Regards,
BALATON Zoltan

Re: [PATCH v5 11/13] hw/pci-host/raven: Do not map regions in init method

[BALATON Zoltan]

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

Hello,

Added a few more people to cc hoping to get some opinion to clear this up. 
This is brought up by my patch trying to simplify hw/pci-host/raven.c part 
of this series:
https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/
(First submitted in May here:
https://patchew.org/QEMU/cover.1746374076.git.balaton@eik.bme.hu/
but that went relatively unnoticed and missed the previous release.)
Find discussion below the patch.

On Sat, 25 Oct 2025, BALATON Zoltan wrote:
> On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
>> On 23/10/2025 16:26, BALATON Zoltan wrote:
>>> Export memory regions as sysbus mmio regions and let the board code
>>> map them similar to how it is done in grackle. While at it rename
>>> raven_pcihost_realizefn to raven_pcihost_realize.
>>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/pci-host/raven.c | 38 +++++++++++++-------------------------
>>>   hw/ppc/prep.c       | 10 ++++++++--
>>>   2 files changed, 21 insertions(+), 27 deletions(-)
>>> 
>>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>>> index ebf0c511dc..1e36a637a6 100644
>>> --- a/hw/pci-host/raven.c
>>> +++ b/hw/pci-host/raven.c
>>> @@ -49,8 +49,6 @@ struct PREPPCIState {
>>>       AddressSpace bm_as;
>>>   };
>>>   -#define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus 
>>> */
>>> -
>>>   static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>>>   {
>>>       return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
>>> @@ -170,7 +168,7 @@ static void raven_change_gpio(void *opaque, int n, int 
>>> level)
>>>       memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
>>>   }
>>>   -static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>> +static void raven_pcihost_realize(DeviceState *d, Error **errp)
>>>   {
>>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>>> @@ -180,7 +178,18 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>>> Error **errp)
>>>         qdev_init_gpio_in(d, raven_change_gpio, 1);
>>>   +    memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
>>> +    memory_region_init_io(&s->pci_discontiguous_io, o,
>>> +                          &raven_io_ops, &s->pci_io,
>>> +                          "pci-discontiguous-io", 8 * MiB);
>>> +    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>>> +    memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
>>> +
>>> +    sysbus_init_mmio(dev, &s->pci_io);
>>> +    sysbus_init_mmio(dev, &s->pci_discontiguous_io);
>>> +    sysbus_init_mmio(dev, &s->pci_memory);
>>>       sysbus_init_irq(dev, &s->irq);
>>> +
>>>       h->bus = pci_register_root_bus(d, NULL, raven_set_irq, 
>>> raven_map_irq,
>>>                                      &s->irq, &s->pci_memory, &s->pci_io, 
>>> 0, 1,
>>>                                      TYPE_PCI_BUS);
>>> @@ -219,32 +228,12 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>>> Error **errp)
>>>       pci_setup_iommu(h->bus, &raven_iommu_ops, s);
>>>   }
>>>   -static void raven_pcihost_initfn(Object *obj)
>>> -{
>>> -    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>>> -    MemoryRegion *address_space_mem = get_system_memory();
>>> -
>>> -    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>>> -    memory_region_init_io(&s->pci_discontiguous_io, obj,
>>> -                          &raven_io_ops, &s->pci_io,
>>> -                          "pci-discontiguous-io", 8 * MiB);
>>> -    memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
>>> -
>>> -    /* CPU address space */
>>> -    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
>>> -                                &s->pci_io);
>>> -    memory_region_add_subregion_overlap(address_space_mem, 
>>> PCI_IO_BASE_ADDR,
>>> -                                        &s->pci_discontiguous_io, 1);
>>> -    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>>> -    memory_region_add_subregion(address_space_mem, 0xc0000000, 
>>> &s->pci_memory);
>>> -}
>>> -
>>>   static void raven_pcihost_class_init(ObjectClass *klass, const void 
>>> *data)
>>>   {
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>         set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>> -    dc->realize = raven_pcihost_realizefn;
>>> +    dc->realize = raven_pcihost_realize;
>>>       dc->fw_name = "pci";
>>>   }
>>>   @@ -278,7 +267,6 @@ static const TypeInfo raven_types[] = {
>>>           .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
>>>           .parent = TYPE_PCI_HOST_BRIDGE,
>>>           .instance_size = sizeof(PREPPCIState),
>>> -        .instance_init = raven_pcihost_initfn,
>>>           .class_init = raven_pcihost_class_init,
>>>       },
>>>       {
>>> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
>>> index 816455d289..973d2fb7eb 100644
>>> --- a/hw/ppc/prep.c
>>> +++ b/hw/ppc/prep.c
>>> @@ -53,8 +53,11 @@
>>>     #define CFG_ADDR 0xf0000510
>>>   -#define KERNEL_LOAD_ADDR 0x01000000
>>> -#define INITRD_LOAD_ADDR 0x01800000
>>> +#define KERNEL_LOAD_ADDR  0x01000000
>>> +#define INITRD_LOAD_ADDR  0x01800000
>>> +
>>> +#define PCI_IO_BASE_ADDR  0x80000000
>>> +#define PCI_MEM_BASE_ADDR 0xc0000000
>>>     #define BIOS_ADDR         0xfff00000
>>>   #define BIOS_SIZE         (1 * MiB)
>>> @@ -293,6 +296,9 @@ static void ibm_40p_init(MachineState *machine)
>>>       pcihost = SYS_BUS_DEVICE(dev);
>>>       object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev));
>>>       sysbus_realize_and_unref(pcihost, &error_fatal);
>>> +    sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
>>> +    sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
>>> +    sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
>>>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>>>       if (!pci_bus) {
>>>           error_report("could not create PCI host controller");
>> 
>> In general the expectation is that memory regions should be initialised in 
>> the _init() function, unless they depend upon a property in which case they 
>> should be initialised in the _realize() function. Why do you feel this 
>> needs to be different?
>
> Is any of it needed before realize? If not why have an init method at all? As 
> shown here this works perfectly without one and is more comprehensible that 
> way for people reading it without deep knowledge about Qdev. In general I 
> think simple devices only need a realize method and the init method is rarely 
> needed, e.g. if there are some child objects that need to be init for passing 
> properties that can be set before realize or similar unusual cases but for 
> most classes init is not needed at all. I only want to keep what's necessary 
> and remove everything that's not needed. I think that makes the device model 
> easier to understand.

I've checked documentation here:
https://www.qemu.org/docs/master/devel/qdev-api.html
but it's not really clear on what's the preferred way of using init and 
realize. It's not even very clear on when to use which to me. So becuase 
that did not help I did a quick survey on what other pci-host models do. 
Of the 32 .c files in hw/pci-host 16 have an init method:

aspeed_pcie.c, astro.c, designware.c, gpex.c, grackle.c, i440fx.c, 
pnv_phb3.c, pnv_phb3_msi.c, pnv_phb3_pbcq.c, pnv_phb4.c, q35.c, raven.c, 
sabre.c, uninorth.c, versatile.c, xilinx-pcie.c

Of these astro.c has an empty init function that should be removed; 
grackle.c, sabre.c and uninorth.c are maintained by you so I'll ignore 
them here; we're discussing raven.c now and i440fx.c has two 
memory_region_init_io calls in init that could be in realize where all 
others are and otherwise all other models do this in realize and only init 
child objects and add properties in init methods when that's needed 
because they need to be available before realize. The other 16 device 
models don't have an init method at all and do all in the realize like I 
proposed in this patch for raven. Since only device models that you 
maintain do it differently I think what you say is not following the 
preferred way so you should not block this patch.

I'd be interested if there is a consensus on this or can we cone to one 
that we can document to avoid this repeating every time.

Regards,
BALATON Zoltan

Re: [PATCH v5 11/13] hw/pci-host/raven: Do not map regions in init method

[Philippe Mathieu-Daudé]

On 25/10/25 16:49, BALATON Zoltan wrote:
> Hello,
> 
> Added a few more people to cc hoping to get some opinion to clear this 
> up. This is brought up by my patch trying to simplify hw/pci-host/ 
> raven.c part of this series:
> https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/
> (First submitted in May here:
> https://patchew.org/QEMU/cover.1746374076.git.balaton@eik.bme.hu/
> but that went relatively unnoticed and missed the previous release.)
> Find discussion below the patch.
> 
> On Sat, 25 Oct 2025, BALATON Zoltan wrote:
>> On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
>>> On 23/10/2025 16:26, BALATON Zoltan wrote:
>>>> Export memory regions as sysbus mmio regions and let the board code
>>>> map them similar to how it is done in grackle. While at it rename
>>>> raven_pcihost_realizefn to raven_pcihost_realize.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   hw/pci-host/raven.c | 38 +++++++++++++-------------------------
>>>>   hw/ppc/prep.c       | 10 ++++++++--
>>>>   2 files changed, 21 insertions(+), 27 deletions(-)


>>>> @@ -180,7 +178,18 @@ static void raven_pcihost_realizefn(DeviceState 
>>>> *d, Error **errp)
>>>>         qdev_init_gpio_in(d, raven_change_gpio, 1);
>>>>   +    memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
>>>> +    memory_region_init_io(&s->pci_discontiguous_io, o,
>>>> +                          &raven_io_ops, &s->pci_io,
>>>> +                          "pci-discontiguous-io", 8 * MiB);
>>>> +    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>>>> +    memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
>>>> +
>>>> +    sysbus_init_mmio(dev, &s->pci_io);
>>>> +    sysbus_init_mmio(dev, &s->pci_discontiguous_io);
>>>> +    sysbus_init_mmio(dev, &s->pci_memory);
>>>>       sysbus_init_irq(dev, &s->irq);
>>>> +
>>>>       h->bus = pci_register_root_bus(d, NULL, raven_set_irq, 
>>>> raven_map_irq,
>>>>                                      &s->irq, &s->pci_memory, &s- 
>>>> >pci_io, 0, 1,
>>>>                                      TYPE_PCI_BUS);
>>>> @@ -219,32 +228,12 @@ static void 
>>>> raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>>>       pci_setup_iommu(h->bus, &raven_iommu_ops, s);
>>>>   }
>>>>   -static void raven_pcihost_initfn(Object *obj)
>>>> -{
>>>> -    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>>>> -    MemoryRegion *address_space_mem = get_system_memory();
>>>> -
>>>> -    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>>>> -    memory_region_init_io(&s->pci_discontiguous_io, obj,
>>>> -                          &raven_io_ops, &s->pci_io,
>>>> -                          "pci-discontiguous-io", 8 * MiB);
>>>> -    memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
>>>> -
>>>> -    /* CPU address space */
>>>> -    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
>>>> -                                &s->pci_io);
>>>> -    memory_region_add_subregion_overlap(address_space_mem, 
>>>> PCI_IO_BASE_ADDR,
>>>> -                                        &s->pci_discontiguous_io, 1);
>>>> -    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>>>> -    memory_region_add_subregion(address_space_mem, 0xc0000000, &s- 
>>>> >pci_memory);
>>>> -}
>>>> -


>>>> @@ -293,6 +296,9 @@ static void ibm_40p_init(MachineState *machine)
>>>>       pcihost = SYS_BUS_DEVICE(dev);
>>>>       object_property_add_child(qdev_get_machine(), "raven", 
>>>> OBJECT(dev));
>>>>       sysbus_realize_and_unref(pcihost, &error_fatal);
>>>> +    sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
>>>> +    sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
>>>> +    sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
>>>>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>>>>       if (!pci_bus) {
>>>>           error_report("could not create PCI host controller");
>>>
>>> In general the expectation is that memory regions should be 
>>> initialised in the _init() function, unless they depend upon a 
>>> property in which case they should be initialised in the _realize() 
>>> function. Why do you feel this needs to be different?
>>
>> Is any of it needed before realize? If not why have an init method at 
>> all? As shown here this works perfectly without one and is more 
>> comprehensible that way for people reading it without deep knowledge 
>> about Qdev. In general I think simple devices only need a realize 
>> method and the init method is rarely needed, e.g. if there are some 
>> child objects that need to be init for passing properties that can be 
>> set before realize or similar unusual cases but for most classes init 
>> is not needed at all. I only want to keep what's necessary and remove 
>> everything that's not needed. I think that makes the device model 
>> easier to understand.
> 
> I've checked documentation here:
> https://www.qemu.org/docs/master/devel/qdev-api.html
> but it's not really clear on what's the preferred way of using init and 
> realize. It's not even very clear on when to use which to me. So becuase 
> that did not help I did a quick survey on what other pci-host models do. 
> Of the 32 .c files in hw/pci-host 16 have an init method:
> 
> aspeed_pcie.c, astro.c, designware.c, gpex.c, grackle.c, i440fx.c, 
> pnv_phb3.c, pnv_phb3_msi.c, pnv_phb3_pbcq.c, pnv_phb4.c, q35.c, raven.c, 
> sabre.c, uninorth.c, versatile.c, xilinx-pcie.c
> 
> Of these astro.c has an empty init function that should be removed; 
> grackle.c, sabre.c and uninorth.c are maintained by you so I'll ignore 
> them here; we're discussing raven.c now and i440fx.c has two 
> memory_region_init_io calls in init that could be in realize where all 
> others are and otherwise all other models do this in realize and only 
> init child objects and add properties in init methods when that's needed 
> because they need to be available before realize. The other 16 device 
> models don't have an init method at all and do all in the realize like I 
> proposed in this patch for raven. Since only device models that you 
> maintain do it differently I think what you say is not following the 
> preferred way so you should not block this patch.
> 
> I'd be interested if there is a consensus on this or can we cone to one 
> that we can document to avoid this repeating every time.

I've been told to stop arguing about QDev on the mailing list, and
instead spend my time and energy in documenting QDev, so we'll discuss
the documentation patches :)

Also we'll try to provide a QDev meaningful state machine, which will
help to enforce doing in the correct places.


Meanwhile...

.instance_init is actually QOM layer, it is called once, and can
not fail. What is allocated here has to be de-allocated in 
.instance_finalize.

.realize is QDev where we check the device properties, reporting error.
What is allocated/configured there has to be de-allocated in .unrealize.

The big difference is for hot-pluggable devices, where unplug calls
unrealize(), keeping the device initialized. Re-plug calls .realize()
again, and we should be able to do that multiple times.

With that in mind, IMO it is better to allocate all we can once in
.instance_init().

Re: [PATCH v5 11/13] hw/pci-host/raven: Do not map regions in init method

[BALATON Zoltan]

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

On Sat, 25 Oct 2025, Philippe Mathieu-Daudé wrote:
> On 25/10/25 16:49, BALATON Zoltan wrote:
>> Hello,
>> 
>> Added a few more people to cc hoping to get some opinion to clear this up. 
>> This is brought up by my patch trying to simplify hw/pci-host/ raven.c part 
>> of this series:
>> https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/
>> (First submitted in May here:
>> https://patchew.org/QEMU/cover.1746374076.git.balaton@eik.bme.hu/
>> but that went relatively unnoticed and missed the previous release.)
>> Find discussion below the patch.
>> 
>> On Sat, 25 Oct 2025, BALATON Zoltan wrote:
>>> On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
>>>> On 23/10/2025 16:26, BALATON Zoltan wrote:
>>>>> Export memory regions as sysbus mmio regions and let the board code
>>>>> map them similar to how it is done in grackle. While at it rename
>>>>> raven_pcihost_realizefn to raven_pcihost_realize.
>>>>> 
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>   hw/pci-host/raven.c | 38 +++++++++++++-------------------------
>>>>>   hw/ppc/prep.c       | 10 ++++++++--
>>>>>   2 files changed, 21 insertions(+), 27 deletions(-)
>
>
>>>>> @@ -180,7 +178,18 @@ static void raven_pcihost_realizefn(DeviceState *d, 
>>>>> Error **errp)
>>>>>         qdev_init_gpio_in(d, raven_change_gpio, 1);
>>>>>   +    memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
>>>>> +    memory_region_init_io(&s->pci_discontiguous_io, o,
>>>>> +                          &raven_io_ops, &s->pci_io,
>>>>> +                          "pci-discontiguous-io", 8 * MiB);
>>>>> +    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>>>>> +    memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
>>>>> +
>>>>> +    sysbus_init_mmio(dev, &s->pci_io);
>>>>> +    sysbus_init_mmio(dev, &s->pci_discontiguous_io);
>>>>> +    sysbus_init_mmio(dev, &s->pci_memory);
>>>>>       sysbus_init_irq(dev, &s->irq);
>>>>> +
>>>>>       h->bus = pci_register_root_bus(d, NULL, raven_set_irq, 
>>>>> raven_map_irq,
>>>>>                                      &s->irq, &s->pci_memory, &s- 
>>>>> >pci_io, 0, 1,
>>>>>                                      TYPE_PCI_BUS);
>>>>> @@ -219,32 +228,12 @@ static void raven_pcihost_realizefn(DeviceState 
>>>>> *d, Error **errp)
>>>>>       pci_setup_iommu(h->bus, &raven_iommu_ops, s);
>>>>>   }
>>>>>   -static void raven_pcihost_initfn(Object *obj)
>>>>> -{
>>>>> -    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>>>>> -    MemoryRegion *address_space_mem = get_system_memory();
>>>>> -
>>>>> -    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>>>>> -    memory_region_init_io(&s->pci_discontiguous_io, obj,
>>>>> -                          &raven_io_ops, &s->pci_io,
>>>>> -                          "pci-discontiguous-io", 8 * MiB);
>>>>> -    memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
>>>>> -
>>>>> -    /* CPU address space */
>>>>> -    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
>>>>> -                                &s->pci_io);
>>>>> -    memory_region_add_subregion_overlap(address_space_mem, 
>>>>> PCI_IO_BASE_ADDR,
>>>>> -                                        &s->pci_discontiguous_io, 1);
>>>>> -    memory_region_set_enabled(&s->pci_discontiguous_io, false);
>>>>> -    memory_region_add_subregion(address_space_mem, 0xc0000000, &s- 
>>>>> >pci_memory);
>>>>> -}
>>>>> -
>
>
>>>>> @@ -293,6 +296,9 @@ static void ibm_40p_init(MachineState *machine)
>>>>>       pcihost = SYS_BUS_DEVICE(dev);
>>>>>       object_property_add_child(qdev_get_machine(), "raven", 
>>>>> OBJECT(dev));
>>>>>       sysbus_realize_and_unref(pcihost, &error_fatal);
>>>>> +    sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
>>>>> +    sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
>>>>> +    sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
>>>>>       pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>>>>>       if (!pci_bus) {
>>>>>           error_report("could not create PCI host controller");
>>>> 
>>>> In general the expectation is that memory regions should be initialised 
>>>> in the _init() function, unless they depend upon a property in which case 
>>>> they should be initialised in the _realize() function. Why do you feel 
>>>> this needs to be different?
>>> 
>>> Is any of it needed before realize? If not why have an init method at all? 
>>> As shown here this works perfectly without one and is more comprehensible 
>>> that way for people reading it without deep knowledge about Qdev. In 
>>> general I think simple devices only need a realize method and the init 
>>> method is rarely needed, e.g. if there are some child objects that need to 
>>> be init for passing properties that can be set before realize or similar 
>>> unusual cases but for most classes init is not needed at all. I only want 
>>> to keep what's necessary and remove everything that's not needed. I think 
>>> that makes the device model easier to understand.
>> 
>> I've checked documentation here:
>> https://www.qemu.org/docs/master/devel/qdev-api.html
>> but it's not really clear on what's the preferred way of using init and 
>> realize. It's not even very clear on when to use which to me. So becuase 
>> that did not help I did a quick survey on what other pci-host models do. Of 
>> the 32 .c files in hw/pci-host 16 have an init method:
>> 
>> aspeed_pcie.c, astro.c, designware.c, gpex.c, grackle.c, i440fx.c, 
>> pnv_phb3.c, pnv_phb3_msi.c, pnv_phb3_pbcq.c, pnv_phb4.c, q35.c, raven.c, 
>> sabre.c, uninorth.c, versatile.c, xilinx-pcie.c
>> 
>> Of these astro.c has an empty init function that should be removed; 
>> grackle.c, sabre.c and uninorth.c are maintained by you so I'll ignore them 
>> here; we're discussing raven.c now and i440fx.c has two 
>> memory_region_init_io calls in init that could be in realize where all 
>> others are and otherwise all other models do this in realize and only init 
>> child objects and add properties in init methods when that's needed because 
>> they need to be available before realize. The other 16 device models don't 
>> have an init method at all and do all in the realize like I proposed in 
>> this patch for raven. Since only device models that you maintain do it 
>> differently I think what you say is not following the preferred way so you 
>> should not block this patch.
>> 
>> I'd be interested if there is a consensus on this or can we cone to one 
>> that we can document to avoid this repeating every time.
>
> I've been told to stop arguing about QDev on the mailing list, and
> instead spend my time and energy in documenting QDev, so we'll discuss
> the documentation patches :)
>
> Also we'll try to provide a QDev meaningful state machine, which will
> help to enforce doing in the correct places.
>
>
> Meanwhile...
>
> .instance_init is actually QOM layer, it is called once, and can
> not fail. What is allocated here has to be de-allocated in 
> .instance_finalize.
>
> .realize is QDev where we check the device properties, reporting error.
> What is allocated/configured there has to be de-allocated in .unrealize.
>
> The big difference is for hot-pluggable devices, where unplug calls
> unrealize(), keeping the device initialized. Re-plug calls .realize()
> again, and we should be able to do that multiple times.
>
> With that in mind, IMO it is better to allocate all we can once in
> .instance_init().

I'd really like if simple devices could be implemented without further 
complication. I don't mind if the method is called init or realize and 
when it's called but most devices should only need one of them and not 
have to care about where each step in their init phase should go when most 
of the time they are just created once and never finalized like this one. 
For these classes having two init methods just distracts from the actual 
functionality of the class and makes it more difficult to write them and 
understand what they do. The way that a lot of classes follow currently 
that they only have realize and only need init if some properties or child 
objects providing properties need to be created before realize to be 
possible to set them is easy to understand and keeps devices simple. So is 
there an advantage for hot-pluggable devices are not deinit when they are 
unplugged or is it a big performance issue to rerun realize on re-plug? 
Given that such classes are not common I'd trade that for keeping most 
other classes simple and more easily understandable and only require 
multiple methods where really needed.

Regards,
BALATON Zoltan

[PATCH v5 12/13] hw/ppc/prep: Fix non-contiguous IO control bit

[BALATON Zoltan]

The bit that is supposed to control if ISA IO ports are accessed with
discontiguous addresses was not connected so it did nothing. We can
now directly enable or disable the discontiguous region so allow the
bit to function. This did not cause a problem so far as nothing seems
to use this bit or discontiguous IO addresses.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c    |  9 ---------
 hw/ppc/prep.c          |  3 +++
 hw/ppc/prep_systemio.c | 17 +++++++++++------
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 1e36a637a6..7ebca8186b 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -161,13 +161,6 @@ static const PCIIOMMUOps raven_iommu_ops = {
     .get_address_space = raven_pcihost_set_iommu,
 };
 
-static void raven_change_gpio(void *opaque, int n, int level)
-{
-    PREPPCIState *s = opaque;
-
-    memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
-}
-
 static void raven_pcihost_realize(DeviceState *d, Error **errp)
 {
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
@@ -176,8 +169,6 @@ static void raven_pcihost_realize(DeviceState *d, Error **errp)
     Object *o = OBJECT(d);
     MemoryRegion *mr, *bm, *address_space_mem = get_system_memory();
 
-    qdev_init_gpio_in(d, raven_change_gpio, 1);
-
     memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
     memory_region_init_io(&s->pci_discontiguous_io, o,
                           &raven_io_ops, &s->pci_io,
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 973d2fb7eb..3f497910f4 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -322,6 +322,9 @@ static void ibm_40p_init(MachineState *machine)
     dev = DEVICE(isa_dev);
     qdev_prop_set_uint32(dev, "ibm-planar-id", 0xfc);
     qdev_prop_set_uint32(dev, "equipment", 0xc0);
+    object_property_set_link(OBJECT(dev), "discontiguous-io",
+                             OBJECT(sysbus_mmio_get_region(pcihost, 1)),
+                             &error_fatal);
     isa_realize_and_unref(isa_dev, isa_bus, &error_fatal);
 
     /* Memory controller */
diff --git a/hw/ppc/prep_systemio.c b/hw/ppc/prep_systemio.c
index 41cd923b94..6ef9b91317 100644
--- a/hw/ppc/prep_systemio.c
+++ b/hw/ppc/prep_systemio.c
@@ -44,9 +44,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(PrepSystemIoState, PREP_SYSTEMIO)
 
 struct PrepSystemIoState {
     ISADevice parent_obj;
+
     MemoryRegion ppc_parity_mem;
+    MemoryRegion *discontiguous_io;
 
-    qemu_irq non_contiguous_io_map_irq;
     uint8_t sreset; /* 0x0092 */
     uint8_t equipment; /* 0x080c */
     uint8_t system_control; /* 0x081c */
@@ -206,8 +207,8 @@ static void prep_port0850_write(void *opaque, uint32_t addr, uint32_t val)
     PrepSystemIoState *s = opaque;
 
     trace_prep_systemio_write(addr, val);
-    qemu_set_irq(s->non_contiguous_io_map_irq,
-                 val & PORT0850_IOMAP_NONCONTIGUOUS);
+    memory_region_set_enabled(s->discontiguous_io,
+                              !(val & PORT0850_IOMAP_NONCONTIGUOUS));
     s->iomap_type = val & PORT0850_IOMAP_NONCONTIGUOUS;
 }
 
@@ -257,10 +258,10 @@ static void prep_systemio_realize(DeviceState *dev, Error **errp)
     PrepSystemIoState *s = PREP_SYSTEMIO(dev);
     PowerPCCPU *cpu;
 
-    qdev_init_gpio_out(dev, &s->non_contiguous_io_map_irq, 1);
+    assert(s->discontiguous_io);
     s->iomap_type = PORT0850_IOMAP_NONCONTIGUOUS;
-    qemu_set_irq(s->non_contiguous_io_map_irq,
-                 s->iomap_type & PORT0850_IOMAP_NONCONTIGUOUS);
+    memory_region_set_enabled(s->discontiguous_io,
+                              !(s->iomap_type & PORT0850_IOMAP_NONCONTIGUOUS));
     cpu = POWERPC_CPU(first_cpu);
     s->softreset_irq = qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_HRESET);
 
@@ -288,6 +289,8 @@ static const VMStateDescription vmstate_prep_systemio = {
 static const Property prep_systemio_properties[] = {
     DEFINE_PROP_UINT8("ibm-planar-id", PrepSystemIoState, ibm_planar_id, 0),
     DEFINE_PROP_UINT8("equipment", PrepSystemIoState, equipment, 0),
+    DEFINE_PROP_LINK("discontiguous-io", PrepSystemIoState, discontiguous_io,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
 };
 
 static void prep_systemio_class_initfn(ObjectClass *klass, const void *data)
@@ -296,6 +299,8 @@ static void prep_systemio_class_initfn(ObjectClass *klass, const void *data)
 
     dc->realize = prep_systemio_realize;
     dc->vmsd = &vmstate_prep_systemio;
+    /* Reason: PReP specific device, needs to be wired via properties */
+    dc->user_creatable = false;
     device_class_set_props(dc, prep_systemio_properties);
 }
 
-- 
2.41.3

[PATCH v5 13/13] hw/ppc/prep: Add reset method to prep-systemio

[BALATON Zoltan]

The initial state needs to be reset so it's not enough to set it once
at realize. Add a reset method to fix device reset state.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/raven.c    |  1 -
 hw/ppc/prep_systemio.c | 13 ++++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 7ebca8186b..fd45acb7eb 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -173,7 +173,6 @@ static void raven_pcihost_realize(DeviceState *d, Error **errp)
     memory_region_init_io(&s->pci_discontiguous_io, o,
                           &raven_io_ops, &s->pci_io,
                           "pci-discontiguous-io", 8 * MiB);
-    memory_region_set_enabled(&s->pci_discontiguous_io, false);
     memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
 
     sysbus_init_mmio(dev, &s->pci_io);
diff --git a/hw/ppc/prep_systemio.c b/hw/ppc/prep_systemio.c
index 6ef9b91317..13b8fdb56b 100644
--- a/hw/ppc/prep_systemio.c
+++ b/hw/ppc/prep_systemio.c
@@ -252,6 +252,15 @@ static const MemoryRegionOps ppc_parity_error_ops = {
     },
 };
 
+static void prep_systemio_reset(DeviceState *dev)
+{
+    PrepSystemIoState *s = PREP_SYSTEMIO(dev);
+
+    s->iomap_type = PORT0850_IOMAP_NONCONTIGUOUS;
+    memory_region_set_enabled(s->discontiguous_io,
+                              !(s->iomap_type & PORT0850_IOMAP_NONCONTIGUOUS));
+}
+
 static void prep_systemio_realize(DeviceState *dev, Error **errp)
 {
     ISADevice *isa = ISA_DEVICE(dev);
@@ -259,9 +268,6 @@ static void prep_systemio_realize(DeviceState *dev, Error **errp)
     PowerPCCPU *cpu;
 
     assert(s->discontiguous_io);
-    s->iomap_type = PORT0850_IOMAP_NONCONTIGUOUS;
-    memory_region_set_enabled(s->discontiguous_io,
-                              !(s->iomap_type & PORT0850_IOMAP_NONCONTIGUOUS));
     cpu = POWERPC_CPU(first_cpu);
     s->softreset_irq = qdev_get_gpio_in(DEVICE(cpu), PPC6xx_INPUT_HRESET);
 
@@ -301,6 +307,7 @@ static void prep_systemio_class_initfn(ObjectClass *klass, const void *data)
     dc->vmsd = &vmstate_prep_systemio;
     /* Reason: PReP specific device, needs to be wired via properties */
     dc->user_creatable = false;
+    device_class_set_legacy_reset(dc, prep_systemio_reset);
     device_class_set_props(dc, prep_systemio_properties);
 }
 
-- 
2.41.3

Re: [PATCH v5 00/13] hw/pci-host/raven clean ups

[Philippe Mathieu-Daudé]

On 23/10/25 17:26, BALATON Zoltan wrote:

> BALATON Zoltan (13):
>    hw/pci-host/raven: Simplify creating PCI facing part
>    hw/pci-host/raven: Simplify PCI facing part
>    hw/pci-host/raven: Simplify host bridge type declaration
>    hw/pci-host/raven: Use DEFINE_TYPES macro
>    hw/pci-host/raven: Simplify PCI bus creation

Queuing reviewed patches, thanks.