[PATCH v2 0/1] RISC-V: KVM: Serialize virtual interrupt pending state updates

[PATCH v2 0/1] RISC-V: KVM: Serialize virtual interrupt pending state updates

[Xie Bo]

Hi,

This is v2 of the RISC-V KVM virtual interrupt synchronization fix.

The first version described the irqs_pending/irqs_pending_mask race, but
it did not provide enough context on:

  - the exact lost-interrupt interleaving
  - the user-visible failure mode
  - why the existing lockless protocol is replaced instead of being
    repaired with additional barriers
  - what was exercised during validation

This version updates only the commit message and keeps the code change
the same.

Problem summary
===============

The current code treats irqs_pending and irqs_pending_mask as a lockless
multiple-producer, single-consumer state machine.  In practice,
kvm_riscv_vcpu_sync_interrupts() is not a pure consumer because it also
writes both bitmaps while synchronizing guest-visible HVIP state back
into KVM state.

That means the two bitmaps represent one logical state transition, but
they are updated independently.  A host-side interrupt injection can
race with guest-side VSSIP clearing and lose the newly injected pending
bit.

Observed symptom
================

The failure that motivated this patch was an occasional guest hang where
the guest remained blocked in WFI even though host-side virtual
interrupt injection and vCPU kick had already happened.

Why replace the lockless protocol
=================================

This is not just a missing barrier.  The bug comes from encoding one
shared state machine in two separate bitmaps while allowing both
producers and the sync path to modify them.

Repairing that while staying lockless would require redesigning the
state transition protocol itself.  A per-vCPU raw spinlock makes the
state transition explicit and keeps the fix small enough for stable
backporting.

Validation
==========

The issue was reproduced under RISC-V KVM stress workloads that exposed
guest stalls in WFI.  After applying this patch, the same stress setup
no longer reproduced the lost-VSSIP hang.

Thanks,
Xie Bo

Xie Bo (1):
  RISC-V: KVM: Serialize virtual interrupt pending state updates

 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(-)

-- 
2.54.0

[PATCH v2 1/1] RISC-V: KVM: Serialize virtual interrupt pending state updates

[Xie Bo]

RISC-V KVM tracks guest interrupt state with two bitmaps:

  - irqs_pending: interrupts that should be visible to the guest
  - irqs_pending_mask: interrupts whose pending state changed

The current code updates those bitmaps with independent atomic bitops
and assumes a multiple-producer, single-consumer protocol.  That model
does not actually hold.

kvm_riscv_vcpu_sync_interrupts() is not a pure consumer.  When the guest
changes guest-visible HVIP state, sync_interrupts() writes both
irqs_pending and irqs_pending_mask to reflect the new guest state back
into KVM state.  As a result, irqs_pending and irqs_pending_mask form a
single logical state transition, but they are not updated atomically as
a pair.

This allows a race where a newly injected interrupt is lost.  For
example:

  CPU0                                          CPU1
  ----                                          ----
  kvm_riscv_vcpu_set_interrupt(VS_SOFT)
    set_bit(VS_SOFT, irqs_pending)
                                                kvm_riscv_vcpu_sync_interrupts()
                                                  sees guest-cleared HVIP.VSSIP
                                                  sets irqs_pending_mask
                                                  clears irqs_pending
    set_bit(VS_SOFT, irqs_pending_mask)
    kvm_vcpu_kick()

After that interleaving, a later flush can update HVIP without VSSIP
even though a new virtual interrupt was injected.  In practice, the
guest can remain blocked in WFI with work pending.

The same pending/mask protocol is shared by VS soft interrupts, PMU
overflow delivery, and AIA high interrupt synchronization, so the race
is not limited to one interrupt source.

Fix this by serializing all updates to irqs_pending and
irqs_pending_mask with a per-vCPU raw spinlock.  This keeps the pending
bit and the dirty mask as one state transition across:

  - set/unset interrupt
  - guest HVIP sync
  - interrupt flush to guest CSR state
  - vCPU reset
  - AIA CSR writes that clear dirty state

This intentionally replaces the existing lockless protocol instead of
trying to repair it with additional barriers.  The problem is not memory
ordering on a single field; it is that two separate bitmaps encode one
shared state machine while both producers and sync paths can modify
them.  A per-vCPU raw spinlock keeps the fix small, local, and suitable
for backporting.

Observed symptom:

  - guest occasionally remains blocked in WFI after host-side virtual
    interrupt injection

Testing:

  - reproduced on a RISC-V KVM setup running lkvm stress workloads
  - observed guest stalls with WFI diagnostics while host-side kick and
    interrupt injection had already happened
  - stress run no longer reproduced the lost-VSSIP hang after applying
    this change

Fixes: cce69aff689e ("RISC-V: KVM: Implement VCPU interrupts and requests handling")
Cc: stable@vger.kernel.org
Signed-off-by: Xie Bo <xb@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

Re: [PATCH v2 1/1] RISC-V: KVM: Serialize virtual interrupt pending state updates

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Updating `sip` or `siph` via CSR ioctls fails to update the master `irqs_pending` bitmaps, which can lead to spurious wakeups or permanently blocked vCPUs in WFI.
- [High] Pre-existing bug: PMU overflow interrupt state (LCOFIP) is corrupted during userspace save/restore of the `sip` register.
--

--- Patch [1]: [PATCH v2 1/1] RISC-V: KVM: Serialize virtual interrupt pending state updates ---
commit 22c10234ade3ab313ea2d3d68ed12cccae2ba34a
Author: Xie Bo <xb@ultrarisc.com>

    RISC-V: KVM: Serialize virtual interrupt pending state updates

This patch introduces a per-vCPU raw spinlock to serialize updates to
virtual interrupt pending states. It replaces the existing lockless protocol
to prevent race conditions where newly injected interrupts could be lost
during guest HVIP sync.

> diff --git a/arch/riscv/kvm/aia.c b/arch/riscv/kvm/aia.c
> index 5ec503288555d..821d2cb6d6a6d 100644
> --- a/arch/riscv/kvm/aia.c
> +++ b/arch/riscv/kvm/aia.c
[ ... ]
> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but when restoring siph via ioctl, 
we clear irqs_pending_mask[1] without syncing irqs_pending[1] with the new val. 

Because kvm_arch_vcpu_runnable() relies exclusively on irqs_pending to
determine if a vCPU has pending events, could restoring a vCPU with
pending interrupts leave it permanently blocked in WFI since
irqs_pending isn't updated here?

> +		} else {
> +			((unsigned long *)csr)[reg_num] = val;
> +		}
> +#else
> +		((unsigned long *)csr)[reg_num] = val;
>  #endif
>  	}
>  
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index bb920e8923c93..cba3682944b69 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
[ ... ]
> @@ -309,10 +310,14 @@ static int kvm_riscv_vcpu_general_set_csr(struct kvm_vcpu *vcpu,
>  	if (reg_num == KVM_REG_RISCV_CSR_REG(sip)) {
>  		reg_val &= VSIP_VALID_MASK;
>  		reg_val <<= VSIP_TO_HVIP_SHIFT;
>  	}

[Severity: High]
This is a pre-existing issue, but does left-shifting the entire register
value here silently corrupt the PMU overflow interrupt state (LCOFIP)?

When userspace saves and restores the sip register, the IRQ_PMU_OVF bit
(bit 13) is shifted to bit 14 when saving to csr->hvip. Then on get_csr,
it shifts it back to bit 12, where it is subsequently cleared because
bit 12 is not included in VSIP_VALID_MASK. Will this break guest profiling
across migrations?

> -	((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);

[Severity: High]
This is also a pre-existing issue, but similar to the siph restore above,
we clear irqs_pending_mask[0] without updating irqs_pending[0] with the
new reg_val. 

Can this leave the vCPU permanently blocked in WFI after live migration
since the irqs_pending bitmap isn't populated with the restored interrupts?

> +	} else {
> +		((unsigned long *)csr)[reg_num] = reg_val;
> +	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178348173646.64306.17766618687631928432@ultrarisc.com?part=1