Re: [RFC PATCH 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: unbacked GICH_LR_HW pending state after active ISPENDR
- [Medium] KVM: arm64: vgic-v2: preserved pending_latch on restore of active level IRQ
--

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

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

This commit preserves pending_latch in vgic_v2_fold_lr() when a
level-triggered interrupt was already active or had GICD_ISPENDR written while
present in a list register. It adds a latch_written flag to struct vgic_irq to
track pending writes that occur while irq->on_lr is set and clears it when
clearing pending state or folding list registers.

[ ... ]

> diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> index 7182f63fc9382..37474ea2431c5 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,9 +89,15 @@ 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. 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 & GICH_LR_STATE))
> +		if (irq->config == VGIC_CONFIG_LEVEL &&
> +		    !(val & GICH_LR_PENDING_BIT) &&
> +		    !was_active && !irq->latch_written)
>  			irq->pending_latch = false;

[Severity: Medium]
Could checking !was_active here cause a spurious second interrupt delivery
after saving and restoring a KVM_DEV_TYPE_ARM_VGIC_V2 guest with an active
level-triggered interrupt?

Unlike KVM_DEV_TYPE_ARM_VGIC_V3, which exposes KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
and returns only irq->pending_latch to userspace in
arch/arm64/kvm/vgic/vgic-mmio.c:vgic_uaccess_read_pending()->__read_pending(),
KVM_DEV_TYPE_ARM_VGIC_V2 returns irq_is_pending(irq):

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

When a GICv2 guest is saved while a level-triggered interrupt is active
(irq->active == true) and its device line is still asserted
(irq->line_level == true, irq->pending_latch == false), userspace reads 1 from
both GIC_DIST_ACTIVE_SET and GIC_DIST_PENDING_SET.

On restore, userspace writes 1 to GIC_DIST_ACTIVE_SET via
vgic_mmio_uaccess_write_sactive(), setting irq->active = true, and writes 1 to
GIC_DIST_PENDING_SET via vgic_uaccess_write_spending()->__set_pending(),
setting irq->pending_latch = true:

    int vgic_uaccess_write_spending(struct kvm_vcpu *vcpu,
                                    gpa_t addr, unsigned int len,
                                    unsigned long val)
    {
        __set_pending(vcpu, addr, len, val, true);
        return 0;
    }

Previously, once the guest cleared the device condition (lowering
irq->line_level) and deactivated the interrupt, vgic_v2_fold_lr() saw
!(val & GICH_LR_STATE) and cleared irq->pending_latch = false. With the
!was_active check in vgic_v2_fold_lr(), won't irq->pending_latch remain set
across the deactivation of the restored active interrupt and cause the
interrupt to be injected a second time?

> +		irq->latch_written = false;
>  
>  		/* Handle resampling for mapped interrupts if required */
>  		vgic_irq_handle_resampling(irq, deactivated, val & GICH_LR_PENDING_BIT);

[Severity: High]
What happens for a mapped level-triggered interrupt (irq->hw == true,
irq->config == VGIC_CONFIG_LEVEL, and !vgic_irq_needs_resampling(irq), such as
the architected timer interrupt) when the guest writes GICD_ISPENDR0 while the
interrupt is active?

In arch/arm64/kvm/vgic/vgic-mmio.c:vgic_mmio_write_spending()->__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 for
the first in-flight delivery, vgic_irq_set_phys_active(irq, true) coalesces
with the existing physical active state. When the guest then deactivates the
first delivery (via GICC_EOIR / GICC_DIR in hardware, or via the EOIcount path
in vgic_v2_fold_lr_state()->vgic_v2_deactivate() writing GIC_CPU_DEACTIVATE),
the physical interrupt is deactivated in the host distributor.

When vgic_v2_fold_lr() runs on exit, was_active or irq->latch_written is true,
so irq->pending_latch remains true while irq->active becomes false. However,
neither vgic_v2_fold_lr() nor
arch/arm64/kvm/vgic/vgic.c:vgic_irq_handle_resampling() re-activates the
physical interrupt in the host distributor (and if lr_deactivated &&
irq->line_level holds with the physical line low, vgic_irq_handle_resampling()
explicitly clears the physical active state):

    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,
arch/arm64/kvm/vgic/vgic-v2.c:vgic_v2_populate_lr()->vgic_v2_compute_lr() sees
irq->active == false and irq_is_pending(irq) == true from irq->pending_latch:

    if (irq->hw && !vgic_irq_needs_resampling(irq)) {
        val |= GICH_LR_HW;
        val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
        if (irq->active)
            allow_pending = false;
    }

    if (allow_pending && irq_is_pending(irq)) {
        val |= GICH_LR_PENDING_BIT;

Can this populate the list register with GICH_LR_HW | GICH_LR_PENDING_BIT
while the corresponding physical interrupt is inactive in the host
distributor, violating the hardware-bit consistency requirement and triggering
a second physical deactivation when the guest completes the second delivery?

>  
>  		irq->on_lr = false;
>  	}

[ ... ]

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