[PATCH v5 0/6] KVM: x86: Fix IOAPIC remote_irr and irr_delivered handling

[PATCH v5 0/6] KVM: x86: Fix IOAPIC remote_irr and irr_delivered handling

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

Fix several cases where KVM updates IOAPIC interrupt state incorrectly.

The first fix avoids setting remote_irr when level-triggered interrupt
delivery fails, which can leave the pin blocked waiting for an EOI that
will never arrive.  The remaining fixes clean up irr_delivered handling
for edge-triggered interrupts, which affects the IOAPIC state exposed to
userspace through KVM_GET_IRQCHIP.

Changes in v5:
- Updated patch 1 changelog to explain the Fixes tag history and added
  Kai's Reviewed-by.
- Split selftest changes for the irr_delivered fixes into separate
  patches.
- Dropped the Fixes tag from the duplicate-edge patch, as the exact
  historical commit is unclear.

Hao Zhang (6):
  KVM: x86: ioapic: Update remote_irr only after successful delivery
  KVM: selftests: Verify failed IOAPIC delivery preserves state
  KVM: x86: ioapic: Preserve irr_delivered for duplicate edge interrupts
  KVM: selftests: Verify duplicate edge preserves IOAPIC state
  KVM: x86: ioapic: Clear irr_delivered on level-triggered RTE
  KVM: selftests: Verify edge-to-level clears IOAPIC state

 arch/x86/kvm/ioapic.c                              |  15 +-
 tools/testing/selftests/kvm/Makefile.kvm           |   1 +
 .../testing/selftests/kvm/x86/ioapic_state_test.c  | 190 +++++++++++++++++++++
 3 files changed, 203 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86/ioapic_state_test.c


base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86

-- 
2.15.0

[PATCH v5 1/6] KVM: x86: ioapic: Update remote_irr only after successful delivery

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

The I/O APIC sets remote_irr for level-triggered interrupts that have
been accepted by a local APIC and are waiting for an EOI.

ioapic_service() currently treats any non-zero return from
kvm_irq_delivery_to_apic() as successful delivery.  But negative return
values are failures, not successful delivery.  Setting remote_irr after
such a failed delivery leaves the pin blocked forever waiting for an EOI
that will never be generated.

The bug dates back to commit 4925663a079c ("KVM: Report IRQ injection
status to userspace."), which made ioapic_service() preserve and return
the delivery result, initialized the result to -1, but kept using
"ret != 0" as the condition for setting remote_irr.  As a result, any
negative delivery failure that reaches this path is incorrectly treated
as a delivered level-triggered interrupt.

Update remote_irr only when the delivery result is positive, i.e. when
at least one local APIC accepted the interrupt.

Fixes: 4925663a079c ("KVM: Report IRQ injection status to userspace.")
Reviewed-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
 arch/x86/kvm/ioapic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 757667fb2bfa..24a7cc3b8b7e 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -491,7 +491,7 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 	} else
 		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
 
-	if (ret && irqe.trig_mode == IOAPIC_LEVEL_TRIG)
+	if (ret > 0 && irqe.trig_mode == IOAPIC_LEVEL_TRIG)
 		entry->fields.remote_irr = 1;
 
 	return ret;
-- 
2.15.0

[PATCH v5 2/6] KVM: selftests: Verify failed IOAPIC delivery preserves state

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

Add an x86 selftest for the in-kernel I/O APIC state that is exposed
through KVM_GET_IRQCHIP.

Create a VM with an in-kernel irqchip and no vCPUs, route an unmasked
level-triggered I/O APIC pin to a non-existent APIC ID, and verify that
failed delivery does not set remote_irr.

Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
 tools/testing/selftests/kvm/Makefile.kvm           |  1 +
 .../testing/selftests/kvm/x86/ioapic_state_test.c  | 92 ++++++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86/ioapic_state_test.c

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 6fc34e9bf8e1..89e3f82e75b0 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -86,6 +86,7 @@ TEST_GEN_PROGS_x86 += x86/hyperv_features
 TEST_GEN_PROGS_x86 += x86/hyperv_ipi
 TEST_GEN_PROGS_x86 += x86/hyperv_svm_test
 TEST_GEN_PROGS_x86 += x86/hyperv_tlb_flush
+TEST_GEN_PROGS_x86 += x86/ioapic_state_test
 TEST_GEN_PROGS_x86 += x86/kvm_clock_test
 TEST_GEN_PROGS_x86 += x86/kvm_pv_test
 TEST_GEN_PROGS_x86 += x86/kvm_buslock_test
diff --git a/tools/testing/selftests/kvm/x86/ioapic_state_test.c b/tools/testing/selftests/kvm/x86/ioapic_state_test.c
new file mode 100644
index 000000000000..be5237f34ad2
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86/ioapic_state_test.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Regression tests for in-kernel I/O APIC state.
+ */
+
+#include "kvm_util.h"
+#include "test_util.h"
+
+#define TEST_IOAPIC_PIN		16
+#define TEST_VECTOR		0x50
+#define NO_SUCH_APIC_ID		0xfe
+#define TEST_IOAPIC_EDGE_TRIG	0
+#define TEST_IOAPIC_LEVEL_TRIG	1
+
+static void get_ioapic(struct kvm_vm *vm, struct kvm_irqchip *irqchip)
+{
+	int r;
+
+	irqchip->chip_id = KVM_IRQCHIP_IOAPIC;
+	r = __vm_ioctl(vm, KVM_GET_IRQCHIP, irqchip);
+	if (r && errno == ENXIO)
+		__TEST_REQUIRE(0, "In-kernel I/O APIC not available");
+
+	TEST_ASSERT(!r, KVM_IOCTL_ERROR(KVM_GET_IRQCHIP, r));
+}
+
+static void set_ioapic(struct kvm_vm *vm, struct kvm_irqchip *irqchip)
+{
+	irqchip->chip_id = KVM_IRQCHIP_IOAPIC;
+	vm_ioctl(vm, KVM_SET_IRQCHIP, irqchip);
+}
+
+static void set_ioapic_entry(struct kvm_vm *vm, bool level_triggered,
+			     u32 dest_id)
+{
+	struct kvm_irqchip irqchip;
+
+	get_ioapic(vm, &irqchip);
+
+	irqchip.chip.ioapic.redirtbl[TEST_IOAPIC_PIN].fields.vector =
+		TEST_VECTOR;
+	irqchip.chip.ioapic.redirtbl[TEST_IOAPIC_PIN].fields.dest_id =
+		dest_id;
+	irqchip.chip.ioapic.redirtbl[TEST_IOAPIC_PIN].fields.dest_mode = 0;
+	irqchip.chip.ioapic.redirtbl[TEST_IOAPIC_PIN].fields.trig_mode =
+		level_triggered ? TEST_IOAPIC_LEVEL_TRIG :
+				  TEST_IOAPIC_EDGE_TRIG;
+	irqchip.chip.ioapic.redirtbl[TEST_IOAPIC_PIN].fields.mask = 0;
+	irqchip.chip.ioapic.redirtbl[TEST_IOAPIC_PIN].fields.remote_irr = 0;
+
+	set_ioapic(vm, &irqchip);
+}
+
+static int kvm_irq_line_status(struct kvm_vm *vm, int level)
+{
+	struct kvm_irq_level irq = {
+		.irq = TEST_IOAPIC_PIN,
+		.level = level,
+	};
+
+	vm_ioctl(vm, KVM_IRQ_LINE_STATUS, &irq);
+	return irq.status;
+}
+
+static void test_no_remote_irr_for_undelivered_level_interrupt(void)
+{
+	struct kvm_irqchip irqchip;
+	struct kvm_vm *vm;
+	int status;
+
+	vm = vm_create_barebones();
+	vm_create_irqchip(vm);
+
+	set_ioapic_entry(vm, true, NO_SUCH_APIC_ID);
+
+	status = kvm_irq_line_status(vm, 1);
+	TEST_ASSERT(status == -1,
+		    "Expected failed interrupt delivery, got %d", status);
+
+	get_ioapic(vm, &irqchip);
+	TEST_ASSERT(!irqchip.chip.ioapic.redirtbl[TEST_IOAPIC_PIN].fields.remote_irr,
+		    "KVM set remote_irr for a level-triggered interrupt that wasn't delivered");
+
+	kvm_vm_free(vm);
+}
+
+int main(void)
+{
+	test_no_remote_irr_for_undelivered_level_interrupt();
+
+	return 0;
+}
-- 
2.15.0

[PATCH v5 3/6] KVM: x86: ioapic: Preserve irr_delivered for duplicate edge interrupts

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

For edge-triggered interrupts, ioapic_set_irq() clears irr_delivered
before checking whether the new edge is a duplicate.  A duplicate edge
is then coalesced without being serviced, leaving the interrupt visible
in KVM_GET_IRQCHIP state even though the original interrupt was already
delivered.

Move the state update after the duplicate-edge check so duplicate edges
do not make a delivered interrupt appear pending during VM migration.

Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
 arch/x86/kvm/ioapic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 24a7cc3b8b7e..676effd674f9 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -230,11 +230,11 @@ static int ioapic_set_irq(struct kvm_ioapic *ioapic, unsigned int irq,
 	old_irr = ioapic->irr;
 	ioapic->irr |= mask;
 	if (edge) {
-		ioapic->irr_delivered &= ~mask;
 		if (old_irr == ioapic->irr) {
 			ret = 0;
 			goto out;
 		}
+		ioapic->irr_delivered &= ~mask;
 	}
 
 	ret = ioapic_service(ioapic, irq, line_status);
-- 
2.15.0

Re: [PATCH v5 3/6] KVM: x86: ioapic: Preserve irr_delivered for duplicate edge interrupts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] KVM's IOAPIC fails to latch edge-triggered interrupts if they pulse while the pin is masked, and fails to deliver latched edge-triggered interrupts upon unmasking.
--

--- Patch [3]: [PATCH v5 3/6] KVM: x86: ioapic: Preserve irr_delivered for duplicate edge interrupts ---
commit 691f4f74cce4e2c3436bc9f3cdf594c985c284e3
Author: Hao Zhang <zhanghao1@kylinos.cn>

KVM: x86: ioapic: Preserve irr_delivered for duplicate edge interrupts

For edge-triggered interrupts, ioapic_set_irq() clears irr_delivered before
checking whether the new edge is a duplicate. This patch moves the state
update after the duplicate-edge check to prevent duplicate edges from making
a delivered interrupt appear pending during VM migration.

> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 24a7cc3b8b7ef..676effd674f9e 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -230,11 +230,11 @@ static int ioapic_set_irq(struct kvm_ioapic *ioapic, unsigned int irq,

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing
ioapic_set_irq(), does KVM fail to latch edge-triggered interrupts if they
pulse while the pin is masked?

Earlier in ioapic_set_irq(), the IRR bit is cleared unconditionally when the
pin goes low:

	if (!irq_level) {
		ioapic->irr &= ~mask;
		ret = 1;
		goto out;
	}

This appears to destroy the latch state for edge-triggered interrupts that
were not yet delivered.

Furthermore, looking at ioapic_write_indirect(), when unmasking, pending
interrupt injection is restricted only to level-triggered interrupts:

		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG &&
		    ioapic->irr & (1 << index) && !e->fields.mask && !e->fields.remote_irr) {

Could this sequence cause latched edge-triggered interrupts to be silently
ignored and lost if a guest temporarily masks IOAPIC pins during interrupt
handling?

>  	old_irr = ioapic->irr;
>  	ioapic->irr |= mask;
>  	if (edge) {
> -		ioapic->irr_delivered &= ~mask;
>  		if (old_irr == ioapic->irr) {
>  			ret = 0;
>  			goto out;
>  		}
> +		ioapic->irr_delivered &= ~mask;
>  	}
>  
>  	ret = ioapic_service(ioapic, irq, line_status);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/anwugm-oU0v1VM-t@192.168.1.215?part=3

[PATCH v5 4/6] KVM: selftests: Verify duplicate edge preserves IOAPIC state

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

Extend ioapic_state_test to verify that a coalesced duplicate edge
interrupt does not make an already-delivered interrupt visible in the
IOAPIC IRR state returned by KVM_GET_IRQCHIP.

Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
 .../testing/selftests/kvm/x86/ioapic_state_test.c  | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86/ioapic_state_test.c b/tools/testing/selftests/kvm/x86/ioapic_state_test.c
index be5237f34ad2..51f6662d553c 100644
--- a/tools/testing/selftests/kvm/x86/ioapic_state_test.c
+++ b/tools/testing/selftests/kvm/x86/ioapic_state_test.c
@@ -3,7 +3,9 @@
  * Regression tests for in-kernel I/O APIC state.
  */
 
+#include "apic.h"
 #include "kvm_util.h"
+#include "processor.h"
 #include "test_util.h"
 
 #define TEST_IOAPIC_PIN		16
@@ -51,6 +53,19 @@ static void set_ioapic_entry(struct kvm_vm *vm, bool level_triggered,
 	set_ioapic(vm, &irqchip);
 }
 
+static void enable_lapic(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic_state lapic;
+	u64 apicbase;
+
+	apicbase = vcpu_get_msr(vcpu, MSR_IA32_APICBASE);
+	vcpu_set_msr(vcpu, MSR_IA32_APICBASE,
+		     apicbase | MSR_IA32_APICBASE_ENABLE);
+	vcpu_ioctl(vcpu, KVM_GET_LAPIC, &lapic);
+	*(u32 *)(lapic.regs + APIC_SPIV) |= APIC_SPIV_APIC_ENABLED;
+	vcpu_ioctl(vcpu, KVM_SET_LAPIC, &lapic);
+}
+
 static int kvm_irq_line_status(struct kvm_vm *vm, int level)
 {
 	struct kvm_irq_level irq = {
@@ -62,6 +77,16 @@ static int kvm_irq_line_status(struct kvm_vm *vm, int level)
 	return irq.status;
 }
 
+static void assert_ioapic_pin_irr(struct kvm_vm *vm, bool expected)
+{
+	struct kvm_irqchip irqchip;
+
+	get_ioapic(vm, &irqchip);
+	TEST_ASSERT(!!(irqchip.chip.ioapic.irr & (1 << TEST_IOAPIC_PIN)) == expected,
+		    "Expected IOAPIC IRR for pin %u to be %u, got 0x%x",
+		    TEST_IOAPIC_PIN, expected, irqchip.chip.ioapic.irr);
+}
+
 static void test_no_remote_irr_for_undelivered_level_interrupt(void)
 {
 	struct kvm_irqchip irqchip;
@@ -84,9 +109,37 @@ static void test_no_remote_irr_for_undelivered_level_interrupt(void)
 	kvm_vm_free(vm);
 }
 
+static void test_duplicate_edge_interrupt_preserves_delivery_state(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int status;
+
+	vm = vm_create_with_one_vcpu(&vcpu, NULL);
+	enable_lapic(vcpu);
+
+	set_ioapic_entry(vm, false, vcpu->id);
+
+	status = kvm_irq_line_status(vm, 1);
+	TEST_ASSERT(status > 0,
+		    "Expected edge interrupt delivery, got %d", status);
+
+	assert_ioapic_pin_irr(vm, false);
+
+	status = kvm_irq_line_status(vm, 1);
+	TEST_ASSERT(!status,
+		    "Expected duplicate edge interrupt to be coalesced, got %d",
+		    status);
+
+	assert_ioapic_pin_irr(vm, false);
+
+	kvm_vm_free(vm);
+}
+
 int main(void)
 {
 	test_no_remote_irr_for_undelivered_level_interrupt();
+	test_duplicate_edge_interrupt_preserves_delivery_state();
 
 	return 0;
 }
-- 
2.15.0

[PATCH v5 5/6] KVM: x86: ioapic: Clear irr_delivered on level-triggered RTE

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

irr_delivered hides delivered edge interrupts from KVM_GET_IRQCHIP.
The interrupt remains in IRR until the guest deasserts the line.

The guest can reprogram an IOAPIC RTE from edge to level via MMIO.
If an edge interrupt was delivered before the trigger mode change, a
stale irr_delivered bit can incorrectly mask a subsequent pending level
interrupt from KVM_GET_IRQCHIP.

That can cause userspace to migrate the VM without a pending level
interrupt that is still present in KVM's in-kernel IOAPIC.

Clear irr_delivered when the RTE is configured for level-triggered
interrupts.

Fixes: 5bda6eed2e36 ("KVM: ioapic: Record edge-triggered interrupts delivery status")
Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
 arch/x86/kvm/ioapic.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 676effd674f9..81db025d2ac9 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -377,8 +377,17 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 		 * as edge-triggered. This behavior is used to simulate an
 		 * explicit EOI on IOAPICs that don't have the EOI register.
 		 */
-		if (e->fields.trig_mode == IOAPIC_EDGE_TRIG)
+		if (e->fields.trig_mode == IOAPIC_EDGE_TRIG) {
 			e->fields.remote_irr = 0;
+		} else {
+			/*
+			 * irr_delivered tracks edge-triggered interrupts that
+			 * have already been delivered.  Clear stale edge-triggered
+			 * delivery state when the entry is configured for
+			 * level-triggered interrupts.
+			 */
+			ioapic->irr_delivered &= ~(1 << index);
+		}
 
 		mask_after = e->fields.mask;
 		if (mask_before != mask_after)
-- 
2.15.0

[PATCH v5 6/6] KVM: selftests: Verify edge-to-level clears IOAPIC state

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

Extend ioapic_state_test to verify that reprogramming an IOAPIC RTE from
edge-triggered to level-triggered via guest MMIO clears stale
edge-triggered delivery state.

Verify that KVM_GET_IRQCHIP still reports the pending IRR bit after the
pin is switched to level-triggered mode.

Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
 .../testing/selftests/kvm/x86/ioapic_state_test.c  | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86/ioapic_state_test.c b/tools/testing/selftests/kvm/x86/ioapic_state_test.c
index 51f6662d553c..f024f4f01571 100644
--- a/tools/testing/selftests/kvm/x86/ioapic_state_test.c
+++ b/tools/testing/selftests/kvm/x86/ioapic_state_test.c
@@ -7,12 +7,18 @@
 #include "kvm_util.h"
 #include "processor.h"
 #include "test_util.h"
+#include "ucall_common.h"
 
 #define TEST_IOAPIC_PIN		16
 #define TEST_VECTOR		0x50
 #define NO_SUCH_APIC_ID		0xfe
 #define TEST_IOAPIC_EDGE_TRIG	0
 #define TEST_IOAPIC_LEVEL_TRIG	1
+#define IOAPIC_DEFAULT_GPA	0xfec00000ULL
+#define IOAPIC_REG_SELECT	0x00
+#define IOAPIC_REG_WINDOW	0x10
+#define IOAPIC_RTE_LOW(pin)	(0x10 + 2 * (pin))
+#define IOAPIC_RTE_TRIG_MODE	BIT(15)
 
 static void get_ioapic(struct kvm_vm *vm, struct kvm_irqchip *irqchip)
 {
@@ -136,10 +142,49 @@ static void test_duplicate_edge_interrupt_preserves_delivery_state(void)
 	kvm_vm_free(vm);
 }
 
+static void guest_switch_ioapic_pin_to_level(void)
+{
+	u32 *ioapic = (u32 *)IOAPIC_DEFAULT_GPA;
+
+	WRITE_ONCE(ioapic[IOAPIC_REG_SELECT / sizeof(u32)],
+		   IOAPIC_RTE_LOW(TEST_IOAPIC_PIN));
+	WRITE_ONCE(ioapic[IOAPIC_REG_WINDOW / sizeof(u32)],
+		   TEST_VECTOR | IOAPIC_RTE_TRIG_MODE);
+
+	GUEST_DONE();
+}
+
+static void test_edge_to_level_clears_delivery_state(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int status;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_switch_ioapic_pin_to_level);
+	enable_lapic(vcpu);
+	virt_map(vm, IOAPIC_DEFAULT_GPA, IOAPIC_DEFAULT_GPA, 1);
+
+	set_ioapic_entry(vm, false, vcpu->id);
+
+	status = kvm_irq_line_status(vm, 1);
+	TEST_ASSERT(status > 0,
+		    "Expected edge interrupt delivery, got %d", status);
+
+	assert_ioapic_pin_irr(vm, false);
+
+	vcpu_run(vcpu);
+	TEST_ASSERT_EQ(get_ucall(vcpu, NULL), UCALL_DONE);
+
+	assert_ioapic_pin_irr(vm, true);
+
+	kvm_vm_free(vm);
+}
+
 int main(void)
 {
 	test_no_remote_irr_for_undelivered_level_interrupt();
 	test_duplicate_edge_interrupt_preserves_delivery_state();
+	test_edge_to_level_clears_delivery_state();
 
 	return 0;
 }
-- 
2.15.0