Re: [PATCH 1/2] KVM: x86: ioapic: Update state only after successful delivery

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

Pre-existing issues:
- [High] Clearing `irr_delivered` for duplicate edge interrupts causes spurious injections upon VM migration.
--

--- Patch [1]: [PATCH 1/2] KVM: x86: ioapic: Update state only after successful delivery ---
commit 709ff05e7e6d1df68c0523d442968791b55f37d1
Author: Hao Zhang <[email protected]>

KVM: x86: ioapic: Update state only after successful delivery

This commit defers updating I/O APIC delivery state until interrupt
delivery actually succeeds. It ensures level-triggered pins aren't blocked
forever on a failed delivery and edge-triggered interrupts aren't incorrectly
dropped from the saved IRR state.

> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 757667fb2bfa0..540e5665fbe4f 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c

[ ... ]

> @@ -491,8 +488,12 @@ 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)
> -		entry->fields.remote_irr = 1;
> +	if (ret > 0) {
> +		if (irqe.trig_mode == IOAPIC_EDGE_TRIG)
> +			ioapic->irr_delivered |= 1 << irq;
> +		else if (irqe.trig_mode == IOAPIC_LEVEL_TRIG)
> +			entry->fields.remote_irr = 1;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but while looking at irr_delivered
tracking, does ioapic_set_irq() have an issue with duplicate edge interrupts?

If a device emulator calls KVM_IRQ_LINE multiple times for an edge-triggered
interrupt without an intervening level=0, the first call delivers the
interrupt and sets irr_delivered here in ioapic_service().

However, on subsequent calls, ioapic_set_irq() does this:

arch/x86/kvm/ioapic.c:ioapic_set_irq() {
	...
	old_irr = ioapic->irr;
	ioapic->irr |= mask;
	if (edge) {
		ioapic->irr_delivered &= ~mask;
		if (old_irr == ioapic->irr) {
			ret = 0;
			goto out;
		}
	}
	...
}

It unsets irr_delivered but then detects the duplicate edge and returns early.

Could this leave irr_delivered incorrectly cleared?

If a VM migration occurs after this, KVM_GET_IRQCHIP might read the interrupt
as pending instead of delivered.

Would this cause a spurious injection on the destination?

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1