[PATCH v3 00/14] hw/pci-host/raven clean ups

[PATCH v3 00/14] 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

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 (14):
  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: Simplify direct config access address decoding
  hw/pci-host/raven: Rename direct config access ops
  hw/pci-host/raven: Use correct parameter in direct access ops
  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/pci-host/raven.c       | 328 ++++++++++++--------------------------
 hw/ppc/prep.c             |  19 ++-
 hw/ppc/prep_systemio.c    |  17 +-
 include/hw/pci/pci_host.h |   1 -
 4 files changed, 130 insertions(+), 235 deletions(-)

-- 
2.41.3

[PATCH v3 01/14] 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 | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index f8c0be5d21..172f01694c 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)
@@ -65,7 +58,6 @@ struct PRePPCIState {
     MemoryRegion bm_ram_alias;
     MemoryRegion bm_pci_memory_alias;
     AddressSpace bm_as;
-    RavenPCIState pci_dev;
 
     int contiguous_map;
 };
@@ -268,8 +260,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)
@@ -277,7 +268,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,
@@ -314,12 +304,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)
@@ -329,16 +313,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);
@@ -350,7 +324,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.
@@ -361,7 +334,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 v3 01/14] hw/pci-host/raven: Simplify PCI facing part

[Mark Cave-Ayland]

On 18/09/2025 19:50, 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 | 30 +-----------------------------
>   1 file changed, 1 insertion(+), 29 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index f8c0be5d21..172f01694c 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)
> @@ -65,7 +58,6 @@ struct PRePPCIState {
>       MemoryRegion bm_ram_alias;
>       MemoryRegion bm_pci_memory_alias;
>       AddressSpace bm_as;
> -    RavenPCIState pci_dev;
>   
>       int contiguous_map;
>   };
> @@ -268,8 +260,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)
> @@ -277,7 +268,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,
> @@ -314,12 +304,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)
> @@ -329,16 +313,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);
> @@ -350,7 +324,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.
> @@ -361,7 +334,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 },

I agree with removing RavenPCIState, but pci_create_simple() isn't the right solution 
here because it both init()s and realize()s the inner object. The right way to do 
this is for the parent to init() its inner object(s) within its init() function, and 
similarly for it to realize() its inner object(s) within its realize() function.

FWIW it looks as if the same mistake is present in several other hw/pci-host devices.


ATB,

Mark.

Re: [PATCH v3 01/14] hw/pci-host/raven: Simplify PCI facing part

[BALATON Zoltan]

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, 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 | 30 +-----------------------------
>>   1 file changed, 1 insertion(+), 29 deletions(-)
>> 
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index f8c0be5d21..172f01694c 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)
>> @@ -65,7 +58,6 @@ struct PRePPCIState {
>>       MemoryRegion bm_ram_alias;
>>       MemoryRegion bm_pci_memory_alias;
>>       AddressSpace bm_as;
>> -    RavenPCIState pci_dev;
>>         int contiguous_map;
>>   };
>> @@ -268,8 +260,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)
>> @@ -277,7 +268,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,
>> @@ -314,12 +304,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)
>> @@ -329,16 +313,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);
>> @@ -350,7 +324,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.
>> @@ -361,7 +334,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 },
>
> I agree with removing RavenPCIState, but pci_create_simple() isn't the right 
> solution here because it both init()s and realize()s the inner object. The 
> right way to do this is for the parent to init() its inner object(s) within 
> its init() function, and similarly for it to realize() its inner object(s) 
> within its realize() function.
>
> FWIW it looks as if the same mistake is present in several other hw/pci-host 
> devices.

So maybe that's not a mistake then. There's no need to init and realize it 
separately as this is an internal object which is enough to be created in 
realize method and init and realize there at one go for which 
pci_create_simple is appropriate. I think this inner object would only 
need to be init separately if it exposed something (like a property) that 
could be inspected or set before realize but that's not the case here so 
it does not have to be created in init only in realize. (A lot of simple 
devices don't even have init method only realize so init is only needed 
for things that have to be set before realize.)

Regards,
BALATON Zoltan

Re: [PATCH v3 01/14] hw/pci-host/raven: Simplify PCI facing part

[Harsh Prateek Bora]

Hi Mark,

Thanks much for pitching in to help with reviewing this series.

On 9/19/25 01:51, BALATON Zoltan wrote:
> On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
>> On 18/09/2025 19:50, 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 | 30 +-----------------------------
>>>   1 file changed, 1 insertion(+), 29 deletions(-)
>>>
>>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>>> index f8c0be5d21..172f01694c 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)
>>> @@ -65,7 +58,6 @@ struct PRePPCIState {
>>>       MemoryRegion bm_ram_alias;
>>>       MemoryRegion bm_pci_memory_alias;
>>>       AddressSpace bm_as;
>>> -    RavenPCIState pci_dev;
>>>         int contiguous_map;
>>>   };
>>> @@ -268,8 +260,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);
>>>   }

<snip>

>>> @@ -361,7 +334,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 },
>>
>> I agree with removing RavenPCIState, but pci_create_simple() isn't the 
>> right solution here because it both init()s and realize()s the inner 
>> object. The right way to do this is for the parent to init() its inner 
>> object(s) within its init() function, and similarly for it to 
>> realize() its inner object(s) within its realize() function.
>>
>> FWIW it looks as if the same mistake is present in several other 
>> hw/pci-host devices.
> 
> So maybe that's not a mistake then. There's no need to init and realize 
> it separately as this is an internal object which is enough to be 
> created in realize method and init and realize there at one go for which 
> pci_create_simple is appropriate. I think this inner object would only 
> need to be init separately if it exposed something (like a property) 
> that could be inspected or set before realize but that's not the case 
> here so it does not have to be created in init only in realize. (A lot 
> of simple devices don't even have init method only realize so init is 
> only needed for things that have to be set before realize.)

Do we have a consensus here ?

regards,
Harsh

> 
> Regards,
> BALATON Zoltan
> 

Re: [PATCH v3 01/14] hw/pci-host/raven: Simplify PCI facing part

[BALATON Zoltan]

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

On Sat, 18 Oct 2025, Harsh Prateek Bora wrote:
> Hi Mark,
>
> Thanks much for pitching in to help with reviewing this series.
>
> On 9/19/25 01:51, BALATON Zoltan wrote:
>> On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
>>> On 18/09/2025 19:50, 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 | 30 +-----------------------------
>>>>   1 file changed, 1 insertion(+), 29 deletions(-)
>>>> 
>>>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>>>> index f8c0be5d21..172f01694c 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)
>>>> @@ -65,7 +58,6 @@ struct PRePPCIState {
>>>>       MemoryRegion bm_ram_alias;
>>>>       MemoryRegion bm_pci_memory_alias;
>>>>       AddressSpace bm_as;
>>>> -    RavenPCIState pci_dev;
>>>>         int contiguous_map;
>>>>   };
>>>> @@ -268,8 +260,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);
>>>>   }
>
> <snip>
>
>>>> @@ -361,7 +334,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 },
>>> 
>>> I agree with removing RavenPCIState, but pci_create_simple() isn't the 
>>> right solution here because it both init()s and realize()s the inner 
>>> object. The right way to do this is for the parent to init() its inner 
>>> object(s) within its init() function, and similarly for it to realize() 
>>> its inner object(s) within its realize() function.
>>> 
>>> FWIW it looks as if the same mistake is present in several other 
>>> hw/pci-host devices.
>> 
>> So maybe that's not a mistake then. There's no need to init and realize it 
>> separately as this is an internal object which is enough to be created in 
>> realize method and init and realize there at one go for which 
>> pci_create_simple is appropriate. I think this inner object would only need 
>> to be init separately if it exposed something (like a property) that could 
>> be inspected or set before realize but that's not the case here so it does 
>> not have to be created in init only in realize. (A lot of simple devices 
>> don't even have init method only realize so init is only needed for things 
>> that have to be set before realize.)
>
> Do we have a consensus here ?

It's hard to get a consensus if only two people care and they have 
different views... I think a separate init is not needed here and as noted 
the same pattern is present elsewhere and wasn't criticised or deprecated 
practice. The separate init and realize is also not a convention known to 
me (unless needed for other reason like I said above) so I regard it as a 
personal preference not something that needs to be followed generally.

Mark had a comment on the last patch about enabling a memory region that 
after thinking about it I think should be in a reset method. I plan to 
submit a new version with that.

Regards,
BALATON Zoltan

Re: [PATCH v3 01/14] hw/pci-host/raven: Simplify PCI facing part

[Mark Cave-Ayland]

On 18/10/2025 03:41, Harsh Prateek Bora wrote:

> Hi Mark,
> 
> Thanks much for pitching in to help with reviewing this series.

Hi Harsh,

No worries - I've looked at raven before when working on adding 40p support for 
OpenBIOS, so I do have some familiarity.

> On 9/19/25 01:51, BALATON Zoltan wrote:
>> On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
>>> On 18/09/2025 19:50, 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 | 30 +-----------------------------
>>>>   1 file changed, 1 insertion(+), 29 deletions(-)
>>>>
>>>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>>>> index f8c0be5d21..172f01694c 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)
>>>> @@ -65,7 +58,6 @@ struct PRePPCIState {
>>>>       MemoryRegion bm_ram_alias;
>>>>       MemoryRegion bm_pci_memory_alias;
>>>>       AddressSpace bm_as;
>>>> -    RavenPCIState pci_dev;
>>>>         int contiguous_map;
>>>>   };
>>>> @@ -268,8 +260,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);
>>>>   }
> 
> <snip>
> 
>>>> @@ -361,7 +334,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 },
>>>
>>> I agree with removing RavenPCIState, but pci_create_simple() isn't the right 
>>> solution here because it both init()s and realize()s the inner object. The right 
>>> way to do this is for the parent to init() its inner object(s) within its init() 
>>> function, and similarly for it to realize() its inner object(s) within its 
>>> realize() function.
>>>
>>> FWIW it looks as if the same mistake is present in several other hw/pci-host devices.
>>
>> So maybe that's not a mistake then. There's no need to init and realize it 
>> separately as this is an internal object which is enough to be created in realize 
>> method and init and realize there at one go for which pci_create_simple is 
>> appropriate. I think this inner object would only need to be init separately if it 
>> exposed something (like a property) that could be inspected or set before realize 
>> but that's not the case here so it does not have to be created in init only in 
>> realize. (A lot of simple devices don't even have init method only realize so init 
>> is only needed for things that have to be set before realize.)
> 
> Do we have a consensus here ?
> 
> regards,
> Harsh
Given there is still some ongoing discussion regarding object modelling, I think this 
will require a separate tidy-up so let's go with the pci_create_simple() approach for 
now.

The changes to the interrupt routing and readability of some of the changes from a 
developer's perspective are still of concern to me.


ATB,

Mark.

Re: [PATCH v3 01/14] hw/pci-host/raven: Simplify PCI facing part

[BALATON Zoltan]

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

On Sun, 19 Oct 2025, Mark Cave-Ayland wrote:
> On 18/10/2025 03:41, Harsh Prateek Bora wrote:
>> Hi Mark,
>> 
>> Thanks much for pitching in to help with reviewing this series.
>
> Hi Harsh,
>
> No worries - I've looked at raven before when working on adding 40p support 
> for OpenBIOS, so I do have some familiarity.
>
>> On 9/19/25 01:51, BALATON Zoltan wrote:
>>> On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
>>>> On 18/09/2025 19:50, 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 | 30 +-----------------------------
>>>>>   1 file changed, 1 insertion(+), 29 deletions(-)
>>>>> 
>>>>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>>>>> index f8c0be5d21..172f01694c 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)
>>>>> @@ -65,7 +58,6 @@ struct PRePPCIState {
>>>>>       MemoryRegion bm_ram_alias;
>>>>>       MemoryRegion bm_pci_memory_alias;
>>>>>       AddressSpace bm_as;
>>>>> -    RavenPCIState pci_dev;
>>>>>         int contiguous_map;
>>>>>   };
>>>>> @@ -268,8 +260,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);
>>>>>   }
>> 
>> <snip>
>> 
>>>>> @@ -361,7 +334,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 },
>>>> 
>>>> I agree with removing RavenPCIState, but pci_create_simple() isn't the 
>>>> right solution here because it both init()s and realize()s the inner 
>>>> object. The right way to do this is for the parent to init() its inner 
>>>> object(s) within its init() function, and similarly for it to realize() 
>>>> its inner object(s) within its realize() function.
>>>> 
>>>> FWIW it looks as if the same mistake is present in several other 
>>>> hw/pci-host devices.
>>> 
>>> So maybe that's not a mistake then. There's no need to init and realize it 
>>> separately as this is an internal object which is enough to be created in 
>>> realize method and init and realize there at one go for which 
>>> pci_create_simple is appropriate. I think this inner object would only 
>>> need to be init separately if it exposed something (like a property) that 
>>> could be inspected or set before realize but that's not the case here so 
>>> it does not have to be created in init only in realize. (A lot of simple 
>>> devices don't even have init method only realize so init is only needed 
>>> for things that have to be set before realize.)
>> 
>> Do we have a consensus here ?
>> 
>> regards,
>> Harsh
> Given there is still some ongoing discussion regarding object modelling, I 
> think this will require a separate tidy-up so let's go with the 
> pci_create_simple() approach for now.
>
> The changes to the interrupt routing and readability of some of the changes 
> from a developer's perspective are still of concern to me.

I think simpler is more readable so not having an or-irq object where not 
needed as the PCI code can handle this makes it more readable (also the 
same as ppc440_pcix which was previously approved by Peter[1] and a patch 
to add or-irq there was dropped as unneeded[2] so doing the same thing the 
same way here is also more readable and more consistent). Thus I think the 
interrupt routing changes should be OK and having an or-irq is an 
unneeded complication.

What other readablility concerns do you have? Is it about not passing the 
whole device state struct to callbacks but only what they need from it? 
I've answered that already and I think that unnecessary casts would not 
add any readablility. I'd like to hear others' opinion too but it seems 
not many care so it's only us and we both seem to have strong view on 
these things so it's hard to come to an agreement.

Regards,
BALATON Zoltan

[1] commit 2a9cf49598c65 and 
https://lists.nongnu.org/archive/html/qemu-ppc/2021-01/msg00031.html

[2] https://lists.nongnu.org/archive/html/qemu-ppc/2020-12/msg00422.html 
https://lists.nongnu.org/archive/html/qemu-ppc/2020-12/msg00423.html

Re: [PATCH v3 01/14] hw/pci-host/raven: Simplify PCI facing part

[Harsh Prateek Bora]

On 10/20/25 04:56, BALATON Zoltan wrote:
> On Sun, 19 Oct 2025, Mark Cave-Ayland wrote:
>> On 18/10/2025 03:41, Harsh Prateek Bora wrote:
>>> Hi Mark,
>>>
>>> Thanks much for pitching in to help with reviewing this series.
>>
>> Hi Harsh,
>>
>> No worries - I've looked at raven before when working on adding 40p 
>> support for OpenBIOS, so I do have some familiarity.

Nice, thanks.

>>
>>> On 9/19/25 01:51, BALATON Zoltan wrote:
>>>> On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
>>>>> On 18/09/2025 19:50, 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 | 30 +-----------------------------
>>>>>>   1 file changed, 1 insertion(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>>>>>> index f8c0be5d21..172f01694c 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)
>>>>>> @@ -65,7 +58,6 @@ struct PRePPCIState {
>>>>>>       MemoryRegion bm_ram_alias;
>>>>>>       MemoryRegion bm_pci_memory_alias;
>>>>>>       AddressSpace bm_as;
>>>>>> -    RavenPCIState pci_dev;
>>>>>>         int contiguous_map;
>>>>>>   };
>>>>>> @@ -268,8 +260,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);
>>>>>>   }
>>>
>>> <snip>
>>>
>>>>>> @@ -361,7 +334,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 },
>>>>>
>>>>> I agree with removing RavenPCIState, but pci_create_simple() isn't 
>>>>> the right solution here because it both init()s and realize()s the 
>>>>> inner object. The right way to do this is for the parent to init() 
>>>>> its inner object(s) within its init() function, and similarly for 
>>>>> it to realize() its inner object(s) within its realize() function.
>>>>>
>>>>> FWIW it looks as if the same mistake is present in several other 
>>>>> hw/pci-host devices.
>>>>
>>>> So maybe that's not a mistake then. There's no need to init and 
>>>> realize it separately as this is an internal object which is enough 
>>>> to be created in realize method and init and realize there at one go 
>>>> for which pci_create_simple is appropriate. I think this inner 
>>>> object would only need to be init separately if it exposed something 
>>>> (like a property) that could be inspected or set before realize but 
>>>> that's not the case here so it does not have to be created in init 
>>>> only in realize. (A lot of simple devices don't even have init 
>>>> method only realize so init is only needed for things that have to 
>>>> be set before realize.)
>>>
>>> Do we have a consensus here ?
>>>
>>> regards,
>>> Harsh
>> Given there is still some ongoing discussion regarding object 
>> modelling, I think this will require a separate tidy-up so let's go 
>> with the pci_create_simple() approach for now.

Sure, thanks for considering.

>>
>> The changes to the interrupt routing and readability of some of the 
>> changes from a developer's perspective are still of concern to me.
> 
> I think simpler is more readable so not having an or-irq object where 
> not needed as the PCI code can handle this makes it more readable (also 
> the same as ppc440_pcix which was previously approved by Peter[1] and a 
> patch to add or-irq there was dropped as unneeded[2] so doing the same 
> thing the same way here is also more readable and more consistent). Thus 
> I think the interrupt routing changes should be OK and having an or-irq 
> is an unneeded complication.
> 
> What other readablility concerns do you have? Is it about not passing 
> the whole device state struct to callbacks but only what they need from 
> it? I've answered that already and I think that unnecessary casts would 
> not add any readablility. I'd like to hear others' opinion too but it 
> seems not many care so it's only us and we both seem to have strong view 
> on these things so it's hard to come to an agreement.

I think since the changes are contained to prep/raven (which although I 
am not so familiar with), I hope we just need to ensure changes are safe 
enough and can be provided a R-b to be considered for merge and any 
improvements can be done as a follow-up later as needed. Thanks again.

regards,
Harsh

> 
> Regards,
> BALATON Zoltan
> 
> [1] commit 2a9cf49598c65 and 
> https://lists.nongnu.org/archive/html/qemu-ppc/2021-01/msg00031.html
> 
> [2] https://lists.nongnu.org/archive/html/qemu-ppc/2020-12/msg00422.html 
> https://lists.nongnu.org/archive/html/qemu-ppc/2020-12/msg00423.html

[PATCH v3 02/14] hw/pci-host/raven: Simplify host bridge type declaration

[BALATON Zoltan]

Use OBJECT_DECLARE_SIMPLE_TYPE macro instead of open coding it.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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 172f01694c..878c915de5 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

Re: [PATCH v3 02/14] hw/pci-host/raven: Simplify host bridge type declaration

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> Use OBJECT_DECLARE_SIMPLE_TYPE macro instead of open coding it.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   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 172f01694c..878c915de5 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;

Possibly worth mentioning the change of case for PREPPCIState? Otherwise:

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


ATB,

Mark.

[PATCH v3 03/14] 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>
---
 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 878c915de5..e0f98afebf 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -304,6 +304,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;
@@ -329,37 +338,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

Re: [PATCH v3 03/14] hw/pci-host/raven: Use DEFINE_TYPES macro

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> 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>
> ---
>   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 878c915de5..e0f98afebf 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -304,6 +304,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;
> @@ -329,37 +338,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)

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


ATB,

Mark.

[PATCH v3 04/14] 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.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 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 e0f98afebf..51427553b2 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;
@@ -239,8 +238,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);
@@ -258,12 +258,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();
 
@@ -286,8 +288,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);
@@ -298,10 +298,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

Re: [PATCH v3 04/14] hw/pci-host/raven: Simplify PCI bus creation

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> Instead of doing it manually use pci_register_root_bus() to create and
> register the PCI bus.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   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 e0f98afebf..51427553b2 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;
> @@ -239,8 +238,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);
> @@ -258,12 +258,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();
>   
> @@ -286,8 +288,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);
> @@ -298,10 +298,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)

It also looks as if this patch removes PREPPCIState::pci_bus in favour of the 
existing PCIHostState::bus which is probably worth mentioning in the commit message. 
Otherwise:

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


ATB,

Mark.

[PATCH v3 05/14] 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.

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 51427553b2..a400a22df3 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;
@@ -183,16 +179,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,
@@ -220,26 +225,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 982e40e53e..d3365414d2 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 v3 05/14] hw/pci-host/raven: Simplify PCI interrupt routing

[Mark Cave-Ayland]

On 18/09/2025 19:50, 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.
> 
> 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 51427553b2..a400a22df3 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;
> @@ -183,16 +179,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,
> @@ -220,26 +225,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 982e40e53e..d3365414d2 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"));

I'm not convinced this is the right approach, because in the past people have 
expressed interest in monitoring the individual PCI IRQ lines (and in fact, I think 
you've exposed them as gpios for the VIA device). I can certainly see the value of 
modelling the OR function and driving the individual PCI IRQs from the raven device.


ATB,

Mark.

Re: [PATCH v3 05/14] hw/pci-host/raven: Simplify PCI interrupt routing

[BALATON Zoltan]

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, 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.
>> 
>> 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 51427553b2..a400a22df3 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;
>> @@ -183,16 +179,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,
>> @@ -220,26 +225,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 982e40e53e..d3365414d2 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"));
>
> I'm not convinced this is the right approach, because in the past people have 
> expressed interest in monitoring the individual PCI IRQ lines (and in fact, I 
> think you've exposed them as gpios for the VIA device). I can certainly see 
> the value of modelling the OR function and driving the individual PCI IRQs 
> from the raven device.

I surely wasn't who proposed exposing interrupts from the VIA device 
through gpios but that's a different problem there anyway. This one is 
similar to hw/pci-host/ppc440_pcix.c (all 4 PCI interrupts go out to a 
single output) and that is tested and works and reviewed several times so 
no need to make this more complex than that here either. At one point we 
wanted to change ppc440_pcix to or-irq as well but found it unnecessary: 
https://lists.nongnu.org/archive/html/qemu-ppc/2020-12/msg00423.html and 
there may have been discussion of it before that which I don't find now. 
I've copied the same comment in this patch that explains why it works.

Regards,
BALATON Zoltan

[PATCH v3 06/14] hw/pci-host/raven: Simplify direct config access address decoding

[BALATON Zoltan]

Use ctz instead of an open coded version and rename function to better
show what it does.

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

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index a400a22df3..66dab28a29 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -57,16 +57,9 @@ struct PREPPCIState {
 
 #define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
 
-static inline uint32_t raven_pci_io_config(hwaddr addr)
+static inline uint32_t raven_idsel_to_addr(hwaddr addr)
 {
-    int i;
-
-    for (i = 0; i < 11; i++) {
-        if ((addr & (1 << (11 + i))) != 0) {
-            break;
-        }
-    }
-    return (addr & 0x7ff) |  (i << 11);
+    return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
 }
 
 static void raven_pci_io_write(void *opaque, hwaddr addr,
@@ -74,7 +67,7 @@ static void raven_pci_io_write(void *opaque, hwaddr addr,
 {
     PREPPCIState *s = opaque;
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
-    pci_data_write(phb->bus, raven_pci_io_config(addr), val, size);
+    pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
 }
 
 static uint64_t raven_pci_io_read(void *opaque, hwaddr addr,
@@ -82,7 +75,7 @@ static uint64_t raven_pci_io_read(void *opaque, hwaddr addr,
 {
     PREPPCIState *s = opaque;
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
-    return pci_data_read(phb->bus, raven_pci_io_config(addr), size);
+    return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
 }
 
 static const MemoryRegionOps raven_pci_io_ops = {
-- 
2.41.3

Re: [PATCH v3 06/14] hw/pci-host/raven: Simplify direct config access address decoding

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> Use ctz instead of an open coded version and rename function to better
> show what it does.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 15 ++++-----------
>   1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index a400a22df3..66dab28a29 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -57,16 +57,9 @@ struct PREPPCIState {
>   
>   #define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
>   
> -static inline uint32_t raven_pci_io_config(hwaddr addr)
> +static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>   {
> -    int i;
> -
> -    for (i = 0; i < 11; i++) {
> -        if ((addr & (1 << (11 + i))) != 0) {
> -            break;
> -        }
> -    }
> -    return (addr & 0x7ff) |  (i << 11);
> +    return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
>   }

This looks like a good rework of the logic.

>   static void raven_pci_io_write(void *opaque, hwaddr addr,
> @@ -74,7 +67,7 @@ static void raven_pci_io_write(void *opaque, hwaddr addr,
>   {
>       PREPPCIState *s = opaque;
>       PCIHostState *phb = PCI_HOST_BRIDGE(s);
> -    pci_data_write(phb->bus, raven_pci_io_config(addr), val, size);
> +    pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
>   }
>   
>   static uint64_t raven_pci_io_read(void *opaque, hwaddr addr,
> @@ -82,7 +75,7 @@ static uint64_t raven_pci_io_read(void *opaque, hwaddr addr,
>   {
>       PREPPCIState *s = opaque;
>       PCIHostState *phb = PCI_HOST_BRIDGE(s);
> -    return pci_data_read(phb->bus, raven_pci_io_config(addr), size);
> +    return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
>   }
>   
>   static const MemoryRegionOps raven_pci_io_ops = {

It's a bit confusing during review that pci_data_read()/pci_data_write() access 
config space, but that's unrelated to this patch.

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


ATB,

Mark.

Re: [PATCH v3 06/14] hw/pci-host/raven: Simplify direct config access address decoding

[Philippe Mathieu-Daudé]

On 18/9/25 20:50, BALATON Zoltan wrote:
> Use ctz instead of an open coded version and rename function to better
> show what it does.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 15 ++++-----------
>   1 file changed, 4 insertions(+), 11 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 07/14] hw/pci-host/raven: Rename direct config access ops

[BALATON Zoltan]

Rename memory io ops implementing PCI configuration direct access to
mmcfg which describes better what these are for.

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

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 66dab28a29..d7a0bde382 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -62,25 +62,24 @@ static inline uint32_t raven_idsel_to_addr(hwaddr addr)
     return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
 }
 
-static void raven_pci_io_write(void *opaque, hwaddr addr,
-                               uint64_t val, unsigned int size)
+static void raven_mmcfg_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned int size)
 {
     PREPPCIState *s = opaque;
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
 }
 
-static uint64_t raven_pci_io_read(void *opaque, hwaddr addr,
-                                  unsigned int size)
+static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned int size)
 {
     PREPPCIState *s = opaque;
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
 }
 
-static const MemoryRegionOps raven_pci_io_ops = {
-    .read = raven_pci_io_read,
-    .write = raven_pci_io_write,
+static const MemoryRegionOps raven_mmcfg_ops = {
+    .read = raven_mmcfg_read,
+    .write = raven_mmcfg_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
@@ -234,8 +233,8 @@ 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(s), &raven_pci_io_ops, s,
-                          "pciio", 0x00400000);
+    memory_region_init_io(&h->mmcfg, OBJECT(s), &raven_mmcfg_ops, s,
+                          "pci-mmcfg", 0x00400000);
     memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
 
     memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
-- 
2.41.3

Re: [PATCH v3 07/14] hw/pci-host/raven: Rename direct config access ops

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> Rename memory io ops implementing PCI configuration direct access to
> mmcfg which describes better what these are for.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 66dab28a29..d7a0bde382 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -62,25 +62,24 @@ static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>       return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);
>   }
>   
> -static void raven_pci_io_write(void *opaque, hwaddr addr,
> -                               uint64_t val, unsigned int size)
> +static void raven_mmcfg_write(void *opaque, hwaddr addr, uint64_t val,
> +                              unsigned int size)
>   {
>       PREPPCIState *s = opaque;
>       PCIHostState *phb = PCI_HOST_BRIDGE(s);
>       pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
>   }
>   
> -static uint64_t raven_pci_io_read(void *opaque, hwaddr addr,
> -                                  unsigned int size)
> +static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned int size)
>   {
>       PREPPCIState *s = opaque;
>       PCIHostState *phb = PCI_HOST_BRIDGE(s);
>       return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
>   }
>   
> -static const MemoryRegionOps raven_pci_io_ops = {
> -    .read = raven_pci_io_read,
> -    .write = raven_pci_io_write,
> +static const MemoryRegionOps raven_mmcfg_ops = {
> +    .read = raven_mmcfg_read,
> +    .write = raven_mmcfg_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>   };
>   
> @@ -234,8 +233,8 @@ 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(s), &raven_pci_io_ops, s,
> -                          "pciio", 0x00400000);
> +    memory_region_init_io(&h->mmcfg, OBJECT(s), &raven_mmcfg_ops, s,
> +                          "pci-mmcfg", 0x00400000);
>       memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
>   
>       memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,

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


ATB,

Mark.

Re: [PATCH v3 07/14] hw/pci-host/raven: Rename direct config access ops

[Philippe Mathieu-Daudé]

On 18/9/25 20:50, BALATON Zoltan wrote:
> Rename memory io ops implementing PCI configuration direct access to
> mmcfg which describes better what these are for.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 08/14] hw/pci-host/raven: Use correct parameter in direct access ops

[BALATON Zoltan]

Instead of passing unneeded enclosing objects to the config direct
access ops that only need the bus we can pass that directly thus
simplifying the functions.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/raven.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index d7a0bde382..2057a1869f 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -65,16 +65,16 @@ static inline uint32_t raven_idsel_to_addr(hwaddr addr)
 static void raven_mmcfg_write(void *opaque, hwaddr addr, uint64_t val,
                               unsigned int size)
 {
-    PREPPCIState *s = opaque;
-    PCIHostState *phb = PCI_HOST_BRIDGE(s);
-    pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
+    PCIBus *hbus = opaque;
+
+    pci_data_write(hbus, raven_idsel_to_addr(addr), val, size);
 }
 
 static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned int size)
 {
-    PREPPCIState *s = opaque;
-    PCIHostState *phb = PCI_HOST_BRIDGE(s);
-    return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
+    PCIBus *hbus = opaque;
+
+    return pci_data_read(hbus, raven_idsel_to_addr(addr), size);
 }
 
 static const MemoryRegionOps raven_mmcfg_ops = {
@@ -233,7 +233,7 @@ 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(s), &raven_mmcfg_ops, s,
+    memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
                           "pci-mmcfg", 0x00400000);
     memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
 
-- 
2.41.3

Re: [PATCH v3 08/14] hw/pci-host/raven: Use correct parameter in direct access ops

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> Instead of passing unneeded enclosing objects to the config direct
> access ops that only need the bus we can pass that directly thus
> simplifying the functions.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/raven.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index d7a0bde382..2057a1869f 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -65,16 +65,16 @@ static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>   static void raven_mmcfg_write(void *opaque, hwaddr addr, uint64_t val,
>                                 unsigned int size)
>   {
> -    PREPPCIState *s = opaque;
> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> -    pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
> +    PCIBus *hbus = opaque;
> +
> +    pci_data_write(hbus, raven_idsel_to_addr(addr), val, size);
>   }
>   
>   static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned int size)
>   {
> -    PREPPCIState *s = opaque;
> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> -    return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
> +    PCIBus *hbus = opaque;
> +
> +    return pci_data_read(hbus, raven_idsel_to_addr(addr), size);
>   }
>   
>   static const MemoryRegionOps raven_mmcfg_ops = {
> @@ -233,7 +233,7 @@ 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(s), &raven_mmcfg_ops, s,
> +    memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
>                             "pci-mmcfg", 0x00400000);
>       memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);

I find this confusing as a reviewer since the general expectation is that the device 
is passed as the opaque for the memory region rather than the bus. What is the reason 
for trying to change an existing convention here?

You could simplify what is there by dropping the PREPPCIState reference and simply doing:

   static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned int size)
   {
      PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
      return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
   }

etc.


ATB,

Mark.

Re: [PATCH v3 08/14] hw/pci-host/raven: Use correct parameter in direct access ops

[BALATON Zoltan]

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

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, BALATON Zoltan wrote:
>> Instead of passing unneeded enclosing objects to the config direct
>> access ops that only need the bus we can pass that directly thus
>> simplifying the functions.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/pci-host/raven.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index d7a0bde382..2057a1869f 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -65,16 +65,16 @@ static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>>   static void raven_mmcfg_write(void *opaque, hwaddr addr, uint64_t val,
>>                                 unsigned int size)
>>   {
>> -    PREPPCIState *s = opaque;
>> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>> -    pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
>> +    PCIBus *hbus = opaque;
>> +
>> +    pci_data_write(hbus, raven_idsel_to_addr(addr), val, size);
>>   }
>>     static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned 
>> int size)
>>   {
>> -    PREPPCIState *s = opaque;
>> -    PCIHostState *phb = PCI_HOST_BRIDGE(s);
>> -    return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
>> +    PCIBus *hbus = opaque;
>> +
>> +    return pci_data_read(hbus, raven_idsel_to_addr(addr), size);
>>   }
>>     static const MemoryRegionOps raven_mmcfg_ops = {
>> @@ -233,7 +233,7 @@ 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(s), &raven_mmcfg_ops, s,
>> +    memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
>>                             "pci-mmcfg", 0x00400000);
>>       memory_region_add_subregion(address_space_mem, 0x80800000, 
>> &h->mmcfg);
>
> I find this confusing as a reviewer since the general expectation is that the 
> device is passed as the opaque for the memory region rather than the bus. 
> What is the reason for trying to change an existing convention here?

I don't think there is such convention that it must be the device state, 
that's just a common thing and obvious choice in a lot of cases but the 
opaque parameter is defined to take a pointer to some data the callback 
needs and what it is is defined by the callback. As this callback only 
needs the bus it's simplest to pass that as the opaque data.

> You could simplify what is there by dropping the PREPPCIState reference and 
> simply doing:
>
>  static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned int 
> size)
>  {
>     PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
>     return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
>  }
>
> etc.

So you broke your own convention here already by passing the parent object 
and not the PREPPCI. Then we can go further the same way and pass only 
what the callback needs which is what my patch does. (There is also no 
convention that opaque must be an object or device so no need to QOM cast 
it either. The type is verified when registered the callback.)

Regards,
BALATON Zoltan

[PATCH v3 09/14] 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 v3 09/14] hw/pci-host/raven: Do not use parent object for mmcfg region

[Mark Cave-Ayland]

On 18/09/2025 19:50, 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;

Is there any reason that the lifetime of the MemoryRegion needs to be separate from 
that of the device itself? If not, I'm struggling to understand why this needs to be 
changed.


ATB,

Mark.

Re: [PATCH v3 09/14] hw/pci-host/raven: Do not use parent object for mmcfg region

[BALATON Zoltan]

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, 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;
>
> Is there any reason that the lifetime of the MemoryRegion needs to be 
> separate from that of the device itself? If not, I'm struggling to understand 
> why this needs to be changed.

I've tried to explain that in the comment message. The lifetime is still 
the same as the device as the memory region is owned by the device but 
this way we can remove the mmcfg field from the parent object that does 
not belong there and only used by this device (maybe for historical 
reasons as this is an old device model).

Regards,
BALATON Zoltan

[PATCH v3 10/14] 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>
---
 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

Re: [PATCH v3 10/14] hw/pci-host/raven: Fix PCI config direct access region

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> 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>
> ---
>   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);

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


ATB,

Mark.

[PATCH v3 11/14] 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 v3 11/14] hw/pci-host/raven: Simpify discontiguous IO access

[Mark Cave-Ayland]

On 18/09/2025 19:50, 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;
>   }

I'm guessing the dispatch target is guaranteed not to be a RAM-backed memory region?

> -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);

Again I think passing the device as the opaque is easier to understand.

>       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 */

Out of curiosity do you have an image or firmware that tests this?


ATB,

Mark.

Re: [PATCH v3 11/14] hw/pci-host/raven: Simpify discontiguous IO access

[BALATON Zoltan]

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, 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;
>>   }
>
> I'm guessing the dispatch target is guaranteed not to be a RAM-backed memory 
> region?

I think so because these are callbacks for the ISA io region so that 
cannot be RAM. But memory_region_dispatch_* should handle it anyway, no?

>> -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);
>
> Again I think passing the device as the opaque is easier to understand.

I disagree here, passing the memory region to memory region callbacks is 
more straight forward than fishing it out from a device state. The 
callback only needs the memory region so that's what I pass it to in 
opaque.

>>       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 */
>
> Out of curiosity do you have an image or firmware that tests this?

I've tried with a firmware image and the test cases and an AIX CD which 
work as before but I don't think any of them use the discontigous io 
setting so I could only verify it does not break the current setting.

Regards,
BALATON Zoltan

[PATCH v3 12/14] 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 v3 12/14] hw/pci-host/raven: Move bus master address space creation to one place

[Mark Cave-Ayland]

On 18/09/2025 19:50, 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)

Same comment here for patch 9: what is the advantage of changing this if the 
lifetimes are not different?


ATB,

Mark.

Re: [PATCH v3 12/14] hw/pci-host/raven: Move bus master address space creation to one place

[BALATON Zoltan]

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, 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)
>
> Same comment here for patch 9: what is the advantage of changing this if the 
> lifetimes are not different?

Same answer. We can get rid of fields from the state struct. We don't need 
to store things there that we never use just to manage the lifetime of the 
memory region.

Regards,
BALATON Zoltan

[PATCH v3 13/14] 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.

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

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index ebf0c511dc..0c4eca04bb 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,17 @@ 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_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 +227,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 +266,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 d3365414d2..23d0e1eeaa 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,10 @@ 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);
+    memory_region_set_enabled(sysbus_mmio_get_region(pcihost, 1), false);
+    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 v3 13/14] hw/pci-host/raven: Do not map regions in init method

[Mark Cave-Ayland]

On 18/09/2025 19:50, 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.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/raven.c | 37 ++++++++++++-------------------------
>   hw/ppc/prep.c       | 11 +++++++++--
>   2 files changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index ebf0c511dc..0c4eca04bb 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)

There is a function rename here not mentioned in the commit message.

>   {
>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
> @@ -180,7 +178,17 @@ 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_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 +227,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);
> -}

Normally if the MemoryRegion does not depend upon a property, it can be initialised 
in an init() function, so these can stay where they are.

But certainly the device should not be mapping itself, so the sysbus-related changes 
look correct.

> -
>   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 +266,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 d3365414d2..23d0e1eeaa 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,10 @@ 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);
> +    memory_region_set_enabled(sysbus_mmio_get_region(pcihost, 1), false);

Why not leave this within the device itself? It seems odd that something external to 
the device is trying to set its initial state.

> +    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");


ATB,

Mark.

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

[BALATON Zoltan]

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, 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.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/pci-host/raven.c | 37 ++++++++++++-------------------------
>>   hw/ppc/prep.c       | 11 +++++++++--
>>   2 files changed, 21 insertions(+), 27 deletions(-)
>> 
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index ebf0c511dc..0c4eca04bb 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)
>
> There is a function rename here not mentioned in the commit message.

I thought this is trivial enough but can mention it in the commit message.

>>   {
>>       SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>       PCIHostState *h = PCI_HOST_BRIDGE(dev);
>> @@ -180,7 +178,17 @@ 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_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 +227,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);
>> -}
>
> Normally if the MemoryRegion does not depend upon a property, it can be 
> initialised in an init() function, so these can stay where they are.

There's no need to have an init method in the first place as these 
also don't provide a property so they can be created in realize instead.

> But certainly the device should not be mapping itself, so the sysbus-related 
> changes look correct.
>
>> -
>>   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 +266,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 d3365414d2..23d0e1eeaa 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,10 @@ 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);
>> +    memory_region_set_enabled(sysbus_mmio_get_region(pcihost, 1), false);
>
> Why not leave this within the device itself? It seems odd that something 
> external to the device is trying to set its initial state.

I don't remember but maybe this is to keep existing behaviour and can be 
removed after next patch where it sets initial state based on property? 
I'd have to check this again.

Regards,
BALATON Zoltan

>> +    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");
>
>
> ATB,
>
> Mark.
>
>

[PATCH v3 14/14] 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 0c4eca04bb..fd45acb7eb 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 23d0e1eeaa..678682fdd2 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -358,6 +358,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);
 
         dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(1, 0),
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

Re: [PATCH v3 14/14] hw/ppc/prep: Fix non-contiguous IO control bit

[Mark Cave-Ayland]

On 18/09/2025 19:50, BALATON Zoltan wrote:

> 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 0c4eca04bb..fd45acb7eb 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 23d0e1eeaa..678682fdd2 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -358,6 +358,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);
>   
>           dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(1, 0),
> 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);
>   }

Making a device non-user-creatable seems to be a step backwards: why not keep the 
gpio for signalling the non-contiguous IO configuration?


ATB,

Mark.

Re: [PATCH v3 14/14] hw/ppc/prep: Fix non-contiguous IO control bit

[BALATON Zoltan]

On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, BALATON Zoltan wrote:
>> 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 0c4eca04bb..fd45acb7eb 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 23d0e1eeaa..678682fdd2 100644
>> --- a/hw/ppc/prep.c
>> +++ b/hw/ppc/prep.c
>> @@ -358,6 +358,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);
>>             dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(1, 0),
>> 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);
>>   }
>
> Making a device non-user-creatable seems to be a step backwards: why not keep 
> the gpio for signalling the non-contiguous IO configuration?

This device implements a bunch of isa ports that control internal 
behaviour of the PReP machine. It does not make sense to add it to any 
other machine and as it needs to access some PReP specific internals it 
needs to be connected. Even a gpio would need to be connected so that does 
not change the reason for making it non-user-creatable. The previous way 
to change a memory region via a gpio was messier and needed access the 
memory region which is now self contained in the device where it belongs 
and passed via a link property which I think is clearer than the previous 
way.

Thanks again for the review. I've noted your Rb tags and made changes to 
the commit messages you asked and answered other questions. I'll wait for 
another round of answers before sending a v4 unless you want to see that 
before you answer.

Regards,
BALATON Zoltan