Re: [RFC PATCH 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() preserves latch on deactivated HW IRQ
--

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

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

This commit updates vgic_v3_fold_lr() so that a soft-pending latch written
while a level-triggered interrupt is active or present in a list register is
preserved across deactivation.

> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 726e20a1da6e7..8009dc6df00db 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,9 +104,15 @@ 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. A write while the irq was in the LR may also have
> +		 * followed the ack, so preserve it too.
>  		 */
> -		if (irq->config == VGIC_CONFIG_LEVEL && !(val & ICH_LR_STATE))
> +		if (irq->config == VGIC_CONFIG_LEVEL &&
> +		    !(val & ICH_LR_PENDING_BIT) &&
> +		    !was_active && !irq->latch_written)
>  			irq->pending_latch = false;
> +		irq->latch_written = false;

[Severity: High]
What happens here when a mapped level-triggered interrupt (irq->hw is true,
irq->config == VGIC_CONFIG_LEVEL, and !vgic_irq_needs_resampling(irq), such
as the architected timer) has its pending latch set via
vgic_mmio_write_spending() while the interrupt is already active or in a list
register?

In arch/arm64/kvm/vgic/vgic-mmio.c:__set_pending(), writing the set-pending
register sets irq->pending_latch = true (and irq->latch_written = true if
irq->on_lr) and calls vgic_irq_set_phys_active(irq, true):

arch/arm64/kvm/vgic/vgic-mmio.c:__set_pending() {
    ...
		if (irq->on_lr)
			irq->latch_written = true;

		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 distributor or
redistributor for the in-flight virtual interrupt, there is only a single
physical active state bit. When the guest then deactivates the virtual
interrupt, hardware deactivation (or vgic_v3_deactivate_phys() when
deactivated outside the list registers) clears the physical active state in
the host.

When vgic_v3_fold_lr() runs on exit, was_active or irq->latch_written is true,
so irq->pending_latch remains true while irq->active becomes false. Neither
vgic_v3_fold_lr() nor vgic_irq_handle_resampling() re-activates the physical
interrupt, and if irq->line_level was true and the physical line is now low,
vgic_irq_handle_resampling() explicitly clears the physical active state:

arch/arm64/kvm/vgic/vgic.c:vgic_irq_handle_resampling() {
	if (vgic_irq_is_mapped_level(irq)) {
		bool resample = false;

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

On the next guest entry, irq->active is false and irq_is_pending(irq) is true
via the preserved irq->pending_latch, so vgic_v3_compute_lr() populates the
list register with ICH_LR_HW | ICH_LR_PENDING_BIT while the physical interrupt
is inactive in the host:

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;
	}
    ...
	if (allow_pending && irq_is_pending(irq)) {
		val |= ICH_LR_PENDING_BIT;
    ...
}

Doesn't placing a list register entry with ICH_LR_HW set in the pending state
while the corresponding physical interrupt is not active in the distributor
or redistributor violate the hardware-bit consistency requirement (resulting
in unpredictable cpu interface behavior and a duplicate physical deactivation
of an inactive physical interrupt)?

[ ... ]

>
> 
>  		if (is_v2_sgi) {
>  			u8 cpuid = FIELD_GET(GICH_LR_PHYSID_CPUID, val);

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