Re: [PATCH v4] RISC-V: KVM: Serialize virtual interrupt pending state updates

On Mon, Jul 13, 2026 at 1:04 PM Xie Bo <[email protected]> wrote:
>
> 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

s/clears irqs_pending/clear_bit(IRQ_VS_SOFT, 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
>
> Use non-atomic bitmap operations while holding the lock. Hold the lock
> across the AIA sync, flush, and pending checks as well, so both bitmap
> words share the same serialization domain.
>
> 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.
>
> Fixes: cce69aff689e ("RISC-V: KVM: Implement VCPU interrupts and requests handling")
> Cc: [email protected]
> Signed-off-by: Xie Bo <[email protected]>
> ---
> Changes in v4:
> - Split the AIA pending bitmap check from the IMSIC VS-file check.
> - Release irqs_pending_lock before the IMSIC check takes vsfile_lock,
>   avoiding sleep-in-atomic on PREEMPT_RT and an ABBA lock ordering cycle.
>
> Changes in v3:
> - Rebase onto Linux 7.2-rc3.
> - Use non-atomic bitmap operations under irqs_pending_lock.
> - Hold irqs_pending_lock across the AIA sync/flush helpers and add
>   lockdep assertions for their locking contract.
> - Protect kvm_riscv_vcpu_has_interrupts() with irqs_pending_lock.
>
> Changes in v2:
> - Expand the race description and user-visible failure mode.
>
>  arch/riscv/include/asm/kvm_aia.h  |  2 +
>  arch/riscv/include/asm/kvm_host.h | 10 ++---
>  arch/riscv/kvm/aia.c              | 45 +++++++++++++++-----
>  arch/riscv/kvm/vcpu.c             | 69 +++++++++++++++++++++----------
>  arch/riscv/kvm/vcpu_onereg.c      | 13 ++++--
>  5 files changed, 99 insertions(+), 40 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_aia.h b/arch/riscv/include/asm/kvm_aia.h
> index c67ec5ac0a1..1fe146675e2 100644
> --- a/arch/riscv/include/asm/kvm_aia.h
> +++ b/arch/riscv/include/asm/kvm_aia.h
> @@ -124,6 +124,8 @@ static inline void kvm_riscv_vcpu_aia_sync_interrupts(struct kvm_vcpu *vcpu)
>  {
>  }
>  #endif
> +bool kvm_riscv_vcpu_aia_has_pending_interrupts(struct kvm_vcpu *vcpu,
> +                                              u64 mask);
>  bool kvm_riscv_vcpu_aia_has_interrupts(struct kvm_vcpu *vcpu, u64 mask);

This split is looking very ugly and unmaintainable.

For now, let's not split kvm_riscv_vcpu_aia_has_interrupts() instead let
kvm_riscv_vcpu_aia_has_interrupts() explicity take irqs_pending_lock.

>
>  void kvm_riscv_vcpu_aia_update_hvip(struct kvm_vcpu *vcpu);
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 60017ceec9d..e2d5808169e 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -209,13 +209,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 bafb009c5ce..001e83032f6 100644
> --- a/arch/riscv/kvm/aia.c
> +++ b/arch/riscv/kvm/aia.c
> @@ -53,12 +53,15 @@ 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;
>
> +       lockdep_assert_held(&vcpu->arch.irqs_pending_lock);
> +
>         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;
> +       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;
> @@ -69,23 +72,35 @@ void kvm_riscv_vcpu_aia_sync_interrupts(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_vcpu_aia_csr *csr = &vcpu->arch.aia_context.guest_csr;
>
> +       lockdep_assert_held(&vcpu->arch.irqs_pending_lock);
> +
>         if (kvm_riscv_aia_available())
>                 csr->vsieh = ncsr_read(CSR_VSIEH);
>  }
>  #endif
>
> -bool kvm_riscv_vcpu_aia_has_interrupts(struct kvm_vcpu *vcpu, u64 mask)
> +bool kvm_riscv_vcpu_aia_has_pending_interrupts(struct kvm_vcpu *vcpu,
> +                                              u64 mask)
>  {
> -       unsigned long seip;
> +       lockdep_assert_held(&vcpu->arch.irqs_pending_lock);
>
>         if (!kvm_riscv_aia_available())
>                 return false;
>
>  #ifdef CONFIG_32BIT
> -       if (READ_ONCE(vcpu->arch.irqs_pending[1]) &
> +       if (vcpu->arch.irqs_pending[1] &
>             (vcpu->arch.aia_context.guest_csr.vsieh & upper_32_bits(mask)))
>                 return true;
>  #endif
> +       return false;
> +}
> +
> +bool kvm_riscv_vcpu_aia_has_interrupts(struct kvm_vcpu *vcpu, u64 mask)
> +{
> +       unsigned long seip;
> +
> +       if (!kvm_riscv_aia_available())
> +               return false;
>
>         seip = vcpu->arch.guest_csr.vsie;
>         seip &= (unsigned long)mask;
> @@ -207,6 +222,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;
> @@ -216,11 +234,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;

The hvip in guest context is not under critical section so don't bring hviph
under critical section.

> +                       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 cf6e231e76e..99c929ec0ac 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,10 +356,13 @@ 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;
> @@ -363,11 +370,13 @@ void kvm_riscv_vcpu_flush_interrupts(struct kvm_vcpu *vcpu)
>
>         /* Flush AIA high interrupts */
>         kvm_riscv_vcpu_aia_flush_interrupts(vcpu);
> +       raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>  }
>
>  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,27 +385,29 @@ 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);

Add newline before and after raw_spin_lock_irqsave() to make it readable.

>         if ((csr->hvip ^ hvip) & (1UL << IRQ_VS_SOFT)) {
>                 if (hvip & (1UL << IRQ_VS_SOFT)) {
> -                       if (!test_and_set_bit(IRQ_VS_SOFT,
> -                                             v->irqs_pending_mask))
> -                               set_bit(IRQ_VS_SOFT, v->irqs_pending);
> +                       if (!__test_and_set_bit(IRQ_VS_SOFT,
> +                                               v->irqs_pending_mask))
> +                               __set_bit(IRQ_VS_SOFT, v->irqs_pending);
>                 } else {
> -                       if (!test_and_set_bit(IRQ_VS_SOFT,
> -                                             v->irqs_pending_mask))
> -                               clear_bit(IRQ_VS_SOFT, v->irqs_pending);
> +                       if (!__test_and_set_bit(IRQ_VS_SOFT,
> +                                               v->irqs_pending_mask))
> +                               __clear_bit(IRQ_VS_SOFT, v->irqs_pending);
>                 }
>         }
>
>         /* Sync up the HVIP.LCOFIP bit changes (only clear) by the guest */
>         if ((csr->hvip ^ hvip) & (1UL << IRQ_PMU_OVF)) {
>                 if (!(hvip & (1UL << IRQ_PMU_OVF)) &&
> -                   !test_and_set_bit(IRQ_PMU_OVF, v->irqs_pending_mask))
> -                       clear_bit(IRQ_PMU_OVF, v->irqs_pending);
> +                   !__test_and_set_bit(IRQ_PMU_OVF, v->irqs_pending_mask))
> +                       __clear_bit(IRQ_PMU_OVF, v->irqs_pending);
>         }
>
>         /* Sync-up AIA high interrupts */
>         kvm_riscv_vcpu_aia_sync_interrupts(vcpu);

Add newline here before raw_spin_unlock_irqrestore().

> +       raw_spin_unlock_irqrestore(&v->irqs_pending_lock, flags);
>
>         /* Sync-up timer CSRs */
>         kvm_riscv_vcpu_timer_sync(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;
>
> -       set_bit(irq, vcpu->arch.irqs_pending);
> -       smp_mb__before_atomic();
> -       set_bit(irq, vcpu->arch.irqs_pending_mask);
> +       raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> +       __set_bit(irq, vcpu->arch.irqs_pending);
> +       __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,26 +455,37 @@ int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
>             irq != IRQ_PMU_OVF)
>                 return -EINVAL;
>
> -       clear_bit(irq, vcpu->arch.irqs_pending);
> -       smp_mb__before_atomic();
> -       set_bit(irq, vcpu->arch.irqs_pending_mask);
> +       raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
> +       __clear_bit(irq, vcpu->arch.irqs_pending);
> +       __set_bit(irq, vcpu->arch.irqs_pending_mask);
> +       raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>
>         return 0;
>  }
>
>  bool kvm_riscv_vcpu_has_interrupts(struct kvm_vcpu *vcpu, u64 mask)
>  {
> +       unsigned long flags;
>         unsigned long ie;
> +       bool ret;
>
> +       raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);
>         ie = ((vcpu->arch.guest_csr.vsie & VSIP_VALID_MASK)
>                 << VSIP_TO_HVIP_SHIFT) & (unsigned long)mask;
>         ie |= vcpu->arch.guest_csr.vsie & ~IRQ_LOCAL_MASK &
>                 (unsigned long)mask;
> -       if (READ_ONCE(vcpu->arch.irqs_pending[0]) & ie)
> -               return true;
> +       ret = vcpu->arch.irqs_pending[0] & ie;
> +
> +       /* Check AIA high pending bitmap while holding irqs_pending_lock */
> +       if (!ret)
> +               ret = kvm_riscv_vcpu_aia_has_pending_interrupts(vcpu, mask);
> +       raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>
> -       /* Check AIA high interrupts */
> -       return kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
> +       /* IMSIC interrupt check takes vsfile_lock, which can sleep on RT. */
> +       if (!ret)
> +               ret = kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
> +
> +       return ret;
>  }
>
>  void __kvm_riscv_vcpu_power_off(struct kvm_vcpu *vcpu)
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index bb920e8923c..cba3682944b 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;

Same comment as above.

> +               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;
>  }
> --
> 2.54.0
>

Regards,
Anup

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