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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: arm64: vgic-v3: vgic_v3_fold_lr() leaves HW IRQ inactive on repend
--

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

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

This commit preserves irq->pending_latch in vgic_v3_fold_lr() when a
level-triggered interrupt was already active, so that a pending state set via
GICD_ISPENDR or GICR_ISPENDR0 while the interrupt is active is not cleared
upon deactivation.

> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 726e20a1da6e7..4d802fc9c6dd5 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -75,7 +75,7 @@ static void vgic_v3_fold_lr(struct kvm_vcpu *vcpu, u64 val)
>  {
>  	struct vgic_irq *irq;
>  	bool is_v2_sgi = false;
> -	bool deactivated;
> +	bool deactivated, was_active;
>  	u32 intid;
>  
>  	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> @@ -93,7 +93,8 @@ static void vgic_v3_fold_lr(struct kvm_vcpu *vcpu, u64 val)
>  		/* Always preserve the active bit for !LPIs, note deactivation */
>  		if (irq->intid >= VGIC_MIN_LPI)
>  			val &= ~ICH_LR_ACTIVE_BIT;
> -		deactivated = irq->active && !(val & ICH_LR_ACTIVE_BIT);
> +		was_active = irq->active;
> +		deactivated = was_active && !(val & ICH_LR_ACTIVE_BIT);
>  		irq->active = !!(val & ICH_LR_ACTIVE_BIT);
>  
>  		/* Edge is the only case where we preserve the pending bit */
> @@ -103,8 +104,11 @@ static void vgic_v3_fold_lr(struct kvm_vcpu *vcpu, u64 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 & ICH_LR_STATE))
> +		if (irq->config == VGIC_CONFIG_LEVEL &&
> +		    !(val & ICH_LR_PENDING_BIT) && !was_active)
>  			irq->pending_latch = false;

[Severity: High]
What happens here for a mapped level-triggered interrupt where irq->hw is
true and !vgic_irq_needs_resampling(irq), such as the architected timer PPI?

When that interrupt is already active in the guest (irq->active is true and
the physical interrupt is active in the host GIC), a guest write to
GICR_ISPENDR0 or GICD_ISPENDR calls:

vgic_mmio_write_spending() -> __set_pending() {
	irq->pending_latch = true;
	if (irq->hw && !is_user)
		vgic_irq_set_phys_active(irq, true);
}

Because the physical interrupt is already active in the host GIC for the
in-flight activation, vgic_irq_set_phys_active(irq, true) does not add a
second physical active state.

When the guest then deactivates the interrupt via ICC_EOIR1_EL1 or
ICC_DIR_EL1 (or via vgic_v3_deactivate_phys()), hardware deactivation clears
the physical active state in the host GIC. On the next VM exit,
vgic_v3_fold_lr() sees was_active is true and !(val & ICH_LR_ACTIVE_BIT),
sets irq->active = false, and preserves irq->pending_latch = true.

Then vgic_irq_handle_resampling() runs:

arch/arm64/kvm/vgic/vgic.c:vgic_irq_handle_resampling() {
	if (unlikely(vgic_irq_needs_resampling(irq))) {
		resample = !(irq->active || irq->pending_latch);
	} else if (lr_pending || (lr_deactivated && irq->line_level)) {
		irq->line_level = vgic_get_phys_line_level(irq);
		resample = !irq->line_level;
	}

	if (resample)
		vgic_irq_set_phys_active(irq, false);
}

This does not re-establish the physical active state in the host GIC when
irq->pending_latch remains true (and if irq->line_level was true while the
timer line is now low, it explicitly clears the physical active state).

On the subsequent guest entry, vgic_v3_populate_lr() -> vgic_v3_compute_lr()
sees irq->active is false and irq_is_pending(irq) is true via
irq->pending_latch:

arch/arm64/kvm/vgic/vgic-v3.c:vgic_v3_compute_lr() {
	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
		val |= ICH_LR_HW;
		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
		if (irq->active)
			allow_pending = false;
	}
}

Wouldn't this populate the list register with ICH_LR_HW | ICH_LR_PENDING_BIT
while the corresponding physical interrupt is inactive in the host GIC,
violating the GICv3 list register hardware-bit consistency requirement and
causing the subsequent guest deactivation to deactivate an inactive physical
interrupt?

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