[PATCH] RISC-V: KVM: Fix lost virtual interrupts during IRQ sync
Anup Patel
anup at brainfault.org
Sun Jul 12 21:43:42 PDT 2026
On Tue, Jun 16, 2026 at 11:48 AM Xie Bo <xb at ultrarisc.com> wrote:
>
> RISC-V KVM tracks guest pending interrupts with irqs_pending and
> irqs_pending_mask. The existing code uses atomic bitops and models the
> state as a multiple-producer, single-consumer queue where the vCPU is
> the consumer.
>
> The atomic bitops make each individual bitmap operation atomic, but
> they do not make the pair of irqs_pending and irqs_pending_mask an
> atomic state transition. The vCPU interrupt sync path is also not a
> pure consumer: it writes both bitmaps when it reflects guest-visible
> HVIP changes back into KVM state.
>
> For example, kvm_riscv_vcpu_set_interrupt() can set IRQ_VS_SOFT in
> irqs_pending while the target vCPU is concurrently syncing a
> guest-cleared HVIP.VSSIP bit. If sync observes irqs_pending_mask before
> the producer sets it, sync can set the mask and clear irqs_pending. The
> producer then sets the mask and kicks the vCPU, but the pending bit has
> already been lost. A subsequent flush updates HVIP without VSSIP, and
> the guest can remain blocked in WFI even though it has work queued.
>
> Serialize all updates to irqs_pending and irqs_pending_mask with a
> per-vCPU raw spinlock. This intentionally replaces the lockless
> pending/mask protocol with one small critical section per vCPU, so the
> pending bit and the dirty mask are updated as one state transition.
> Use the same lock for sync, flush, reset, and userspace CSR writes that
> clear the dirty mask, so a newly injected interrupt cannot be
> overwritten by a concurrent HVIP sync.
Overall, I agree with the race condition and proposed solution. The
race-condition is primarily because for VSSIP the VCPU acts as both
producer and consumer which breaks the lockless multi-producer
single-consumer approach.
I have the following suggestions:
1) Now that we have a spinlock protecting irqs_pending* bitmaps,
it is better to do non-atomic bitmap operations on these bitmaps
2) To minimize the lock/unlock dance, various kvm_riscv_vcpu_aia_xyz()
functions must be called with irqs_pending_lock held
3) The kvm_riscv_vcpu_has_interrupts() function must also be updated
to use the irqs_pending_lock
>
> Fixes: cce69aff689e ("RISC-V: KVM: Implement VCPU interrupts and requests handling")
> Cc: stable at vger.kernel.org
> Signed-off-by: Xie Bo <xb at ultrarisc.com>
> ---
> arch/riscv/include/asm/kvm_host.h | 10 +++++-----
> arch/riscv/kvm/aia.c | 28 +++++++++++++++++++++-------
> arch/riscv/kvm/vcpu.c | 27 ++++++++++++++++++++++-----
> arch/riscv/kvm/vcpu_onereg.c | 13 +++++++++----
> 4 files changed, 57 insertions(+), 21 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 75b0a951c..97e42645c 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -207,13 +207,13 @@ struct kvm_vcpu_arch {
> /*
> * VCPU interrupts
> *
> - * We have a lockless approach for tracking pending VCPU interrupts
> - * implemented using atomic bitops. The irqs_pending bitmap represent
> - * pending interrupts whereas irqs_pending_mask represent bits changed
> - * in irqs_pending. Our approach is modeled around multiple producer
> - * and single consumer problem where the consumer is the VCPU itself.
> + * The irqs_pending bitmap represents pending interrupts whereas
> + * irqs_pending_mask represents bits changed in irqs_pending. Updates
> + * to these bitmaps are serialized so vcpu interrupt sync/flush cannot
> + * drop a newly injected interrupt while syncing guest-visible HVIP.
> */
> #define KVM_RISCV_VCPU_NR_IRQS 64
> + raw_spinlock_t irqs_pending_lock;
> DECLARE_BITMAP(irqs_pending, KVM_RISCV_VCPU_NR_IRQS);
> DECLARE_BITMAP(irqs_pending_mask, KVM_RISCV_VCPU_NR_IRQS);
>
> diff --git a/arch/riscv/kvm/aia.c b/arch/riscv/kvm/aia.c
> index 5ec503288..821d2cb6d 100644
> --- a/arch/riscv/kvm/aia.c
> +++ b/arch/riscv/kvm/aia.c
> @@ -50,17 +50,21 @@ void kvm_riscv_vcpu_aia_flush_interrupts(struct kvm_vcpu *vcpu)
> {
> struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
> unsigned long mask, val;
> + unsigned long flags;
>
> if (!kvm_riscv_aia_available())
> return;
>
> - if (READ_ONCE(vcpu->arch.irqs_pending_mask[1])) {
> - mask = xchg_acquire(&vcpu->arch.irqs_pending_mask[1], 0);
> - val = READ_ONCE(vcpu->arch.irqs_pending[1]) & mask;
> + raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> + mask = vcpu->arch.irqs_pending_mask[1];
> + if (mask) {
> + vcpu->arch.irqs_pending_mask[1] = 0;
> + val = vcpu->arch.irqs_pending[1] & mask;
>
> csr->hviph &= ~mask;
> csr->hviph |= val;
> }
> + raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
> }
>
> void kvm_riscv_vcpu_aia_sync_interrupts(struct kvm_vcpu *vcpu)
> @@ -205,6 +209,9 @@ int kvm_riscv_vcpu_aia_set_csr(struct kvm_vcpu *vcpu,
> {
> struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
> unsigned long regs_max = sizeof(struct kvm_riscv_aia_csr) / sizeof(unsigned long);
> +#ifdef CONFIG_32BIT
> + unsigned long flags;
> +#endif
>
> if (!riscv_isa_extension_available(vcpu->arch.isa, SSAIA))
> return -ENOENT;
> @@ -214,11 +221,18 @@ int kvm_riscv_vcpu_aia_set_csr(struct kvm_vcpu *vcpu,
> reg_num = array_index_nospec(reg_num, regs_max);
>
> if (kvm_riscv_aia_available()) {
> - ((unsigned long *)csr)[reg_num] = val;
> -
> #ifdef CONFIG_32BIT
> - if (reg_num == KVM_REG_RISCV_CSR_AIA_REG(siph))
> - WRITE_ONCE(vcpu->arch.irqs_pending_mask[1], 0);
> + if (reg_num == KVM_REG_RISCV_CSR_AIA_REG(siph)) {
> + raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> + ((unsigned long *)csr)[reg_num] = val;
> + vcpu->arch.irqs_pending_mask[1] = 0;
> + raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock,
> + flags);
> + } else {
> + ((unsigned long *)csr)[reg_num] = val;
> + }
> +#else
> + ((unsigned long *)csr)[reg_num] = val;
> #endif
> }
>
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index a73690eda..b04730ccd 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -80,6 +80,7 @@ static void kvm_riscv_vcpu_context_reset(struct kvm_vcpu *vcpu,
>
> static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu, bool kvm_sbi_reset)
> {
> + unsigned long flags;
> bool loaded;
>
> /**
> @@ -104,8 +105,10 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu, bool kvm_sbi_reset)
>
> kvm_riscv_vcpu_aia_reset(vcpu);
>
> + raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> bitmap_zero(vcpu->arch.irqs_pending, KVM_RISCV_VCPU_NR_IRQS);
> bitmap_zero(vcpu->arch.irqs_pending_mask, KVM_RISCV_VCPU_NR_IRQS);
> + raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>
> kvm_riscv_vcpu_pmu_reset(vcpu);
>
> @@ -151,6 +154,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>
> /* Setup VCPU hfence queue */
> spin_lock_init(&vcpu->arch.hfence_lock);
> + raw_spin_lock_init(&vcpu->arch.irqs_pending_lock);
>
> spin_lock_init(&vcpu->arch.reset_state.lock);
>
> @@ -352,14 +356,18 @@ void kvm_riscv_vcpu_flush_interrupts(struct kvm_vcpu *vcpu)
> {
> struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
> unsigned long mask, val;
> + unsigned long flags;
>
> - if (READ_ONCE(vcpu->arch.irqs_pending_mask[0])) {
> - mask = xchg_acquire(&vcpu->arch.irqs_pending_mask[0], 0);
> - val = READ_ONCE(vcpu->arch.irqs_pending[0]) & mask;
> + raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> + mask = vcpu->arch.irqs_pending_mask[0];
> + if (mask) {
> + vcpu->arch.irqs_pending_mask[0] = 0;
> + val = vcpu->arch.irqs_pending[0] & mask;
>
> csr->hvip &= ~mask;
> csr->hvip |= val;
> }
> + raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>
> /* Flush AIA high interrupts */
> kvm_riscv_vcpu_aia_flush_interrupts(vcpu);
> @@ -368,6 +376,7 @@ void kvm_riscv_vcpu_flush_interrupts(struct kvm_vcpu *vcpu)
> void kvm_riscv_vcpu_sync_interrupts(struct kvm_vcpu *vcpu)
> {
> unsigned long hvip;
> + unsigned long flags;
> struct kvm_vcpu_arch *v = &vcpu->arch;
> struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>
> @@ -376,6 +385,7 @@ void kvm_riscv_vcpu_sync_interrupts(struct kvm_vcpu *vcpu)
>
> /* Sync-up HVIP.VSSIP bit changes does by Guest */
> hvip = ncsr_read(CSR_HVIP);
> + raw_spin_lock_irqsave(&v->irqs_pending_lock, flags);
> if ((csr->hvip ^ hvip) & (1UL << IRQ_VS_SOFT)) {
> if (hvip & (1UL << IRQ_VS_SOFT)) {
> if (!test_and_set_bit(IRQ_VS_SOFT,
> @@ -394,6 +404,7 @@ void kvm_riscv_vcpu_sync_interrupts(struct kvm_vcpu *vcpu)
> !test_and_set_bit(IRQ_PMU_OVF, v->irqs_pending_mask))
> clear_bit(IRQ_PMU_OVF, v->irqs_pending);
> }
> + raw_spin_unlock_irqrestore(&v->irqs_pending_lock, flags);
>
> /* Sync-up AIA high interrupts */
> kvm_riscv_vcpu_aia_sync_interrupts(vcpu);
> @@ -404,6 +415,8 @@ void kvm_riscv_vcpu_sync_interrupts(struct kvm_vcpu *vcpu)
>
> int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
> {
> + unsigned long flags;
> +
> /*
> * We only allow VS-mode software, timer, and external
> * interrupts when irq is one of the local interrupts
> @@ -416,9 +429,10 @@ int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
> irq != IRQ_PMU_OVF)
> return -EINVAL;
>
> + raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> set_bit(irq, vcpu->arch.irqs_pending);
> - smp_mb__before_atomic();
> set_bit(irq, vcpu->arch.irqs_pending_mask);
> + raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>
> kvm_vcpu_kick(vcpu);
>
> @@ -427,6 +441,8 @@ int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
>
> int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
> {
> + unsigned long flags;
> +
> /*
> * We only allow VS-mode software, timer, counter overflow and external
> * interrupts when irq is one of the local interrupts
> @@ -439,9 +455,10 @@ int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
> irq != IRQ_PMU_OVF)
> return -EINVAL;
>
> + raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> clear_bit(irq, vcpu->arch.irqs_pending);
> - smp_mb__before_atomic();
> set_bit(irq, vcpu->arch.irqs_pending_mask);
> + raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>
> return 0;
> }
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index bb920e892..cba368294 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
> @@ -298,6 +298,7 @@ static int kvm_riscv_vcpu_general_set_csr(struct kvm_vcpu *vcpu,
> {
> struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
> unsigned long regs_max = sizeof(struct kvm_riscv_csr) / sizeof(unsigned long);
> + unsigned long flags;
>
> if (reg_num >= regs_max)
> return -ENOENT;
> @@ -309,10 +310,14 @@ static int kvm_riscv_vcpu_general_set_csr(struct kvm_vcpu *vcpu,
> reg_val <<= VSIP_TO_HVIP_SHIFT;
> }
>
> - ((unsigned long *)csr)[reg_num] = reg_val;
> -
> - if (reg_num == KVM_REG_RISCV_CSR_REG(sip))
> - WRITE_ONCE(vcpu->arch.irqs_pending_mask[0], 0);
> + if (reg_num == KVM_REG_RISCV_CSR_REG(sip)) {
> + raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> + ((unsigned long *)csr)[reg_num] = reg_val;
> + vcpu->arch.irqs_pending_mask[0] = 0;
> + raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
> + } else {
> + ((unsigned long *)csr)[reg_num] = reg_val;
> + }
>
> return 0;
> }
>
> base-commit: 481329ec5b31d2c48ac6b8b703c8008133884c7e
> --
> 2.54.0
>
Regards,
Anup
More information about the kvm-riscv
mailing list