Re: [PATCH v4 16/48] KVM: arm64: gic-v5: Request doorbells when VPEs enter WFI

On Fri, 2026-07-24 at 11:41 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] Stale `db_fired` state causes an infinite loop bypassing WFI
> sleep, pegging the host CPU to 100%.
> - [High] Incorrect GICv5 doorbell priority mask (DBPM) calculation
> drops preempting interrupts and vNMIs.
> --
> 
> --- Patch [16]: [PATCH v4 16/48] KVM: arm64: gic-v5: Request
> doorbells when VPEs enter WFI ---
> commit 4c69e1e71f915cfc6ba78559fe449983dee20dd6
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Request doorbells when VPEs enter WFI
> 
> This commit requests a GICv5 VPE doorbell when a vcpu enters WFI,
> allowing 
> KVM to be notified when an SPI or LPI becomes pending. It also
> calculates 
> a doorbell priority mask (DBPM) from the virtual priority state to
> avoid 
> waking the guest for masked interrupts.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> [ ... ]
> > +		/*
> > +		 * Request a doorbell *unless* the priority is 0,
> > indicating
> > +		 * that no interrupt can wake the CPU up.
> > +		 */
> > +		if (priority_mask) {
> 
> [Severity: High]
> Does skipping the doorbell request when the priority is 0 break vNMI
> wakeups?
> 
> Architectural semantics dictate that vNMIs have superpriority and
> must wake
> a vCPU even when the running priority is 0.

KVM does not expose FEAT_NMI to a guest, so this is not an issue.

> 
> > +			int db_irq = vgic_v5_vpe_db(vcpu);
> > +			struct irq_data *d =
> > irq_get_irq_data(db_irq);
> > +			const struct cpumask *aff =
> > irq_data_get_effective_affinity_mask(d);
> > +			int cpu = smp_processor_id();
> > +
> > +			dbpm = priority_mask - 1;
> 
> [Severity: High]
> Does subtracting 1 from the priority mask drop valid preempting
> interrupts?

No. We discussed this last time. This is correct for GICv5, but
wouldn't be for GICv3.

> 
> The hardware doorbell condition triggers when the pending priority is
> less
> than the threshold. If the highest active priority is 5, the vCPU
> should
> be woken by any interrupt with a priority less than 5, for example,
> 4.
> 
> If the priority mask is 5, this code sets the DBPM to 4. The hardware
> condition then becomes priority < 4. An interrupt with priority 4
> will
> not signal the doorbell, despite having a higher priority than the
> active
> interrupt, causing missed wakeups and guest hangs.
> 
> > +			cpu_if->vgic_contextr =
> > FIELD_PREP(ICH_CONTEXTR_EL2_DB, 1) |
> > +						FIELD_PREP(ICH_CON
> > TEXTR_EL2_DBPM, dbpm);
> [ ... ]
> > diff --git a/arch/arm64/kvm/vgic/vgic.c
> > b/arch/arm64/kvm/vgic/vgic.c
> > --- a/arch/arm64/kvm/vgic/vgic.c
> > +++ b/arch/arm64/kvm/vgic/vgic.c
> [ ... ]
> > @@ -1227,8 +1227,12 @@ int kvm_vgic_vcpu_pending_irq(struct
> > kvm_vcpu *vcpu)
> >  	unsigned long flags;
> >  	struct vgic_vmcr vmcr;
> >  
> > -	if (vgic_is_v5(vcpu->kvm))
> > +	if (vgic_is_v5(vcpu->kvm)) {
> > +		if (READ_ONCE(vcpu-
> > >arch.vgic_cpu.vgic_v5.gicv5_vpe.db_fired))
> > +			return true;
> 
> [Severity: High]
> Can a stale db_fired state cause an infinite loop bypassing WFI
> sleep?
> 
> The db_fired flag is cleared only in __vgic_v5_make_non_resident(),
> which
> executes during schedule-out.
> 
> If a doorbell fires, db_fired becomes true and the vCPU wakes up to
> enter
> the guest. When the vCPU subsequently re-enters WFI without a
> schedule-out,
> kvm_vcpu_block() evaluates kvm_vgic_vcpu_pending_irq(). Because
> db_fired
> remains true, this instantly returns without sleeping.
> 
> This leaves the vCPU in an infinite loop returning from WFI and
> pegging
> the host CPU at 100% until a host timer tick forces preemption.

Entering WFI (when it is trapped to KVM) results in a vgic_v5_put()
which makes the VPE non-resident. At this point, it would be cleared.

In the event that we sleep in the guest, then we indeed won't call
vgic_v5_put() until we schedule out - but that is by design. We've
already chosen to sleep in the guest context at this point, but will
return to the host on a scheduler tick, at which point we take whatever
action is appropriate, such as making the VPE non-resident.

> 
> > +
> >  		return vgic_v5_has_pending_ppi(vcpu);
> > +	}
> >  
> >  	if (!vcpu->kvm->arch.vgic.enabled)
> >  		return false;
> 

Thanks,
Sascha