Re: [PATCH] tests/functional: Convert the kvm_xen_guest avocado test

[David Woodhouse]

On Thu, 2024-12-19 at 13:24 +0100, Thomas Huth wrote:
> On 19/12/2024 09.49, David Woodhouse wrote:
> > On 19 December 2024 09:35:13 CET, Thomas Huth <thuth@redhat.com> wrote:
> > > On 18/12/2024 23.14, David Woodhouse wrote:
> > > > On Wed, 2024-12-18 at 16:54 +0100, Thomas Huth wrote:
> > > > > On 18/12/2024 12.48, David Woodhouse wrote:
> > > > > > On 18 December 2024 12:32:49 CET, Thomas Huth <thuth@redhat.com> 
> > > > > > wrote:
> > > > > > > Use the serial console to execute the commands in the guest 
> > > > > > > instead
> > > > > > > of using ssh since we don't have ssh support in the functional
> > > > > > > framework yet.
> > > > > > > 
> > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > > > 
> > > > > > Hm, but serial is lossy and experience shows that it leads to flaky 
> > > > > > tests if the guest (or host) misses bytes. While SSH would just go 
> > > > > > slower.
> > > > > 
> > > > > I now noticed some issue with the serial console in this test, too.
> > > > > Looks like the "Starting dropbear sshd: OK" is not print in an atomic 
> > > > > way by
> > > > > the guest, sometimes there are other kernel messages between the ":" 
> > > > > and the
> > > > > "OK". It works reliable when removing the "OK" from the string.
> > > > 
> > > > Nah, that still isn't atomic; you just got lucky because the race
> > > > window is smaller. It's not like serial ports are at a premium; can't
> > > > you have a separate port for kernel vs. userspace messages?
> > > 
> > > Maybe easiest solution: Simply add "quiet" to the kernel command line, 
> > > then it does not write the kernel messages to the serial console anymore.
> > 
> > Want to resend the bug report about that test failing again? But without 
> > the kernel messages this time... :)
> 
> With "quiet", the output just looks like this when it hangs:
> 
>   Cannot get hvm parameter CONSOLE_EVTCHN (18): -22!
>   Spectre V2 : Kernel not compiled with retpoline; no mitigation available!
>   kvm_intel: VMX not supported by CPU 0
>   Cannot get hvm parameter CONSOLE_EVTCHN (18): -22!
>   fail to initialize ptp_kvm

Yeah, that request was rhetorical. That output is useless for
understanding anything about what happened.

> Anyway, to properly track this, I've now created a ticket with the full log:
> 
>   https://gitlab.com/qemu-project/qemu/-/issues/2731

The patch below should fix it. I don't like it very much; it's very
much papering over a much bigger generic problem with QEMU's handling
of shared interrupts.

Basically, *nothing* should just directly set the system GSIs to
"their" desired level with qemu_set_irq(). Each device should feed into
a multiplexer which is essentially an OR gate, and the *output* of that
mux goes into the actual GSI.

Alternatively, when the interrupt line is acked at the interrupt
controller, we should do a callback to query whether that line should
still be asserted (which is exactly how VFIO's resampler should work,
but we can't use it correctly from QEMU and currently just invoke it
for *every* MMIO access to a VFIO device!).

This just implements the logical OR for the Xen event channel callback
GSI, since the Xen code *already* has a hook in the x86 gsi_handler()
function to capture external interrupts being routed to event channel
PIRQs. So we just shuffle that a little and let it 'adjust' the level
that's being set.


diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index 07bd0c9ab8..e3bd48d954 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,9 @@ 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);
+        s->setting_callback_gsi = true;
+        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 +1600,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 +1612,29 @@ 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.
+     * 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.
      *
-     * 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.
+     * Remember the level which is being set by *other* callers.
+     *
+     * The event channel GSI cannot be routed to PIRQ, as that would make
+     * no sense. It would also be a locking problem so bail out early and
+     * don't potentially deadlock on s->port_lock.
      */
     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;
+            if (!s->extern_gsi_level) {
+                struct vcpu_info *vi = kvm_xen_get_vcpu_info_hva(0);
+                if (vi && vi->evtchn_upcall_pending) {
+                    *level = 1;
+                }
+            }
+        }
+
         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 dc031af662..77acd331ee 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -450,28 +450,36 @@ 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);
-    switch (n) {
-    case 0 ... ISA_NUM_IRQS - 1:
-        if (s->i8259_irq[n]) {
-            /* Under KVM, Kernel will forward to both PIC and IOAPIC */
-            qemu_set_irq(s->i8259_irq[n], 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.
+         *
+         * 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 && xen_evtchn_set_gsi(n, level)) {
-            break;
-        }
+    if (xen_mode == XEN_EMULATE)
+        bypass_ioapic = xen_evtchn_set_gsi(n, &level);
 #endif
-        qemu_set_irq(s->ioapic_irq[n], level);
+
+    switch (n) {
+    case 0 ... ISA_NUM_IRQS - 1:
+        if (s->i8259_irq[n]) {
+            /* Under KVM, Kernel will forward to both PIC and IOAPIC */
+            qemu_set_irq(s->i8259_irq[n], level);
+        }
+        /* fall through */
+    case ISA_NUM_IRQS ... IOAPIC_NUM_PINS - 1:
+        if (!bypass_ioapic)
+            qemu_set_irq(s->ioapic_irq[n], level);
         break;
     case IO_APIC_SECONDARY_IRQBASE
         ... IO_APIC_SECONDARY_IRQBASE + IOAPIC_NUM_PINS - 1:

smime.p7s
Description: S/MIME cryptographic signature