[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH v2 2/5] pci: Always pass own DeviceState to pci_map_irq_fn's
Date: Sun, 13 Feb 2022 16:22:02 +0100 (CET)

On Sun, 13 Feb 2022, Bernhard Beschow wrote:
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

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.

I'm not sure what will be the next patch after all the changes we were talking about but don't put too many changes in one patch. It may worth keeping this as a separate change even if it's simple and only affecting piix4 now just to make it clear and simpler to review and maybe bisect later. Generally one change per patch is preferred even if it results in a lot of small patches (unless maybe combining a small style fix or very simple one line change with another simple patch that may not worth a separate patch). This may be over that limit so better to keep as separate patch but again not sure what patch you meant to combine it with but the patch submission guidelines say separate changes should be separate patches.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]