Am 12. Februar 2022 17:13:19 MEZ schrieb BALATON Zoltan <bala...@eik.bme.hu>: >On Sat, 12 Feb 2022, Bernhard Beschow wrote: >> Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <bala...@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 <shen...@gmail.com> >>>> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> >>>> Reviewed-by: Philippe Mathieu-Daudé <f4...@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
