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
© 2016 - 2025 Red Hat, Inc.