[PATCH v2 26/36] KVM: arm64: gic-v5: Bump arch timer for GICv5
Sascha Bischoff
Sascha.Bischoff at arm.com
Fri Jan 9 08:56:31 PST 2026
On Wed, 2026-01-07 at 16:08 +0000, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 15:52:45 +0000
> Sascha Bischoff <Sascha.Bischoff at arm.com> wrote:
>
> > Now that GICv5 has arrived, the arch timer requires some TLC to
> > address some of the key differences introduced with GICv5.
> >
> > For PPIs on GICv5, the set_pending_state and queue_irq_unlock
> > irq_ops
> > are used as AP lists are not required at all for GICv5. The arch
> > timer
> > also introduces an irq_op - get_input_level. Extend the
> > arch-timer-provided irq_ops to include the two PPI ops for vgic_v5
> > guests.
> >
> > When possible, DVI (Direct Virtual Interrupt) is set for PPIs when
> > using a vgic_v5, which directly inject the pending state in to the
>
> into ?
>
> > guest. This means that the host never sees the interrupt for the
> > guest
> > for these interrupts. This has two impacts.
> >
> > * First of all, the kvm_cpu_has_pending_timer check is updated to
> > explicitly check if the timers are expected to fire.
> >
> > * Secondly, for mapped timers (which use DVI) they must be masked
> > on
> > the host prior to entering a GICv5 guest, and unmasked on the
> > return
> > path. This is handled in set_timer_irq_phys_masked.
> >
> > The final, but rather important, change is that the architected
> > PPIs
> > for the timers are made mandatory for a GICv5 guest. Attempts to
> > set
> > them to anything else are actively rejected. Once a vgic_v5 is
> > initialised, the arch timer PPIs are also explicitly reinitialised
> > to
> > ensure the correct GICv5-compatible PPIs are used - this also adds
> > in
> > the GICv5 PPI type to the intid.
> >
> > Signed-off-by: Sascha Bischoff <sascha.bischoff at arm.com>
> Various comments inline.
>
> J
> > ---
> > arch/arm64/kvm/arch_timer.c | 110 ++++++++++++++++++++++++++--
> > ----
> > arch/arm64/kvm/vgic/vgic-init.c | 9 +++
> > arch/arm64/kvm/vgic/vgic-v5.c | 8 +--
> > include/kvm/arm_arch_timer.h | 7 +-
> > include/kvm/arm_vgic.h | 4 ++
> > 5 files changed, 115 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/arch_timer.c
> > b/arch/arm64/kvm/arch_timer.c
> > index 6f033f6644219..78d66a67b34ac 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
>
>
> > void kvm_timer_sync_nested(struct kvm_vcpu *vcpu)
> > @@ -1034,12 +1079,15 @@ void kvm_timer_vcpu_reset(struct kvm_vcpu
> > *vcpu)
> > if (timer->enabled) {
> > for (int i = 0; i < nr_timers(vcpu); i++)
> > kvm_timer_update_irq(vcpu, false,
> > - vcpu_get_timer(vcpu,
> > i));
> > + vcpu_get_timer(vcpu, i));
>
> Unrelated change, and a bad one at that!
>
>
> >
> > if (irqchip_in_kernel(vcpu->kvm)) {
> > - kvm_vgic_reset_mapped_irq(vcpu,
> > timer_irq(map.direct_vtimer));
> > + kvm_vgic_reset_mapped_irq(
> > + vcpu,
> > timer_irq(map.direct_vtimer));
>
> Also unrelated and not a good change.
>
> > if (map.direct_ptimer)
> > - kvm_vgic_reset_mapped_irq(vcpu,
> > timer_irq(map.direct_ptimer));
> > + kvm_vgic_reset_mapped_irq(
> > + vcpu,
> > + timer_irq(map.direct_ptime
> > r));
>
> Leave all these alone.
Yeah, have done. Bad revert of a change here on my part.
>
> > }
> > }
> >
> > @@ -1092,10 +1140,19 @@ void kvm_timer_vcpu_init(struct kvm_vcpu
> > *vcpu)
> > HRTIMER_MODE_ABS_HARD);
> > }
> >
> > +/*
> > + * This is always called during kvm_arch_init_vm, but will also be
> > + * called from kvm_vgic_create if we have a vGICv5.
> > + */
> > void kvm_timer_init_vm(struct kvm *kvm)
> > {
> > + /*
> > + * Set up the default PPIs - note that we adjust them
> > based on
> > + * the model of the GIC as GICv5 uses a different way to
> > + * describing interrupts.
> > + */
> > for (int i = 0; i < NR_KVM_TIMERS; i++)
> > - kvm->arch.timer_data.ppi[i] = default_ppi[i];
> > + kvm->arch.timer_data.ppi[i] = get_vgic_ppi(kvm,
> > default_ppi[i]);
> > }
> >
> > void kvm_timer_cpu_up(void)
> > @@ -1347,6 +1404,7 @@ static int kvm_irq_init(struct
> > arch_timer_kvm_info *info)
> > }
> >
> > arch_timer_irq_ops.flags |= VGIC_IRQ_SW_RESAMPLE;
> > + arch_timer_irq_ops_vgic_v5.flags |=
> > VGIC_IRQ_SW_RESAMPLE;
> > WARN_ON(irq_domain_push_irq(domain,
> > host_vtimer_irq,
> > (void
> > *)TIMER_VTIMER));
> > }
> > @@ -1497,10 +1555,13 @@ static bool timer_irqs_are_valid(struct
> > kvm_vcpu *vcpu)
> > break;
> >
> > /*
> > - * We know by construction that we only have PPIs,
> > so
> > - * all values are less than 32.
> > + * We know by construction that we only have PPIs,
> > so all values
> > + * are less than 32 for non-GICv5 vgics. On GICv5,
> > they are
>
> VGICs maybe? It's not consistent in existing comments in this file
> though.
>
> > + * architecturally defined to be under 32 too.
> > However, we mask
> > + * off most of the bits as we might be presented
> > with a GICv5
> > + * style PPI where the type is encoded in the top-
> > bits.
> > */
> > - ppis |= BIT(irq);
> > + ppis |= BIT(irq & 0x1f);
> > }
> >
> > valid = hweight32(ppis) == nr_timers(vcpu);
> > @@ -1538,7 +1599,9 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> > {
> > struct arch_timer_cpu *timer = vcpu_timer(vcpu);
> > struct timer_map map;
> > + struct irq_ops *ops;
> > int ret;
> > + int irq;
> Might as well put irq on same line as ret
>
Actually, this should really be a u32! However, I've dropped the local
var as you suggest below.
> >
> > if (timer->enabled)
> > return 0;
> > @@ -1556,20 +1619,22 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> > return -EINVAL;
> > }
> >
> > + ops = vgic_is_v5(vcpu->kvm) ? &arch_timer_irq_ops_vgic_v5
> > :
> > + &arch_timer_irq_ops;
> > +
> > get_timer_map(vcpu, &map);
> >
> > - ret = kvm_vgic_map_phys_irq(vcpu,
> > - map.direct_vtimer-
> > >host_timer_irq,
> > - timer_irq(map.direct_vtimer),
> > - &arch_timer_irq_ops);
> > + irq = timer_irq(map.direct_vtimer);
> > + ret = kvm_vgic_map_phys_irq(vcpu, map.direct_vtimer-
> > >host_timer_irq,
> > + irq, ops);
>
> As irq is only used with this value in here, I'd avoid having the
> local variable
> that changes meaning.
Agreed.
>
> ret = kvm_vgic_map_phys_irq(vcpu, map.direct_vtimer-
> >host_timer_irq,
> timer_irq(map.direct_vtimer),
> ops);
> > if (ret)
> > return ret;
> >
> > if (map.direct_ptimer) {
> > + irq = timer_irq(map.direct_ptimer);
> > ret = kvm_vgic_map_phys_irq(vcpu,
> > map.direct_ptimer-
> > >host_timer_irq,
> > -
> > timer_irq(map.direct_ptimer),
> > - &arch_timer_irq_ops);
> > + irq, ops);
> As above
>
> timer_irq(map.direct_ptimer), ops);
>
> Doesn't make it much harder to read and avoids the local variable
> being needed.
> > }
> >
> > if (ret)
> > @@ -1627,6 +1692,15 @@ int kvm_arm_timer_set_attr(struct kvm_vcpu
> > *vcpu, struct kvm_device_attr *attr)
> > goto out;
> > }
> >
> > + /*
> > + * The PPIs for the Arch Timers arch architecturally
> > defined for
> > + * GICv5. Reject anything that changes them from the
> > specified value.
> > + */
> > + if (vgic_is_v5(vcpu->kvm) && vcpu->kvm-
> > >arch.timer_data.ppi[idx] != irq) {
> > + ret = -EINVAL;
> > + goto out;
>
> Whilst you are here, maybe throw some guard() magic dust at this and
> do a direct return?
> Or leave it for someone else who has more spare time ;)
I might as well as it makes the code cleaner.
>
> > + }
> > +
> > /*
> > * We cannot validate the IRQ unicity before we run, so
> > take it at
> > * face value. The verdict will be given on first vcpu
> > run, for each
>
> > diff --git a/include/kvm/arm_arch_timer.h
> > b/include/kvm/arm_arch_timer.h
> > index 7310841f45121..6cb9c20f9db65 100644
> > --- a/include/kvm/arm_arch_timer.h
> > +++ b/include/kvm/arm_arch_timer.h
>
> >
> > struct arch_timer_context {
> > @@ -130,6 +132,9 @@ void kvm_timer_init_vhe(void);
> > #define
> > timer_vm_data(ctx) (&(timer_context_to_vcpu(ctx)->kvm->arch.timer_data))
> > #define timer_irq(ctx) (timer_vm_data(ctx)-
> > >ppi[arch_timer_ctx_index(ctx)])
> >
> > +#define get_vgic_ppi(k, i) (((k)->arch.vgic.vgic_model !=
> > KVM_DEV_TYPE_ARM_VGIC_V5) ? \
> > + (i) : ((i) |
> > FIELD_PREP(GICV5_HWIRQ_TYPE, GICV5_HWIRQ_TYPE_PPI)))
>
> Similar to earlier comment I'd use FIELD_PREP() for i as well but not
> that important
> I'm just lazy about remembering where the numbers go.
Done, thanks.
Sascha
More information about the linux-arm-kernel
mailing list