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

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

[Andrew Jones]

kvm_riscv_vcpu_aia_imsic_put() runs from the scheduler preemption
notifier with the raw runqueue lock held, so it must not take
vsfile_lock, an rwlock_t, which sleeps on PREEMPT_RT. All vsfile_lock
critical sections are short, so a raw spinlock is fine.

Fixes: 4cec89db80ba ("RISC-V: KVM: Move HGEI[E|P] CSR access to IMSIC virtualization")
Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
v2:
 - Convert vsfile_lock to raw_spinlock_t instead of adding a separate
   hgei_lock [Anup]
---
 arch/riscv/kvm/aia_imsic.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
index f17dc559a709..bad0ce20edc1 100644
--- a/arch/riscv/kvm/aia_imsic.c
+++ b/arch/riscv/kvm/aia_imsic.c
@@ -47,7 +47,7 @@ struct imsic {
 	 */
 
 	/* IMSIC VS-file */
-	rwlock_t vsfile_lock;
+	raw_spinlock_t vsfile_lock;
 	int vsfile_cpu;
 	int vsfile_hgei;
 	void __iomem *vsfile_va;
@@ -593,13 +593,13 @@ static void imsic_vsfile_cleanup(struct imsic *imsic)
 	 * VCPU is being destroyed.
 	 */
 
-	write_lock_irqsave(&imsic->vsfile_lock, flags);
+	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
 	old_vsfile_hgei = imsic->vsfile_hgei;
 	old_vsfile_cpu = imsic->vsfile_cpu;
 	imsic->vsfile_cpu = imsic->vsfile_hgei = -1;
 	imsic->vsfile_va = NULL;
 	imsic->vsfile_pa = 0;
-	write_unlock_irqrestore(&imsic->vsfile_lock, flags);
+	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
 
 	memset(imsic->swfile, 0, sizeof(*imsic->swfile));
 
@@ -687,7 +687,7 @@ bool kvm_riscv_vcpu_aia_imsic_has_interrupt(struct kvm_vcpu *vcpu)
 	 * only check for interrupt when IMSIC VS-file is being used.
 	 */
 
-	read_lock_irqsave(&imsic->vsfile_lock, flags);
+	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
 	if (imsic->vsfile_cpu > -1) {
 		/*
 		 * This function is typically called from kvm_vcpu_block() via
@@ -702,7 +702,7 @@ bool kvm_riscv_vcpu_aia_imsic_has_interrupt(struct kvm_vcpu *vcpu)
 		else
 			ret = !!(csr_read(CSR_HGEIP) & BIT(imsic->vsfile_hgei));
 	}
-	read_unlock_irqrestore(&imsic->vsfile_lock, flags);
+	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
 
 	return ret;
 }
@@ -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);
 }
 
 void kvm_riscv_vcpu_aia_imsic_release(struct kvm_vcpu *vcpu)
@@ -744,13 +744,13 @@ void kvm_riscv_vcpu_aia_imsic_release(struct kvm_vcpu *vcpu)
 		return;
 
 	/* Read and clear IMSIC VS-file details */
-	write_lock_irqsave(&imsic->vsfile_lock, flags);
+	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
 	old_vsfile_hgei = imsic->vsfile_hgei;
 	old_vsfile_cpu = imsic->vsfile_cpu;
 	imsic->vsfile_cpu = imsic->vsfile_hgei = -1;
 	imsic->vsfile_va = NULL;
 	imsic->vsfile_pa = 0;
-	write_unlock_irqrestore(&imsic->vsfile_lock, flags);
+	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
 
 	/* Do nothing, if no IMSIC VS-file to release */
 	if (old_vsfile_cpu < 0)
@@ -807,10 +807,10 @@ int kvm_riscv_vcpu_aia_imsic_update(struct kvm_vcpu *vcpu)
 		return 1;
 
 	/* Read old IMSIC VS-file details */
-	read_lock_irqsave(&imsic->vsfile_lock, flags);
+	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
 	old_vsfile_hgei = imsic->vsfile_hgei;
 	old_vsfile_cpu = imsic->vsfile_cpu;
-	read_unlock_irqrestore(&imsic->vsfile_lock, flags);
+	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
 
 	/* Do nothing if we are continuing on same CPU */
 	if (old_vsfile_cpu == vcpu->cpu)
@@ -860,12 +860,12 @@ int kvm_riscv_vcpu_aia_imsic_update(struct kvm_vcpu *vcpu)
 	/* TODO: Update the IOMMU mapping ??? */
 
 	/* Update new IMSIC VS-file details in IMSIC context */
-	write_lock_irqsave(&imsic->vsfile_lock, flags);
+	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
 	imsic->vsfile_hgei = new_vsfile_hgei;
 	imsic->vsfile_cpu = vcpu->cpu;
 	imsic->vsfile_va = new_vsfile_va;
 	imsic->vsfile_pa = new_vsfile_pa;
-	write_unlock_irqrestore(&imsic->vsfile_lock, flags);
+	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
 
 	/*
 	 * At this point, all interrupt producers have been moved
@@ -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;
 }
@@ -1118,7 +1118,7 @@ int kvm_riscv_vcpu_aia_imsic_init(struct kvm_vcpu *vcpu)
 
 	/* Setup IMSIC context  */
 	imsic->nr_msis = kvm->arch.aia.nr_ids + 1;
-	rwlock_init(&imsic->vsfile_lock);
+	raw_spin_lock_init(&imsic->vsfile_lock);
 	imsic->nr_eix = BITS_TO_U64(imsic->nr_msis);
 	imsic->nr_hw_eix = BITS_TO_U64(kvm_riscv_aia_max_ids);
 	imsic->vsfile_hgei = imsic->vsfile_cpu = -1;
-- 
2.56.0

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

[sashiko-bot]

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

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

[Andrew Jones]

On Mon, Oct 19, 2026 at 03:47:09PM +0200, Andrew Jones wrote:
> kvm_riscv_vcpu_aia_imsic_put() runs from the scheduler preemption
> notifier with the raw runqueue lock held, so it must not take
> vsfile_lock, an rwlock_t, which sleeps on PREEMPT_RT. All vsfile_lock
> critical sections are short, so a raw spinlock is fine.
> 
> Fixes: 4cec89db80ba ("RISC-V: KVM: Move HGEI[E|P] CSR access to IMSIC virtualization")
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
> v2:
>  - Convert vsfile_lock to raw_spinlock_t instead of adding a separate
>    hgei_lock [Anup]

Hi Anup,

Sashiko found an ABBA deadlock with this v2[1]. Making vsfile_lock
exclusive serializes kvm_riscv_vcpu_aia_imsic_put(), which runs under
rq->__lock, against the SW-file inject paths, which kick the vCPU
(pi_lock/rq->__lock) while holding vsfile_lock. Lockdep confirms it with
an EMUL mode guest, and the host then hangs (I should have tested with
EMUL before...). v1 doesn't have the problem, since its hgei_lock never
has anything that wakes tasks nested under it. I tested v1 in both EMUL
mode and hwaccel mode [again] with lockdep and it's good.

Should we take v1 instead?

[1] https://sashiko.dev/#/patchset/20261019134709.2610129-1-andrew.jones@linux.dev?part=1

Thanks,
drew