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

[Sascha Bischoff]

On Tue, 2026-03-31 at 18:02 +0100, Marc Zyngier wrote:
> On Tue, 31 Mar 2026 16:44:04 +0100,
> Sascha Bischoff <Sascha.Bischoff at 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 at 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);
>  

I've just tested the above and can confirm that it does work as
expected. It indeed makes a lot more sense than what I'd suggested.

Thanks,
Sascha

> 
> Thanks,
> 
> 	M.
>