Re: [PATCH v6 04/39] KVM: arm64: vgic: Split out mapping IRQs and setting irq_ops

Sascha Bischoff
2026-03-18T17:31:25Z

On Tue, 2026-03-17 at 16:00 +0000, Marc Zyngier wrote:
> On Tue, 17 Mar 2026 11:40:59 +0000,
> Sascha Bischoff <Sascha.Bischoff@arm.com> wrote:
> >
> > Prior to this change, the act of mapping a virtual IRQ to a
> > physical
> > one also set the irq_ops. Unmapping then reset the irq_ops to NULL.
> > So
> > far, this has been fine and hasn't caused any major issues.
> >
> > Now, however, as GICv5 support is being added to KVM, it has become
> > apparent that conflating mapping/unmapping IRQs and
> > setting/clearing
> > irq_ops can cause issues. The reason is that the upcoming GICv5
> > support introduces a set of default irq_ops for PPIs, and removing
> > this when unmapping will cause things to break rather horribly.
> >
> > Split out the mapping/unmapping of IRQs from the setting/clearing
> > of
> > irq_ops. The arch timer code is updated to set the irq_ops
> > following a
> > successful map. The irq_ops are intentionally not removed again on
> > an
> > unmap as the only irq_op introduced by the arch timer only takes
> > effect if the hw bit in struct vgic_irq is set. Therefore, it is
> > safe
> > to leave this in place, and it avoids additional complexity when
> > GICv5
> > support is introduced.
> >
> > Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> > ---
> >  arch/arm64/kvm/arch_timer.c | 32 ++++++++++++++++++-------------
> >  arch/arm64/kvm/vgic/vgic.c  | 38 +++++++++++++++++++++++++++++++--
> > ----
> >  include/kvm/arm_vgic.h      |  5 ++++-
> >  3 files changed, 55 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/arch_timer.c
> > b/arch/arm64/kvm/arch_timer.c
> > index 600f250753b45..1f536dd5978d4 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -740,14 +740,17 @@ static void
> > kvm_timer_vcpu_load_nested_switch(struct kvm_vcpu *vcpu,
> >
> >   ret = kvm_vgic_map_phys_irq(vcpu,
> >       map->direct_vtimer-
> > >host_timer_irq,
> > -     timer_irq(map-
> > >direct_vtimer),
> > -     &arch_timer_irq_ops);
> > - WARN_ON_ONCE(ret);
> > +     timer_irq(map-
> > >direct_vtimer));
> > + if (!WARN_ON_ONCE(ret))
> > + kvm_vgic_set_irq_ops(vcpu, timer_irq(map-
> > >direct_vtimer),
> > +      &arch_timer_irq_ops);
> > +
> >   ret = kvm_vgic_map_phys_irq(vcpu,
> >       map->direct_ptimer-
> > >host_timer_irq,
> > -     timer_irq(map-
> > >direct_ptimer),
> > -     &arch_timer_irq_ops);
> > - WARN_ON_ONCE(ret);
> > +     timer_irq(map-
> > >direct_ptimer));
> > + if (!WARN_ON_ONCE(ret))
> > + kvm_vgic_set_irq_ops(vcpu, timer_irq(map-
> > >direct_ptimer),
> > +      &arch_timer_irq_ops);
>
> Do we really need this eager setting of ops? Given that nothing seems
> to clear them, why can't we just set the ops at vcpu init time? Given
> that this is a pretty hot path (on each exception/exception return
> between L2 and L1), the least we do here, the better.

Hmm, I think you're right. When making this change, I was trying to
preserve the existing behaviour so set the irq_ops for each map call.
However, as you say nothing is clearing the ops (as things stand, at
least), so that does indeed make sense to do IMO.

>
> >   }
> >  }
> >
> > @@ -1565,20 +1568,23 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >
> >   ret = kvm_vgic_map_phys_irq(vcpu,
> >       map.direct_vtimer-
> > >host_timer_irq,
> > -     timer_irq(map.direct_vtimer),
> > -     &arch_timer_irq_ops);
> > +     timer_irq(map.direct_vtimer));
> >   if (ret)
> >   return ret;
> >
> > + kvm_vgic_set_irq_ops(vcpu, timer_irq(map.direct_vtimer),
> > +      &arch_timer_irq_ops);
> > +
> >   if (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);
> > - }
> > +
> > timer_irq(map.direct_ptimer));
> > + if (ret)
> > + return ret;
> >
> > - if (ret)
> > - return ret;
> > + kvm_vgic_set_irq_ops(vcpu,
> > timer_irq(map.direct_ptimer),
> > +      &arch_timer_irq_ops);
> > + }
>
> which would mean moving this to kvm_timer_vcpu_init().

This, however, is not quite that simple.

It turns out that we actually call kvm_timer_vcpu_init() before
kvm_vgic_vcpu_init() from kvm_arch_vcpu_create(), meaning that we don't
have the private IRQs allocated.

Is there a good reason that the timer (and PMU) are initialised prior
to initialising the CPU?

I've tried making this chang, and once I reorder the timer and vcpu
initialisation I can confirm that things work with and without nested.

>
> >
> >  no_vgic:
> >   timer->enabled = 1;
> > diff --git a/arch/arm64/kvm/vgic/vgic.c
> > b/arch/arm64/kvm/vgic/vgic.c
> > index e22b79cfff965..e37c640d74bcf 100644
> > --- a/arch/arm64/kvm/vgic/vgic.c
> > +++ b/arch/arm64/kvm/vgic/vgic.c
> > @@ -553,10 +553,38 @@ int kvm_vgic_inject_irq(struct kvm *kvm,
> > struct kvm_vcpu *vcpu,
> >   return 0;
> >  }
> >
> > +void kvm_vgic_set_irq_ops(struct kvm_vcpu *vcpu, u32 vintid,
> > + struct irq_ops *ops)
> > +{
> > + struct vgic_irq *irq = vgic_get_vcpu_irq(vcpu, vintid);
> > +
> > + BUG_ON(!irq);
> > +
> > + scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> > + {
> > + irq->ops = ops;
> > + }
>
> nit: opening brace in the wrong spot, and overall not useful. This
> could simply be written as:
>
> scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> irq->ops = ops;

Argh, sorry that slipped through!

>
> > +
> > + vgic_put_irq(vcpu->kvm, irq);
> > +}
> > +
> > +void kvm_vgic_clear_irq_ops(struct kvm_vcpu *vcpu, u32 vintid)
> > +{
> > + struct vgic_irq *irq = vgic_get_vcpu_irq(vcpu, vintid);
> > +
> > + BUG_ON(!irq);
> > +
> > + scoped_guard(raw_spinlock_irqsave, &irq->irq_lock)
> > + {
> > + irq->ops = NULL;
> > + }
> > +
> > + vgic_put_irq(vcpu->kvm, irq);
> > +}
> > +
>
> nit: that could also be written as:
>
> void kvm_vgic_clear_irq_ops(struct kvm_vcpu *vcpu, u32 vintid)
> {
> kvm_vgic_set_irq_ops(vcpu, vintid, NULL);
> }

Ah, that is indeed cleaner.

>
> I can fix all of it when applying if that works for you.

If you're happy to do that, that is great! Do note what I said above
regarding the order of vcpu and timer init!

Thanks,
Sascha

>
> Thanks,
>
> M.
>