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

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

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
>