[PATCH v3 1/3] KVM: x86: ioapic: Update remote_irr only after successful delivery

[PATCH v3 1/3] 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
kvm_irq_delivery_to_apic() returns -1 when no destination is found.
Setting remote_irr for that failed delivery leaves the pin blocked
forever waiting for an EOI that will never be generated.

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.")
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;

base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86
-- 
2.15.0

[PATCH v3 2/3] 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 v3 3/3] 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.

Add a selftest to verify that a duplicate edge interrupt does not make
an already-delivered interrupt visible in the saved IRR state.

Fixes: 5bda6eed2e36 ("KVM: ioapic: Record edge-triggered interrupts delivery status")
Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
 arch/x86/kvm/ioapic.c                              |  2 +-
 .../testing/selftests/kvm/x86/ioapic_state_test.c  | 47 ++++++++++++++++++++++
 2 files changed, 48 insertions(+), 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);
diff --git a/tools/testing/selftests/kvm/x86/ioapic_state_test.c b/tools/testing/selftests/kvm/x86/ioapic_state_test.c
index be5237f34ad2..da0276a1542f 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
@@ -62,6 +64,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 +96,44 @@ 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_lapic_state lapic;
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	u64 apicbase;
+	int status;
+
+	vm = vm_create_with_one_vcpu(&vcpu, NULL);
+	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);
+
+	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

Re: [PATCH v3 3/3] 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] This problem wasn't introduced by this patch, but changing an IOAPIC pin's trigger mode from edge-triggered to level-triggered via MMIO does not clear `irr_delivered`, causing subsequent level-triggered interrupts to be masked and lost across live migration.
--

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

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

This commit moves the state update of irr_delivered after the duplicate edge
check in ioapic_set_irq() to prevent duplicate edge interrupts from leaving
the interrupt visible in the KVM_GET_IRQCHIP state. It also adds a selftest
to verify this behavior.

> 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,
>  	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;
>  	}

[Severity: High]
This is a pre-existing issue, but does changing an IOAPIC pin's trigger mode
from edge-triggered to level-triggered via MMIO leave a stale irr_delivered
bit?

If a guest configures a pin as edge-triggered, an interrupt sets the bit in
ioapic->irr_delivered. If the guest then reconfigures the pin to
level-triggered in ioapic_write_indirect(), the stale irr_delivered bit is
not cleared.

When a subsequent level interrupt is asserted, ioapic_set_irq() sets the irr
bit but skips clearing irr_delivered because the pin is now level-triggered
and edge is false.

During VM migration, kvm_get_ioapic() extracts the state:

	state->irr &= ~ioapic->irr_delivered;

Could this stale irr_delivered bit incorrectly mask out the pending level
interrupt and permanently lose it across live migration?

[ ... ]

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