All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bernhard Beschow <shentey@gmail.com>
To: BALATON Zoltan <balaton@eik.bme.hu>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Yoshinori Sato" <ysato@users.sourceforge.jp>,
	"Magnus Damm" <magnus.damm@gmail.com>,
	qemu-devel@nongnu.org, "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"open list:Versatile PB" <qemu-arm@nongnu.org>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"open list:ppc4xx" <qemu-ppc@nongnu.org>,
	"Aurelien Jarno" <aurelien@aurel32.net>
Subject: Re: [PATCH v2 2/5] pci: Always pass own DeviceState to pci_map_irq_fn's
Date: Sun, 13 Feb 2022 15:31:41 +0100	[thread overview]
Message-ID: <554F5BFB-D5CD-470B-8EEC-2218BBC67A2F@gmail.com> (raw)
In-Reply-To: <d13634a-b414-7663-42c-6390c5d58a1e@eik.bme.hu>

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


  reply	other threads:[~2022-02-13 14:33 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-12 11:35 [PATCH v2 0/5] malta: Fix PCI IRQ levels to be preserved during migration Bernhard Beschow
2022-02-12 11:35 ` [PATCH v2 1/5] malta: Move PCI interrupt handling from gt64xxx to piix4 Bernhard Beschow
2022-02-12 13:18   ` BALATON Zoltan
2022-02-12 16:44     ` BALATON Zoltan
2022-02-13 14:21       ` Bernhard Beschow
2022-02-13 14:34     ` Bernhard Beschow
2022-02-12 11:35 ` [PATCH v2 2/5] pci: Always pass own DeviceState to pci_map_irq_fn's Bernhard Beschow
2022-02-12 13:27   ` BALATON Zoltan
2022-02-12 14:23     ` Bernhard Beschow
2022-02-12 16:13       ` BALATON Zoltan
2022-02-13 14:31         ` Bernhard Beschow [this message]
2022-02-13 15:22           ` BALATON Zoltan
2022-02-12 16:16     ` Peter Maydell
2022-02-12 11:35 ` [PATCH v2 3/5] isa/piix4: Resolve global variables Bernhard Beschow
2022-02-12 11:35 ` [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState Bernhard Beschow
2022-02-12 13:42   ` BALATON Zoltan
2022-02-12 16:41     ` Peter Maydell
2022-02-12 17:02       ` BALATON Zoltan
2022-02-12 18:30         ` Peter Maydell
2022-02-12 21:44           ` BB
2022-02-12 21:46           ` Bernhard Beschow
2022-02-12 11:35 ` [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute Bernhard Beschow
2022-02-12 13:19   ` BALATON Zoltan
2022-02-12 14:02     ` BB
2022-02-12 14:05     ` Bernhard Beschow
2022-02-12 15:57       ` BALATON Zoltan
2022-02-12 15:58         ` BALATON Zoltan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=554F5BFB-D5CD-470B-8EEC-2218BBC67A2F@gmail.com \
    --to=shentey@gmail.com \
    --cc=aurelien@aurel32.net \
    --cc=balaton@eik.bme.hu \
    --cc=f4bug@amsat.org \
    --cc=hpoussin@reactos.org \
    --cc=magnus.damm@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=ysato@users.sourceforge.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.