On Thu, Dec 19, 2024 at 05:24:11PM +0100, David Woodhouse wrote:
> From: David Woodhouse <d...@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.
>
> 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 <th...@redhat.com>
> Signed-off-by: David Woodhouse <d...@amazon.co.uk>
Xen things so feel free to merge.
Acked-by: Michael S. Tsirkin <m...@redhat.com>
> ---
> hw/i386/kvm/xen_evtchn.c | 48 +++++++++++++++++++++++++++++++---------
> hw/i386/kvm/xen_evtchn.h | 2 +-
> hw/i386/x86-common.c | 32 ++++++++++++++++++---------
> 3 files changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
> index 07bd0c9ab8..62b906e4ef 100644
> --- a/hw/i386/kvm/xen_evtchn.c
> +++ b/hw/i386/kvm/xen_evtchn.c
> @@ -140,6 +140,8 @@ struct XenEvtchnState {
>
> uint64_t callback_param;
> bool evtchn_in_kernel;
> + bool setting_callback_gsi;
> + int extern_gsi_level;
> uint32_t callback_gsi;
>
> QEMUBH *gsi_bh;
> @@ -431,7 +433,16 @@ void xen_evtchn_set_callback_level(int level)
> }
>
> if (s->callback_gsi && s->callback_gsi < s->nr_callback_gsis) {
> - qemu_set_irq(s->callback_gsis[s->callback_gsi], level);
> + /*
> + * Ugly, but since we hold the BQL we can set this flag so that
> + * xen_evtchn_set_gsi() can tell the difference between this code
> + * setting the GSI, and an external device (PCI INTx) doing so.
> + */
> + s->setting_callback_gsi = true;
> + /* Do not deassert the line if an external device is asserting it. */
> + qemu_set_irq(s->callback_gsis[s->callback_gsi],
> + level || s->extern_gsi_level);
> + s->setting_callback_gsi = false;
> if (level) {
> /* Ensure the vCPU polls for deassertion */
> kvm_xen_set_callback_asserted();
> @@ -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;
> @@ -1608,16 +1619,33 @@ bool xen_evtchn_set_gsi(int gsi, int level)
> }
>
> /*
> - * Check that that it *isn't* the event channel GSI, and thus
> - * that we are not recursing and it's safe to take s->port_lock.
> - *
> - * Locking aside, it's perfectly sane to bail out early for that
> - * special case, as it would make no sense for the event channel
> - * GSI to be routed back to event channels, when the delivery
> - * method is to raise the GSI... that recursion wouldn't *just*
> - * be a locking issue.
> + * For the callback_gsi we need to implement a logical OR of the event
> + * channel GSI and the external input (e.g. from PCI INTx), because
> + * QEMU itself doesn't support shared level interrupts via demux or
> + * resamplers.
> */
> if (gsi && gsi == s->callback_gsi) {
> + /* Remember the external state of the GSI pin (e.g. from PCI INTx) */
> + if (!s->setting_callback_gsi) {
> + s->extern_gsi_level = *level;
> +
> + /*
> + * Don't allow the external device to deassert the line if the
> + * eveht channel GSI should still be asserted.
> + */
> + if (!s->extern_gsi_level) {
> + struct vcpu_info *vi = kvm_xen_get_vcpu_info_hva(0);
> + if (vi && vi->evtchn_upcall_pending) {
> + *level = 1;
> + }
> + }
> + }
> +
> + /*
> + * The event channel GSI cannot be routed to PIRQ, as that would make
> + * no sense. It could also deadlock on s->port_lock, if we proceed.
> + * So bail out now.
> + */
> return false;
> }
>
> diff --git a/hw/i386/kvm/xen_evtchn.h b/hw/i386/kvm/xen_evtchn.h
> index b740acfc0d..0521ebc092 100644
> --- a/hw/i386/kvm/xen_evtchn.h
> +++ b/hw/i386/kvm/xen_evtchn.h
> @@ -23,7 +23,7 @@ void xen_evtchn_set_callback_level(int level);
>
> int xen_evtchn_set_port(uint16_t port);
>
> -bool xen_evtchn_set_gsi(int gsi, int level);
> +bool xen_evtchn_set_gsi(int gsi, int *level);
> void xen_evtchn_snoop_msi(PCIDevice *dev, bool is_msix, unsigned int vector,
> uint64_t addr, uint32_t data, bool is_masked);
> void xen_evtchn_remove_pci_device(PCIDevice *dev);
> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> index 3f78182692..e061580f67 100644
> --- a/hw/i386/x86-common.c
> +++ b/hw/i386/x86-common.c
> @@ -450,8 +450,27 @@ static long get_file_size(FILE *f)
> void gsi_handler(void *opaque, int n, int level)
> {
> GSIState *s = opaque;
> + bool bypass_ioapic = false;
>
> trace_x86_gsi_interrupt(n, level);
> +
> +#ifdef CONFIG_XEN_EMU
> + /*
> + * Xen delivers the GSI to the Legacy PIC (not that Legacy PIC
> + * routing actually works properly under Xen). And then to
> + * *either* the PIRQ handling or the I/OAPIC depending on
> + * whether the former wants it.
> + *
> + * Additionally, this hook allows the Xen event channel GSI to
> + * work around QEMU's lack of support for shared level interrupts,
> + * by keeping track of the externally driven state of the pin and
> + * implementing a logical OR with the state of the evtchn GSI.
> + */
> + if (xen_mode == XEN_EMULATE) {
> + bypass_ioapic = xen_evtchn_set_gsi(n, &level);
> + }
> +#endif
> +
> switch (n) {
> case 0 ... ISA_NUM_IRQS - 1:
> if (s->i8259_irq[n]) {
> @@ -460,18 +479,9 @@ void gsi_handler(void *opaque, int n, int level)
> }
> /* fall through */
> case ISA_NUM_IRQS ... IOAPIC_NUM_PINS - 1:
> -#ifdef CONFIG_XEN_EMU
> - /*
> - * Xen delivers the GSI to the Legacy PIC (not that Legacy PIC
> - * routing actually works properly under Xen). And then to
> - * *either* the PIRQ handling or the I/OAPIC depending on
> - * whether the former wants it.
> - */
> - if (xen_mode == XEN_EMULATE && xen_evtchn_set_gsi(n, level)) {
> - break;
> + if (!bypass_ioapic) {
> + qemu_set_irq(s->ioapic_irq[n], level);
> }
> -#endif
> - qemu_set_irq(s->ioapic_irq[n], level);
> break;
> case IO_APIC_SECONDARY_IRQBASE
> ... IO_APIC_SECONDARY_IRQBASE + IOAPIC_NUM_PINS - 1:
> --
> 2.47.0
>
>
