Re: [PATCH v2 1/3] KVM: arm64: vgic-v2: Don't drop pending writes to active level IRQs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: arm64: vgic-v2: GICH_LR_HW pending LR programmed with inactive phys IRQ
- [Medium] KVM: arm64: vgic-v2: spurious level IRQ after save/restore in vgic_v2_fold_lr()
--

commit 48710a44e389b45084a926f40b58ded3edf3a4dd
Author: Karl Mehltretter <kmehltretter@gmail.com>

KVM: arm64: vgic-v2: Don't drop pending writes to active level IRQs

This commit synchronizes the target vCPU's LRs in vgic_mmio_write_spending()
and preserves irq->pending_latch in vgic_v2_fold_lr() when a level-triggered
interrupt was already active, so that a pending state written via GICD_ISPENDR
while the interrupt is active is not dropped on deactivation.

> diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> index 7182f63fc9382..f3ba3177dd97b 100644
> --- a/arch/arm64/kvm/vgic/vgic-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-v2.c
> @@ -57,7 +57,7 @@ static void vgic_v2_fold_lr(struct kvm_vcpu *vcpu, u32 val)
>  {
>  	u32 cpuid, intid = val & GICH_LR_VIRTUALID;
>  	struct vgic_irq *irq;
> -	bool deactivated;
> +	bool deactivated, was_active;
>  
>  	/* Extract the source vCPU id from the LR */
>  	cpuid = FIELD_GET(GICH_LR_PHYSID_CPUID, val) & 7;
> @@ -71,7 +71,8 @@ static void vgic_v2_fold_lr(struct kvm_vcpu *vcpu, u32 val)
>  
>  	scoped_guard(raw_spinlock, &irq->irq_lock) {
>  		/* Always preserve the active bit, note deactivation */
> -		deactivated = irq->active && !(val & GICH_LR_ACTIVE_BIT);
> +		was_active = irq->active;
> +		deactivated = was_active && !(val & GICH_LR_ACTIVE_BIT);
>  		irq->active = !!(val & GICH_LR_ACTIVE_BIT);
>  
>  		if (irq->active && vgic_irq_is_sgi(intid))
> @@ -88,8 +89,11 @@ static void vgic_v2_fold_lr(struct kvm_vcpu *vcpu, u32 val)
>  
>  		/*
>  		 * Clear soft pending state when level irqs have been acked.
> +		 * A latch written while the irq was active is a new pending
> +		 * state and was not represented in the LR, so preserve it.
>  		 */
> -		if (irq->config == VGIC_CONFIG_LEVEL && !(val & GICH_LR_STATE))
> +		if (irq->config == VGIC_CONFIG_LEVEL &&
> +		    !(val & GICH_LR_PENDING_BIT) && !was_active)
>  			irq->pending_latch = false;

[Severity: High]
Can preserving irq->pending_latch here in vgic_v2_fold_lr() in
arch/arm64/kvm/vgic/vgic-v2.c cause a GICH_LR_HW list register to be
populated as pending while the physical interrupt is inactive in the host
distributor?

If the guest writes to GICD_ISPENDR0 for a mapped level-triggered interrupt
such as the architected timer PPI (irq->hw is true, irq->config is
VGIC_CONFIG_LEVEL, and !vgic_irq_needs_resampling(irq)) while that interrupt
is already active, vgic_mmio_write_spending() -> __set_pending() in
arch/arm64/kvm/vgic/vgic-mmio.c sets irq->pending_latch and marks the
physical interrupt active while the first instance is already active in
hardware:

arch/arm64/kvm/vgic/vgic-mmio.c:__set_pending() {
    ...
		irq->pending_latch = true;
		if (irq->hw && !is_user)
			vgic_irq_set_phys_active(irq, true);
    ...
}

When the guest deactivates that first in-flight instance, the hardware
deactivation clears the physical interrupt's active state in the host GIC
distributor. On the subsequent exit, vgic_v2_fold_lr() sees was_active == true
and deactivated == true, sets irq->active = false, and preserves
irq->pending_latch = true without re-activating the physical interrupt.

On the next guest entry, vgic_v2_populate_lr() -> vgic_v2_compute_lr() in
arch/arm64/kvm/vgic/vgic-v2.c sees irq->active == false and
irq_is_pending(irq) == true via irq->pending_latch:

arch/arm64/kvm/vgic/vgic-v2.c:vgic_v2_compute_lr() {
    ...
	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
		val |= GICH_LR_HW;
		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
		/*
		 * Never set pending+active on a HW interrupt, as the
		 * pending state is kept at the physical distributor
		 * level.
		 */
		if (irq->active)
			allow_pending = false;
	}
    ...
}

Wouldn't this program the list register with GICH_LR_HW | GICH_LR_PENDING_BIT
while the corresponding physical interrupt in the host distributor is
inactive?

[Severity: Medium]
Also, what happens across a GICv2 VM save and restore when a level-triggered
device interrupt is active and its input line is still asserted
(irq->active == true, irq->line_level == true, and irq->pending_latch ==
false)?

When userspace saves KVM_DEV_TYPE_ARM_VGIC_V2 distributor state via
vgic_uaccess_read_pending() -> __read_pending() in
arch/arm64/kvm/vgic/vgic-mmio.c, GICv2 reads irq_is_pending(irq):

arch/arm64/kvm/vgic/vgic-mmio.c:__read_pending() {
    ...
			switch (vcpu->kvm->arch.vgic.vgic_model) {
			case KVM_DEV_TYPE_ARM_VGIC_V3:
				if (is_user) {
					val = irq->pending_latch;
					break;
				}
				fallthrough;
			default:
				val = irq_is_pending(irq);
				break;
			}
    ...
}

Because irq->line_level is true, the saved pending state is 1. On restore,
userspace restores the active state via vgic_mmio_uaccess_write_sactive()
(setting irq->active = true) and restores the pending state via
vgic_uaccess_write_spending() -> __set_pending() (setting irq->pending_latch =
true).

When the guest resumes, clears the interrupt condition at the device (setting
irq->line_level = false), and writes GICC_EOIR to deactivate the interrupt,
vgic_v2_fold_lr() runs with was_active == true and !(val & GICH_LR_STATE).
With the new !was_active check in vgic_v2_fold_lr(), won't irq->pending_latch
remain true on deactivation and cause a spurious second delivery of the level
interrupt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028050113.10046-1-kmehltretter@gmail.com?part=1