Re: [PATCH] hw/i386/pc: Fix level interrupt sharing for Xen event channel GSI

[Bernhard Beschow <shentey@gmail.com>]

Am 7. Januar 2025 16:20:28 UTC schrieb David Woodhouse <dwmw2@infradead.org>:
>On Tue, 2025-01-07 at 11:07 -0500, Michael S. Tsirkin wrote:
>> On Thu, Dec 19, 2024 at 05:24:11PM +0100, David Woodhouse wrote:
>> > From: David Woodhouse <dwmw@amazon.co.uk>
>> > 
>> > The system GSIs are not designed for sharing. One device might assert a
>> > shared interrupt with qemu_set_irq() and another might deassert it, and
>> > the level from the first device is lost.
>> > 
>> > This could be solved by using a multiplexer which functions as an OR
>> > gate, much like the PCI code already implements for pci_set_irq() for
>> > muxing the INTx lines.

Just curious: Why not use that aporoach? Could <https://lore.kernel.org/qemu-devel/20250108092538.11474-5-shentey@gmail.com/> help?

Best regards,
Bernhard

>> > 
>> > Alternatively, it could be solved by having a 'resample' callback which
>> > is invoked when the interrupt is acked at the interrupt controller, and
>> > causes the devices to re-trigger the interrupt if it should still be
>> > pending. This is the model that VFIO in Linux uses, with a 'resampler'
>> > eventfd that actually unmasks the interrupt on the hardware device and
>> > thus triggers a new interrupt from it if needed. QEMU currently doesn't
>> > use that VFIO interface correctly, and just bashes on the resampler for
>> > every MMIO access to the device "just in case".
>> > 
>> > This does neither of those. The Xen event channel GSI support *already*
>> > has hooks into the PC gsi_handler() code, for routing GSIs to PIRQs. So
>> > we can implement the logical OR of the external input (from PCI INTx,
>> > serial etc.) with the Xen event channel GSI by allowing that existing
>> > hook to modify the 'level' being asserted.
>> > 
>> > Closes: https://gitlab.com/qemu-project/qemu/-/issues/2731
>> > Reported-by: Thomas Huth <thuth@redhat.com>
>> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> 
>> Xen things so feel free to merge.
>> 
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
>Thanks.
>
>Further testing shows I need one minor fix...
>
>> > @@ -1596,7 +1607,7 @@ static int allocate_pirq(XenEvtchnState *s, int type, int gsi)
>> >      return pirq;
>> >  }
>> >  
>> > -bool xen_evtchn_set_gsi(int gsi, int level)
>> > +bool xen_evtchn_set_gsi(int gsi, int *level)
>> >  {
>> >      XenEvtchnState *s = xen_evtchn_singleton;
>> >      int pirq;
>
>...
>
>@@ -1628,7 +1656,7 @@ bool xen_evtchn_set_gsi(int gsi, int level)
>         return false;
>     }
> 
>-    if (level) {
>+    if (*level) {
>         int port = s->pirq[pirq].port;
> 
>         s->pirq_gsi_set |= (1U << gsi);
>
>
>