* [PATCH] RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock
@ 2026-11-14 13:50 Anup Patel
2026-11-14 14:08 ` sashiko-bot
0 siblings, 1 reply; 2+ messages in thread
From: Anup Patel @ 2026-11-14 13:50 UTC (permalink / raw)
To: Palmer Dabbelt, Paul Walmsley, Atish Patra, Andrew Jones
Cc: Paolo Bonzini, Anup Patel, kvm, kvm-riscv, linux-riscv,
linux-kernel, Anup Patel
The 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.
In addition, converting vsfile_lock to raw spinlock allows us to drop
the vsfile_hgei_lock and also allows us to use writel_relaxed() in
kvm_riscv_vcpu_aia_imsic_inject() since raw_spin_unlock() internally
does mmiowb_spin_unlock() which in-turn drains the write-buffer.
Co-developed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
arch/riscv/kvm/aia_imsic.c | 79 ++++++++++++++++++++------------------
1 file changed, 41 insertions(+), 38 deletions(-)
diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
index 302938972aa0..0b9d15721d52 100644
--- a/arch/riscv/kvm/aia_imsic.c
+++ b/arch/riscv/kvm/aia_imsic.c
@@ -47,9 +47,7 @@ struct imsic {
*/
/* IMSIC VS-file */
- rwlock_t vsfile_lock;
- /* Writers hold both locks; scheduler-out only takes vsfile_hgei_lock. */
- raw_spinlock_t vsfile_hgei_lock;
+ raw_spinlock_t vsfile_lock;
int vsfile_cpu;
int vsfile_hgei;
void __iomem *vsfile_va;
@@ -490,17 +488,6 @@ static void imsic_vsfile_local_update(int vsfile_hgei, u32 nr_eix,
csr_write(CSR_VSISELECT, old_vsiselect);
}
-static void imsic_vsfile_set_hgei(struct imsic *imsic, int cpu, int hgei)
-{
- unsigned long flags;
-
- /* Serialize the CPU/HGEI pair with scheduler-out's wakeup setup. */
- raw_spin_lock_irqsave(&imsic->vsfile_hgei_lock, flags);
- imsic->vsfile_cpu = cpu;
- imsic->vsfile_hgei = hgei;
- raw_spin_unlock_irqrestore(&imsic->vsfile_hgei_lock, flags);
-}
-
static void imsic_vsfile_cleanup(struct imsic *imsic)
{
int old_vsfile_hgei, old_vsfile_cpu;
@@ -512,13 +499,14 @@ 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_set_hgei(imsic, -1, -1);
+ imsic->vsfile_cpu = -1;
+ 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));
@@ -606,7 +594,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
@@ -621,7 +609,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;
}
@@ -646,10 +634,10 @@ void kvm_riscv_vcpu_aia_imsic_put(struct kvm_vcpu *vcpu)
if (!kvm_vcpu_is_blocking(vcpu))
return;
- raw_spin_lock_irqsave(&imsic->vsfile_hgei_lock, flags);
+ raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
if (imsic->vsfile_cpu > -1)
csr_set(CSR_HGEIE, BIT(imsic->vsfile_hgei));
- raw_spin_unlock_irqrestore(&imsic->vsfile_hgei_lock, flags);
+ raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
}
void kvm_riscv_vcpu_aia_imsic_release(struct kvm_vcpu *vcpu)
@@ -663,13 +651,14 @@ 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_set_hgei(imsic, -1, -1);
+ imsic->vsfile_cpu = -1;
+ 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)
@@ -726,10 +715,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)
@@ -779,11 +768,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);
- imsic_vsfile_set_hgei(imsic, vcpu->cpu, new_vsfile_hgei);
+ raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
+ imsic->vsfile_cpu = vcpu->cpu;
+ imsic->vsfile_hgei = new_vsfile_hgei;
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
@@ -874,6 +864,7 @@ int kvm_riscv_aia_imsic_rw_attr(struct kvm *kvm, unsigned long type,
struct imsic *imsic;
struct kvm_vcpu *vcpu;
int rc, vsfile_hgei, vsfile_cpu;
+ bool swfile_update = false;
if (!kvm_riscv_aia_initialized(kvm))
return -ENODEV;
@@ -893,7 +884,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;
@@ -902,13 +893,16 @@ int kvm_riscv_aia_imsic_rw_attr(struct kvm *kvm, unsigned long type,
if (write) {
rc = imsic_mrif_rmw(imsic->swfile, imsic->nr_eix,
isel, NULL, *val, -1UL);
- imsic_swfile_extirq_update(vcpu);
+ swfile_update = true;
} else
rc = imsic_mrif_rmw(imsic->swfile, imsic->nr_eix,
isel, val, 0, 0);
}
- read_unlock_irqrestore(&imsic->vsfile_lock, flags);
+ raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
+
+ if (swfile_update)
+ imsic_swfile_extirq_update(vcpu);
if (!rc && vsfile_cpu >= 0)
rc = imsic_vsfile_rw(vsfile_hgei, vsfile_cpu, imsic->nr_eix,
@@ -959,6 +953,7 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
unsigned long flags;
struct imsic_mrif_eix *eix;
struct imsic *imsic = vcpu->arch.aia_context.imsic_state;
+ bool swfile_update = false;
/* We only emulate one IMSIC MMIO page for each Guest VCPU */
if (!imsic || !iid || guest_index ||
@@ -970,17 +965,26 @@ 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);
+ /*
+ * We can use writel_relaxed() over here because the
+ * raw_spin_unlock() below will do mmiowb_spin_unlock()
+ * which in-turn drains the write-buffer ensuring the
+ * write is visible to the CPU owning the IMSIC VS-file.
+ */
+ writel_relaxed(iid, imsic->vsfile_va + IMSIC_MMIO_SETIPNUM_LE);
} else {
eix = &imsic->swfile->eix[iid / BITS_PER_TYPE(u64)];
set_bit(iid & (BITS_PER_TYPE(u64) - 1), eix->eip);
- imsic_swfile_extirq_update(vcpu);
+ swfile_update = true;
}
- read_unlock_irqrestore(&imsic->vsfile_lock, flags);
+ raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
+
+ if (swfile_update)
+ imsic_swfile_extirq_update(vcpu);
return 0;
}
@@ -1036,8 +1040,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_hgei_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.43.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock
2026-11-14 13:50 [PATCH] RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock Anup Patel
@ 2026-11-14 14:08 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-11-14 14:08 UTC (permalink / raw)
To: Anup Patel; +Cc: kvm
Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] KVM: riscv: unordered MMIO write in
kvm_riscv_vcpu_aia_imsic_inject()
- [High] KVM: riscv: race on IRQ_VS_EXT in
kvm_riscv_vcpu_aia_imsic_inject()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] RISC-V: KVM: double-free in
kvm_riscv_vcpu_aia_imsic_update() [1]
--
commit 7416cdfbe4d0521c182f4faca63b3ac6e4ae60b3
Author: Anup Patel <anup.patel@oss.qualcomm.com>
RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock
This commit converts imsic->vsfile_lock from rwlock_t to raw_spinlock_t,
removes vsfile_hgei_lock, moves imsic_swfile_extirq_update() outside
vsfile_lock, and switches kvm_riscv_vcpu_aia_imsic_inject() to
writel_relaxed().
> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> index 302938972aa0e..0b9d15721d52f 100644
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c
[ ... ]
> @@ -726,10 +715,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);
[Severity: Critical]
This problem wasn't introduced by this patch, but can [1] cause a
double free of old_vsfile_hgei when kvm_riscv_vcpu_aia_imsic_update()
races with kvm_riscv_vcpu_aia_imsic_release()?
kvm_riscv_vcpu_aia_imsic_update() caches old_vsfile_hgei and
old_vsfile_cpu under imsic->vsfile_lock and drops the lock before later
freeing the old HGEI slot:
CPU0 CPU1
kvm_riscv_vcpu_aia_imsic_update() kvm_riscv_vcpu_aia_imsic_release()
raw_spin_lock_irqsave(...)
old_vsfile_hgei = imsic->vsfile_hgei;
old_vsfile_cpu = imsic->vsfile_cpu;
raw_spin_unlock_irqrestore(...)
raw_spin_lock_irqsave(...)
old_vsfile_hgei = imsic->vsfile_hgei;
old_vsfile_cpu = imsic->vsfile_cpu;
imsic->vsfile_cpu = -1;
imsic->vsfile_hgei = -1;
raw_spin_unlock_irqrestore(...)
...
kvm_riscv_aia_free_hgei(...)
...
if (old_vsfile_cpu >= 0) {
...
kvm_riscv_aia_free_hgei(...) <-- double free
If a concurrent CPU hotplug or vCPU reset runs
kvm_riscv_vcpu_aia_imsic_release() in that window, both paths free the
same HGEI slot. If that slot was reallocated to another VM in between,
the second free corrupts cross-VM interrupt state.
[ ... ]
> @@ -970,17 +965,26 @@ 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);
> + /*
> + * We can use writel_relaxed() over here because the
> + * raw_spin_unlock() below will do mmiowb_spin_unlock()
> + * which in-turn drains the write-buffer ensuring the
> + * write is visible to the CPU owning the IMSIC VS-file.
> + */
> + writel_relaxed(iid, imsic->vsfile_va + IMSIC_MMIO_SETIPNUM_LE);
[Severity: High]
Does replacing writel() with writel_relaxed() leave the MMIO write to
imsic->vsfile_va unordered with raw_spin_unlock_irqrestore()?
mmiowb_spin_unlock() only emits mmiowb() when ms->mmiowb_pending is set:
include/asm-generic/mmiowb.h:mmiowb_spin_unlock() {
struct mmiowb_state *ms = __mmiowb_state();
if (unlikely(ms->mmiowb_pending)) {
ms->mmiowb_pending = 0;
mmiowb();
}
}
On RISC-V, writel() calls __io_bw() and __io_aw() (which calls
mmiowb_set_pending()), whereas writel_relaxed() uses __io_rbw() and
__io_raw(), which are both no-ops. Because writel_relaxed() never sets
ms->mmiowb_pending, raw_spin_unlock_irqrestore() does not emit mmiowb()
(RISCV_FENCE(o, w)). If the vCPU concurrently migrates or releases its
VS-file in kvm_riscv_vcpu_aia_imsic_update() or
kvm_riscv_vcpu_aia_imsic_release(), the store to IMSIC_MMIO_SETIPNUM_LE
can arrive after imsic_vsfile_read() has cleared the old VS-file and
kvm_riscv_aia_free_hgei() has freed the slot, losing the guest interrupt
or delivering a spurious MSI to another vCPU.
> } else {
> eix = &imsic->swfile->eix[iid / BITS_PER_TYPE(u64)];
> set_bit(iid & (BITS_PER_TYPE(u64) - 1), eix->eip);
> - imsic_swfile_extirq_update(vcpu);
> + swfile_update = true;
> }
>
> - read_unlock_irqrestore(&imsic->vsfile_lock, flags);
> + raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
> +
> + if (swfile_update)
> + imsic_swfile_extirq_update(vcpu);
[Severity: High]
Can moving imsic_swfile_extirq_update() outside imsic->vsfile_lock race
with kvm_riscv_vcpu_aia_imsic_update() and leave IRQ_VS_EXT stuck pending?
imsic_swfile_read() clears imsic->swfile and unsets IRQ_VS_EXT without
taking swfile_extirq_lock, relying on vsfile_lock to serialize against
SW-file producers:
CPU0 CPU1
kvm_riscv_vcpu_aia_imsic_inject() kvm_riscv_vcpu_aia_imsic_update()
raw_spin_lock_irqsave(&vsfile_lock)
/* vsfile_cpu < 0 */
set_bit(..., eix->eip)
swfile_update = true
raw_spin_unlock_irqrestore(...)
imsic_swfile_extirq_update()
raw_spin_lock_irqsave(&extirq_lock)
/* reads eidelivery && topei != 0 */
raw_spin_lock_irqsave(&vsfile_lock)
imsic->vsfile_cpu = vcpu->cpu
raw_spin_unlock_irqrestore(...)
imsic_swfile_read(..., true, ...)
memset(imsic->swfile, 0, ...)
kvm_riscv_vcpu_unset_interrupt(
vcpu, IRQ_VS_EXT)
kvm_riscv_vcpu_set_interrupt(
vcpu, IRQ_VS_EXT) <-- re-asserts IRQ_VS_EXT
Once the vCPU is using a hardware VS-file (HSTATUS.VGEIN > 0), guest
accesses to CSR_VSTOPEI are handled by hardware without trapping, so
nothing clears IRQ_VS_EXT in CSR_HVIP and the guest vCPU is wedged in a
spurious external interrupt storm.
[1] https://sashiko.dev/#/bug/linux-0a58883c-0da3-49e7-847e-a71b508f0638
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261114135056.202556-1-anup.patel@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-11-14 14:08 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-14 13:50 [PATCH] RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock Anup Patel
2026-11-14 14:08 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox