Re: [PATCH v2 2/5] pci: Always pass own DeviceState to pci_map_irq_fn's

[Bernhard Beschow]

Am 12. Februar 2022 17:13:19 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>>> Passing own DeviceState rather than just the IRQs allows for resolving
>>>> global variables.
>>>
>>> Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?
>>
>> I'm referring to the typedef in pci.h because the patch establishes
>> consistency across the board.
>>
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>> hw/isa/piix4.c          | 6 +++---
>>>> hw/pci-host/sh_pci.c    | 6 +++---
>>>> hw/pci-host/versatile.c | 6 +++---
>>>> hw/ppc/ppc440_pcix.c    | 6 +++---
>>>> hw/ppc/ppc4xx_pci.c     | 6 +++---
>>>> 5 files changed, 15 insertions(+), 15 deletions(-)
>>>
>>> You don't seem to change any global function definition that reqires this
>>> change in all these devices so why can't these decide on their own what
>>> their preferred opaque data is for their set irq function and only change
>>> piix4? Am I missing something? I'm not opposed to this change but it seems
>>> to be unnecessary to touch other device implementations for this and may
>>> just make them more complex for no good reason.
>>
>> This patch is a segway into a direction where the type of the opaque 
>> parameter
>> of the pci_map_irq_fn typedef could be changed from void* to DeviceState* in
>
>I'm still not quite sure what you mean considering these typedefs in 
>include/hw/pci/pci.h:
>
>typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
>typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
>
>pci_map_irq_fn already has PCIDevice *, it's pci_set_irq_fn that has void 
>*opaque and is what this patch appears to be changing. Am I looking at the 
>wrong typedefs?

Oh sorry, I mixed things up. You're correct: I meant pci_set_irq_fn.

>> order to facilitate the more typesafe QOM casting. I see, though, why this is
>> confusing in the context of this patch series. I don't have strong feelings 
>> for
>> converting the other devices or not. So I can only change piix4 if desired.
>
>Yes that seems to be an independent cahange so maybe it's better to just 
>change piix4 in this series and then have a separate patch for changing 
>the void *opaque to DeviceState * independent of this series. But I'm not 
>sure that's necessarily a good idea. It may give some more checks but for 
>callbacks used internally by device implementations I think it can be 
>expected that code that registers the callback also knows what its opaque 
>data should be so it does not have to be checked additionally, especially 
>in code that may be called frequently. Although in a similar via-ide 
>callback I could not measure a big penalty the last time I tried but I 
>suspect there still mey be some overhead involving QOM casts (maybe there 
>are just bigger bottle necks in ide emulation so it was masked) so I'm not 
>sure it's worth the added complexity but if others prefer that I'm not 
>that much opposed to it but it's clearer as a separate change anyway.

I'll just change piix4, leaving the other devices as is. This also allows for 
merging this patch with the next.

Regards,
Bernhard

>Regards,
>BALATON Zoltan