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

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

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