Re: [PATCH v2 6/6] KVM: arm64: vgic-v3: Indicate vgic_put_irq() may take LPI xarray lock

[Marc Zyngier <maz@kernel.org>]

On Wed, 05 Nov 2025 09:37:10 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> Hi Oliver,
> 
> On 2025/9/5 18:05, Oliver Upton wrote:
> > The release path on LPIs is quite rare, meaning it can be difficult to
> > find lock ordering bugs on the LPI xarray's spinlock. Tell lockdep that
> > vgic_put_irq() might acquire the xa_lock to make unsafe patterns more
> > obvious.
> > 
> > Reviewed-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/kvm/vgic/vgic.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> > index a21b482844ce..3b247041a130 100644
> > --- a/arch/arm64/kvm/vgic/vgic.c
> > +++ b/arch/arm64/kvm/vgic/vgic.c
> > @@ -142,6 +142,9 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
> >  {
> >  	struct vgic_dist *dist = &kvm->arch.vgic;
> >  
> > +	if (irq->intid >= VGIC_MIN_LPI)
> > +		might_lock(&dist->lpi_xa.xa_lock);
> 
> I got the following splat on a lockdep kernel. The reproducing step can
> be easily inferred from the backtrace (i.e., starting a guest with an
> assigned device).
> 
>  ================================
>  WARNING: inconsistent lock state
>  6.18.0-rc4-00019-g284922f4c563-dirty #2390 Not tainted
>  --------------------------------
>  inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
>  swapper/10/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
>  ffff8000a504de18 (&xa->xa_lock#19){?.+.}-{3:3}, at: vgic_put_irq+0x28/0x110
>  {HARDIRQ-ON-W} state was registered at:
>    lock_acquire+0x1c8/0x354
>    _raw_spin_lock+0x48/0x60
>    vgic_add_lpi.part.0+0x70/0x2f8
>    vgic_its_cmd_handle_mapi.isra.0+0x398/0x418
>    vgic_its_process_commands.part.0+0x4d4/0xfa0
>    vgic_mmio_write_its_cwriter+0x80/0xa4
>    dispatch_mmio_write+0xd0/0x128
>    __kvm_io_bus_write+0xb4/0xe8
>    kvm_io_bus_write+0x58/0x98
>    io_mem_abort+0xe8/0x3f0
>    kvm_handle_guest_abort+0x4d0/0x1414
>    handle_exit+0x6c/0x1c4
>    kvm_arch_vcpu_ioctl_run+0x678/0xbfc
>    kvm_vcpu_ioctl+0x1ac/0xb24
>    __arm64_sys_ioctl+0xac/0x104
>    invoke_syscall+0x48/0x10c
>    el0_svc_common.constprop.0+0x40/0xe0
>    do_el0_svc+0x1c/0x28
>    el0_svc+0x50/0x2c0
>    el0t_64_sync_handler+0xa0/0xe4
>    el0t_64_sync+0x198/0x19c
>  irq event stamp: 5415534
>  hardirqs last  enabled at (5415533): [<ffff8000813e291c>]
> default_idle_call+0x7c/0x138
>  hardirqs last disabled at (5415534): [<ffff8000813dabf4>]
> enter_from_kernel_mode+0x10/0x3c
>  softirqs last  enabled at (5415516): [<ffff8000800c7b54>]
> handle_softirqs+0x4ac/0x4c4
>  softirqs last disabled at (5415511): [<ffff800080010748>]
> __do_softirq+0x14/0x20
> 
>  other info that might help us debug this:
>   Possible unsafe locking scenario:
> 
>         CPU0
>         ----
>    lock(&xa->xa_lock#19);
>    <Interrupt>
>      lock(&xa->xa_lock#19);
> 
>   *** DEADLOCK ***
> 
>  2 locks held by swapper/10/0:
>   #0: ffff00280db646a0 (&ctx->wqh#2){-...}-{3:3}, at:
> eventfd_signal_mask+0x38/0xc0
>   #1: ffff8000a504e480 (&kvm->irq_srcu){.?.+}-{0:0}, at:
> irqfd_wakeup+0x88/0x2ac
> 
>  stack backtrace:
>  CPU: 10 UID: 0 PID: 0 Comm: swapper/10 Kdump: loaded Not tainted
> 6.18.0-rc4-00019-g284922f4c563-dirty #2390 PREEMPT
>  Call trace:
>   show_stack+0x18/0x24 (C)
>   dump_stack_lvl+0x90/0xd0
>   dump_stack+0x18/0x24
>   print_usage_bug.part.0+0x29c/0x358
>   mark_lock+0x6c0/0x960
>   __lock_acquire+0xd4c/0x20fc
>   lock_acquire+0x1c8/0x354
>   vgic_put_irq+0x54/0x110
>   vgic_its_inject_cached_translation+0x178/0x25c
>   kvm_arch_set_irq_inatomic+0xac/0x124

Right. This might_lock() is gross, and clearly doesn't do the right
thing outside of direct injection of LPIs.

I think we should drop it, but we should ensure that lpi_xa.xa_lock is
never taken in interrupt context.

Oliver, what do you think?

	M.

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