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

Marc Zyngier
2026-03-17T16:00:59Z

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.

> }
> }
>
> @@ -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().

>
> 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;

> +
> + 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);
}

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

Thanks,

M.

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