Re: [PATCH] RISC-V: KVM: Fix lost virtual interrupts during IRQ sync

On Tue, Jun 16, 2026 at 11:48 AM Xie Bo <[email protected]> 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: [email protected]
> Signed-off-by: Xie Bo <[email protected]>
> ---
>  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

-- 
kvm-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kvm-riscv