Re: [PATCH 12/15] KVM: arm64: Remove evaluation of timer state in kvm_cpu_has_pending_timer()

[Marc Zyngier <maz@kernel.org>]

On Tue, 31 Mar 2026 16:44:04 +0100,
Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> 
> On Thu, 2026-03-26 at 15:35 +0000, Marc Zyngier wrote:
> > The vgic-v5 code added some evaluations of the timers in a helper
> > funtion
> > (kvm_cpu_has_pending_timer()) that is called to determine whether
> > the vcpu can wake-up.
> > 
> > But looking at the timer there is wrong:
> > 
> > - we want to see timers that are signalling an interrupt to the
> >   vcpu, and not just that have a pending interrupt
> > 
> > - we already have kvm_arch_vcpu_runnable() that evaluates the
> >   state of interrupts
> > 
> > - kvm_cpu_has_pending_timer() really is about WFIT, as the timeout
> >   does not generate an interrupt, and is therefore distinct from
> >   the point above
> > 
> > As a consequence, revert these changes.
> > 
> > Fixes: 9491c63b6cd7b ("KVM: arm64: gic-v5: Enlighten arch timer for
> > GICv5")
> > Link:
> > https://sashiko.dev/#/patchset/20260319154937.3619520-1-sascha.bischoff%40arm.com
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/arch_timer.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c
> > b/arch/arm64/kvm/arch_timer.c
> > index 37279f8748695..6608c47d1f628 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -402,11 +402,7 @@ static bool kvm_timer_should_fire(struct
> > arch_timer_context *timer_ctx)
> >  
> >  int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
> >  {
> > -	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> > -	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> > -
> > -	return kvm_timer_should_fire(vtimer) ||
> > kvm_timer_should_fire(ptimer) ||
> > -	       (vcpu_has_wfit_active(vcpu) && wfit_delay_ns(vcpu) ==
> > 0);
> > +	return vcpu_has_wfit_active(vcpu) && wfit_delay_ns(vcpu) ==
> > 0;
> >  }
> >  
> >  /*
> 
> Hi Marc,
> 
> It appears that I'd misunderstood the intent of this function when I
> originally wrote this bit code. That is: I agree that these checks
> shouldn't be here.
> 
> However, said checks are needed somewhere. With GICv5, we directly
> inject the timer state (when possible, at least) which means that we
> never see the timer interrupt firing on the host, and don't track if it
> is pending or not in struct vgic_irq as the pending state is driven by
> the hardware itself. The result of this is that we explicitly need to
> check if the timer interrupt would be pending if the guest were running
> somewhere.
> 
> I've run with this complete series and have tested the following
> change. It is sufficient to catch this case, and does it as part of
> checking if there are pending interrupts, i.e., a more appropriate
> place called via kvm_arch_vcpu_runnable(). It is yet-another GICv5
> special case, however. I'd love to hear your thoughts.
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index cbea4d9ee9552..f8b95721857c3 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -400,6 +400,14 @@ static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
>         return cval <= now;
>  }
>  
> +int kvm_cpu_timer_should_fire(struct kvm_vcpu *vcpu)
> +{
> +       struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> +       struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> +
> +       return kvm_timer_should_fire(vtimer) || kvm_timer_should_fire(ptimer);
> +}
> +
>  int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
>  {
>         return vcpu_has_wfit_active(vcpu) && wfit_delay_ns(vcpu) == 0;
> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> index 7680ced92f715..ffb91f535efe8 100644
> --- a/arch/arm64/kvm/vgic/vgic.c
> +++ b/arch/arm64/kvm/vgic/vgic.c
> @@ -1238,6 +1238,9 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
>                 if (READ_ONCE(vcpu->arch.vgic_cpu.vgic_v5.gicv5_vpe.db_fired))
>                         return true;
>  
> +               if (kvm_cpu_timer_should_fire(vcpu))
> +                       return true;
> +

This unfortunately seems to suffer from the exact same problem: you
are evaluating the output of the timer independently of the enable
bits gating the timer interrupt at the GIC level.

With this, you can disable the timers at the GIC level, arm the timers
so that they are in a firing position, and enter WFI: the vcpu will
exit WFI immediately, which is not the expected result.

I'd suggest something like this instead (compile tested only):

diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
index 75372bbfb6a6a..e7d23d0519e8b 100644
--- a/arch/arm64/kvm/vgic/vgic-v5.c
+++ b/arch/arm64/kvm/vgic/vgic-v5.c
@@ -365,9 +365,13 @@ bool vgic_v5_has_pending_ppi(struct kvm_vcpu *vcpu)
 
 		irq = vgic_get_vcpu_irq(vcpu, intid);
 
-		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
-			has_pending = (irq->enabled && irq_is_pending(irq) &&
+		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
+			bool pending;
+
+			pending = irq->hw ? vgic_get_phys_line_level(irq) : irq_is_pending(irq);
+			has_pending = (irq->enabled && pending &&
 				       irq->priority < priority_mask);
+		}
 
 		vgic_put_irq(vcpu->kvm, irq);
 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.