[PATCH 0/2] KVM: x86: Fix stale RTC pending-EOI tracking under AVIC

[PATCH 0/2] KVM: x86: Fix stale RTC pending-EOI tracking under AVIC

[Xiang Zheng]

ioapic_lazy_update_eoi() exists because AVIC completes EOIs of
edge-triggered interrupts in hardware, without a VM exit.  It clears
the RTC pending-EOI record of vCPUs that no longer have the vector in
IRR/ISR, but identifies those vCPUs with the current redirection entry
instead of the delivery record kept in rtc_status.  A guest that
retargets the RTC after such an EOI therefore leaves the pending EOI
set, and every later RTC interrupt is dropped as coalesced.  A guest
that relies on the RTC timer interrupt to advance timekeeping, e.g. a
Windows guest without HPET and with an unused APIC timer, then loses
its timer and hangs.

Patch 1 makes the lazy update consult the delivery record.  Patch 2
adds a selftest that reproduces the bookkeeping failure without a
Windows guest: userspace injects GSI 8, the target vCPU handles it and
writes the EOI, the guest retargets the RTC, and the next injection
must still be reported as delivered by KVM_IRQ_LINE_STATUS.

Tested on an AMD host running Linux 7.3-rc7: with AVIC enabled, the
unpatched kernel fails the selftest at the second injection (status 0,
coalesced) and the patched kernel passes all 64 rounds; with AVIC
disabled both pass.  Not tested on Intel/APICv.

Thanks,
Xiang

Xiang Zheng (2):
  KVM: x86: ioapic: Clear stale RTC pending EOIs using the delivery
    record
  KVM: selftests: x86: Add RTC EOI retargeting regression test

 arch/x86/kvm/ioapic.c                         |  19 +--
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/x86/rtc_eoi_retarget_test.c | 155 ++++++++++++++++++
 3 files changed, 164 insertions(+), 11 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86/rtc_eoi_retarget_test.c


base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.50.1 (Apple Git-155)

[PATCH 1/2] KVM: x86: ioapic: Clear stale RTC pending EOIs using the delivery record

[Xiang Zheng]

Use the vCPU/vector pairs recorded in rtc_status to clear completed RTC
pending EOIs instead of consulting the current redirection entry.

AVIC completes EOIs of edge-triggered interrupts in hardware, without a
VM exit, so KVM cannot decrement the RTC pending-EOI count from an EOI
exit.  ioapic_lazy_update_eoi() compensates at the next injection by
clearing the pending EOI of any vCPU that no longer has the vector in
IRR/ISR, but it identifies those vCPUs with the current redirection
entry instead of the delivery record kept in rtc_status.

If the guest rewrites the RTC redirection entry after an interrupt was
delivered and EOI'd without an exit, e.g. because the guest moves the
RTC to another processor, the new route no longer matches the recorded
delivery, so the pending EOI is left set and every later RTC interrupt
is reported as coalesced and dropped.  A guest that relies on the RTC
timer interrupt to advance timekeeping, e.g. a Windows guest without
HPET and with an unused APIC timer, then loses its timer and hangs.

Walk the vCPUs present in rtc_status and test the vector stored at
delivery.  While at it, clear every completed record instead of bailing
out after the first one, and skip pins other than the RTC, the only pin
with delivery tracking.

Fixes: f458d039db7e ("kvm: ioapic: Lazy update IOAPIC EOI")
Signed-off-by: Xiang Zheng <xiang.zheng@linux.alibaba.com>
---
 arch/x86/kvm/ioapic.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 0d59b9c758c2..803fda172db5 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -167,21 +167,18 @@ static void ioapic_lazy_update_eoi(struct kvm_ioapic *ioapic, int irq)
 {
 	unsigned long i;
 	struct kvm_vcpu *vcpu;
-	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
+	struct rtc_status *status = &ioapic->rtc_status;
+
+	if (irq != RTC_GSI || !status->pending_eoi)
+		return;
 
+	/* Use the target recorded at delivery; the entry may have changed. */
 	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
-		if (!kvm_apic_match_dest(vcpu, NULL, APIC_DEST_NOSHORT,
-					 entry->fields.dest_id,
-					 entry->fields.dest_mode) ||
-		    kvm_apic_pending_eoi(vcpu, entry->fields.vector))
+		if (!test_bit(vcpu->vcpu_id, status->map) ||
+		    kvm_apic_pending_eoi(vcpu, status->vectors[vcpu->vcpu_id]))
 			continue;
 
-		/*
-		 * If no longer has pending EOI in LAPICs, update
-		 * EOI for this vector.
-		 */
-		rtc_irq_eoi(ioapic, vcpu, entry->fields.vector);
-		break;
+		rtc_irq_eoi(ioapic, vcpu, status->vectors[vcpu->vcpu_id]);
 	}
 }
 
-- 
2.50.1 (Apple Git-155)

[PATCH 2/2] KVM: selftests: x86: Add RTC EOI retargeting regression test

[Xiang Zheng]

Inject the RTC interrupt (GSI 8) from userspace, let the target vCPU
handle it and write the EOI, then have the guest retarget the RTC to
the other vCPU before the next injection.  Every injection must be
reported as delivered by KVM_IRQ_LINE_STATUS, and each target vCPU must
see its own interrupt count advance, for 64 rounds.

Without the ioapic fix, and with AVIC enabled, the second injection is
reported as coalesced and the test fails; it passes with AVIC disabled
or with the fix applied.

Signed-off-by: Xiang Zheng <xiang.zheng@linux.alibaba.com>
---
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/x86/rtc_eoi_retarget_test.c | 155 ++++++++++++++++++
 2 files changed, 156 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86/rtc_eoi_retarget_test.c

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index f43549a56f0b..7bc2db4c57be 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -140,6 +140,7 @@ TEST_GEN_PROGS_x86 += x86/apic_bus_clock_test
 TEST_GEN_PROGS_x86 += x86/xapic_ipi_test
 TEST_GEN_PROGS_x86 += x86/xapic_state_test
 TEST_GEN_PROGS_x86 += x86/xapic_tpr_test
+TEST_GEN_PROGS_x86 += x86/rtc_eoi_retarget_test
 TEST_GEN_PROGS_x86 += x86/xcr0_cpuid_test
 TEST_GEN_PROGS_x86 += x86/xss_msr_test
 TEST_GEN_PROGS_x86 += x86/debug_regs
diff --git a/tools/testing/selftests/kvm/x86/rtc_eoi_retarget_test.c b/tools/testing/selftests/kvm/x86/rtc_eoi_retarget_test.c
new file mode 100644
index 000000000000..564bc7199c1c
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86/rtc_eoi_retarget_test.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <stdio.h>
+#include <unistd.h>
+
+#include "apic.h"
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+
+#define IOAPIC_GPA		0xfec00000ULL
+#define IOAPIC_REGSEL		0x00
+#define IOAPIC_WINDOW		0x10
+#define IOAPIC_REDIR_LO(pin)	(0x10 + 2 * (pin))
+#define IOAPIC_REDIR_HI(pin)	(0x11 + 2 * (pin))
+
+#define RTC_GSI			8
+#define RTC_VECTOR		0xd1
+#define NR_VCPUS		2
+#define NR_ROUNDS		64
+
+static u32 irq_count[NR_VCPUS];
+
+static void pio_outb(u16 port, u8 val)
+{
+	asm volatile("outb %0, %1" : : "a"(val), "Nd"(port));
+}
+
+static void ioapic_write(u32 reg, u32 val)
+{
+	WRITE_ONCE(*(u32 *)(IOAPIC_GPA + IOAPIC_REGSEL), reg);
+	WRITE_ONCE(*(u32 *)(IOAPIC_GPA + IOAPIC_WINDOW), val);
+}
+
+static void rtc_handler(struct ex_regs *regs)
+{
+	u32 me = GET_APIC_ID_FIELD(xapic_read_reg(APIC_ID));
+
+	WRITE_ONCE(irq_count[me], READ_ONCE(irq_count[me]) + 1);
+	xapic_write_reg(APIC_EOI, 0);
+}
+
+static void guest_code(void)
+{
+	u32 me, expected = 0;
+
+	xapic_enable();
+	me = GET_APIC_ID_FIELD(xapic_read_reg(APIC_ID));
+
+	if (!me) {
+		/* Mask the PIC so only the IOAPIC contributes to IRQ status. */
+		pio_outb(0x21, 0xff);
+		pio_outb(0xa1, 0xff);
+		ioapic_write(IOAPIC_REDIR_HI(RTC_GSI), 0);
+		ioapic_write(IOAPIC_REDIR_LO(RTC_GSI), RTC_VECTOR);
+	}
+	sti();
+	GUEST_SYNC1(0);
+
+	for (;;) {
+		expected++;
+		while (READ_ONCE(irq_count[me]) < expected)
+			cpu_relax();
+		/* Change the route before the next lazy EOI update. */
+		ioapic_write(IOAPIC_REDIR_HI(RTC_GSI), (me ^ 1) << 24);
+		GUEST_SYNC1(READ_ONCE(irq_count[me]));
+	}
+}
+
+static void run_until_sync(struct kvm_vcpu *vcpu, u64 expected)
+{
+	struct ucall uc;
+
+	vcpu_run(vcpu);
+	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
+
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_SYNC:
+		TEST_ASSERT(uc.args[0] == expected,
+			    "vCPU%u: got %lu RTC interrupts, expected %lu",
+			    vcpu->id, (unsigned long)uc.args[0],
+			    (unsigned long)expected);
+		break;
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+	default:
+		TEST_FAIL("Unexpected ucall %lu", uc.cmd);
+	}
+}
+
+static int inject_rtc(struct kvm_vm *vm)
+{
+	struct kvm_irq_level irq = { .irq = RTC_GSI, .level = 1 };
+	int status;
+
+	vm_ioctl(vm, KVM_IRQ_LINE_STATUS, &irq);
+	status = irq.status;
+
+	/*
+	 * Deassert (level 0) to clear the IOAPIC IRR bit.  An edge assert
+	 * leaves IRR set, so without this the next round would be reported
+	 * as already pending, not delivered.  .irq and .status share a union.
+	 */
+	irq.irq = RTC_GSI;
+	irq.level = 0;
+	vm_ioctl(vm, KVM_IRQ_LINE_STATUS, &irq);
+
+	return status;
+}
+
+int main(void)
+{
+	struct kvm_vcpu *vcpus[NR_VCPUS];
+	struct kvm_irqchip chip = { .chip_id = KVM_IRQCHIP_IOAPIC };
+	struct kvm_vm *vm;
+	int round, status, r;
+
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_IRQCHIP));
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_IRQ_INJECT_STATUS));
+
+	vm = vm_create_with_vcpus(NR_VCPUS, guest_code, vcpus);
+	vm_install_exception_handler(vm, RTC_VECTOR, rtc_handler);
+	virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA);
+	virt_pg_map(vm, IOAPIC_GPA, IOAPIC_GPA);
+
+	/*
+	 * Require a full in-kernel IOAPIC; vm_create_irqchip() silently falls
+	 * back to a split irqchip when KVM_CREATE_IRQCHIP is unavailable.
+	 */
+	r = __vm_ioctl(vm, KVM_GET_IRQCHIP, &chip);
+	__TEST_REQUIRE(!r || (errno != ENOTTY && errno != ENXIO),
+		       "Full in-kernel IRQCHIP is required");
+	TEST_ASSERT_VM_VCPU_IOCTL(!r, KVM_GET_IRQCHIP, r, vm);
+
+	alarm(30);
+	/* vCPU0 programs the IOAPIC, so it must run first. */
+	run_until_sync(vcpus[0], 0);
+	run_until_sync(vcpus[1], 0);
+
+	for (round = 0; round < NR_ROUNDS; round++) {
+		int target = round & 1;
+
+		status = inject_rtc(vm);
+		TEST_ASSERT(status == 1,
+			    "round %d: RTC to vCPU%d not delivered, status %d%s",
+			    round, target, status,
+			    status == 0 ? " (coalesced)" : "");
+		run_until_sync(vcpus[target], round / 2 + 1);
+	}
+	alarm(0);
+
+	printf("PASS: %d RTC injections delivered across target changes\n",
+	       NR_ROUNDS);
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.50.1 (Apple Git-155)