Re: [PATCH v2] RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RISC-V: KVM: ABBA deadlock on vsfile_lock in kvm_riscv_vcpu_aia_imsic_put()
--

commit 87fb1e91c477214da3458429ee5e7b7ff86ac173
Author: Andrew Jones <andrew.jones@linux.dev>

RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock

This commit converts imsic->vsfile_lock from rwlock_t to raw_spinlock_t
so that kvm_riscv_vcpu_aia_imsic_put() does not sleep while holding the
scheduler runqueue lock on PREEMPT_RT.

> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> index f17dc559a7092..bad0ce20edc11 100644
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c

[ ... ]

> @@ -727,10 +727,10 @@ void kvm_riscv_vcpu_aia_imsic_put(struct kvm_vcpu *vcpu)
>  	if (!kvm_vcpu_is_blocking(vcpu))
>  		return;
>  
> -	read_lock_irqsave(&imsic->vsfile_lock, flags);
> +	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
>  	if (imsic->vsfile_cpu > -1)
>  		csr_set(CSR_HGEIE, BIT(imsic->vsfile_hgei));
> -	read_unlock_irqrestore(&imsic->vsfile_lock, flags);
> +	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
>  }

[ ... ]

> @@ -975,7 +975,7 @@ int kvm_riscv_aia_imsic_rw_attr(struct kvm *kvm, unsigned long type,
>  	}
>  	isel = KVM_DEV_RISCV_AIA_IMSIC_GET_ISEL(type);
>  
> -	read_lock_irqsave(&imsic->vsfile_lock, flags);
> +	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
>  
>  	rc = 0;
>  	vsfile_hgei = imsic->vsfile_hgei;
> @@ -990,7 +990,7 @@ int kvm_riscv_aia_imsic_rw_attr(struct kvm *kvm, unsigned long type,
>  					    isel, val, 0, 0);
>  	}
>  
> -	read_unlock_irqrestore(&imsic->vsfile_lock, flags);
> +	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
>  
>  	if (!rc && vsfile_cpu >= 0)
>  		rc = imsic_vsfile_rw(vsfile_hgei, vsfile_cpu, imsic->nr_eix,
> @@ -1052,7 +1052,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>  	if (imsic->nr_msis <= iid)
>  		return -EINVAL;
>  
> -	read_lock_irqsave(&imsic->vsfile_lock, flags);
> +	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
>  
>  	if (imsic->vsfile_cpu >= 0) {
>  		writel(iid, imsic->vsfile_va + IMSIC_MMIO_SETIPNUM_LE);
> @@ -1062,7 +1062,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>  		imsic_swfile_extirq_update(vcpu);
>  	}
>  
> -	read_unlock_irqrestore(&imsic->vsfile_lock, flags);
> +	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
>  
>  	return 0;
>  }

[Severity: High]
Can converting vsfile_lock from rwlock_t to an exclusive raw_spinlock_t
cause an ABBA deadlock between rq->__lock and imsic->vsfile_lock when
imsic->vsfile_cpu < 0 (such as in KVM_DEV_RISCV_AIA_MODE_EMUL, in
KVM_DEV_RISCV_AIA_MODE_AUTO when HGEI lines are exhausted, or after
kvm_riscv_vcpu_aia_imsic_release())?

When a vCPU blocks in kvm_vcpu_block(), prepare_to_rcuwait() sets the
rcuwait task before schedule() is called, making kvm_vcpu_is_blocking()
return true. During the context switch out on CPU 0, rq->__lock is held
and prev->on_cpu is still 1 when the preempt notifier runs:

CPU 0:
__schedule() [holds rq->__lock, prev->on_cpu == 1]
  -> context_switch()
    -> prepare_task_switch()
      -> fire_sched_out_preempt_notifiers()
        -> kvm_sched_out()
          -> kvm_arch_vcpu_put()
            -> kvm_riscv_vcpu_aia_put()
              -> kvm_riscv_vcpu_aia_imsic_put()
                -> raw_spin_lock_irqsave(&imsic->vsfile_lock, flags)

This establishes the lock ordering rq->__lock -> imsic->vsfile_lock.

Concurrently on CPU 1, kvm_riscv_vcpu_aia_imsic_inject() or
kvm_riscv_aia_imsic_rw_attr() acquires imsic->vsfile_lock and, when
imsic->vsfile_cpu < 0, calls imsic_swfile_extirq_update() while still
holding imsic->vsfile_lock:

CPU 1:
kvm_riscv_vcpu_aia_imsic_inject() / kvm_riscv_aia_imsic_rw_attr()
  -> raw_spin_lock_irqsave(&imsic->vsfile_lock, flags)
  -> imsic_swfile_extirq_update()
    -> kvm_riscv_vcpu_set_interrupt()
      -> kvm_vcpu_kick()
        -> kvm_vcpu_wake_up()
          -> rcuwait_wake_up()
            -> wake_up_process()
              -> try_to_wake_up()

Inside try_to_wake_up(), CPU 1 acquires p->pi_lock and either spins
waiting for CPU 0's rq->__lock (for example in ttwu_runnable() when
delayed dequeue keeps p->on_rq set, or if preempted after
prepare_to_rcuwait()) or spins in smp_cond_load_acquire(&p->on_cpu, !VAL)
waiting for CPU 0 to finish switching out the vCPU task. Meanwhile, CPU 0
is spinning on imsic->vsfile_lock in prepare_task_switch() before
finish_task() can clear p->on_cpu or release rq->__lock.

Previously, both paths took read_lock_irqsave(&imsic->vsfile_lock, flags)
and did not block each other unless a writer was active (and writers run
with vcpu->mutex held outside schedule()). With raw_spinlock_t, these two
paths now serialize against each other and can deadlock.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019134709.2610129-1-andrew.jones@linux.dev?part=1