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.3On 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.
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
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
>
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.
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
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
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
© 2016 - 2026 Red Hat, Inc.