Re: [PATCH 1/2] genirq/manage: Provide irq_set_percpu_irqchip_state()

[Marc Zyngier <maz@kernel.org>]

On Sat, 24 Oct 2026 22:28:44 +0100,
Amir Noohi <amirmnoohi@gmail.com> wrote:
> 
> Hi Marc,
> 
> On Sat, 24 Oct 2026 13:21:00 +0100, Marc Zyngier <maz@kernel.org> wrote:
> > It really feels like a leap of faith.
> >
> > There is nothing that says that it is safe to call this without
> > holding the global lock. For all we know, drivers have been relying on
> > it for a good decade, and I don't feel confident letting things rip
> > just because it (mostly) works with the GIC.
> >
> > At the very least, something exposed by the irqchip (a per-IRQ flag?)
> > should tell you whether it is safe to elide the lock.
> 
> Agreed, and v2 does that. There's a new IRQCHIP_PERCPU_STATE_LOCKLESS
> flag, and irq_set_percpu_irqchip_state() only skips the descriptor lock
> if the chip implementing irq_set_irqchip_state() has it set. Otherwise,
> or if there's a bus lock, it just calls irq_set_irqchip_state(), so
> drivers that don't opt in see no change.

I'm not sure this is the correct option. Making it per irqchip is
again assuming that the irqchip has a uniform handling of the state
across all interrupts. I'm afraid that's not universally true, which
is why I hinted at a per-IRQ flag.

Alternatively, you can audit all the providers of this API and work
out if is it safe to do so, and split the corresponding callbacks.
Good luck with that. :-/

> 
> Only GICv3 sets it, in a separate patch. For SGIs/PPIs/EPPIs its
> callback only touches the local redistributor (gic_data_rdist_sgi_base()
> and the RWP poll), nothing shared.

Read the code again. There is *one* callback for all distributor-owned
interrupts, which also involves SPIs/ESPIs.

> FWIW, irq-gic.c, irq-gic-v3.c and
> irq-gic-v5.c are the only drivers with both per-CPU devid interrupts and
> an irq_set_irqchip_state() callback. KVM's own timer chip (GICv5, no HW
> deactivation) doesn't set it either, so those keep the lock.

The KVM chip inherits whatever it is pushed upon, and it is still a
per-CPU interrupt. To be clear, this is an all or nothing approach as
far as KVM is concerned, so anything that KVM uses should be converted
to the lockless approach.

Even in the GICv5 case, the ICC_PPI_ENABLER0 register cannot be
modified concurrently, so it should be safe to advertise these as
lockless.

> 
> On GICv2 in QEMU it still takes the lock like mainline does (333k vs
> 321k acquisitions in lock_stat, within 4%), and on GICv3 it doesn't
> take it at all. arch_timer, arch_timer_edge_cases, vgic_irq and
> vgic_init pass on the Altra. Under QEMU with lockdep, DEBUG_PREEMPT and
> DEBUG_ATOMIC_SLEEP, the selftests pass on GICv3 (VHE and nVHE) and
> GICv2 (the vGICv3 ones skip there). No splats.
> 
> I made it a chip flag since it's about the callback, but I can make it
> per-IRQ if you prefer.

I reckon this is a requirement.

Thanks,

	M.

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