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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Acquiring a sleepable rwlock (`imsic->vsfile_lock`) while holding a raw spinlock (`vcpu->arch.irqs_pending_lock`) causes a PREEMPT_RT sleep-in-atomic BUG, PROVE_RAW_LOCK_NESTING splats, and introduces an ABBA deadlock.

Pre-existing issues:
- [High] Desynchronization between `irqs_pending` and `hvip` when user-space writes the `sip` CSR, potentially causing infinite WFI wakeup loops.
--

--- Patch [1]: [PATCH v3] RISC-V: KVM: Serialize virtual interrupt pending state updates ---
commit 84ae8e6240f7f5635c2560729711fc1ceb24234d
Author: Xie Bo <xb@ultrarisc.com>

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

This commit replaces the lockless atomic operations for the irqs_pending
and irqs_pending_mask bitmaps with a per-vCPU raw spinlock. This groups
the updates into a single state transition to avoid losing interrupts.

> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index cf6e231e76e25..7d8d20839d46e 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
[ ... ]
> @@ -455,26 +469,33 @@ int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq)
[ ... ]
>  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 interrupts */
> -	return kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
> +	if (!ret)
> +		ret = kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
> +	raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
> +
> +	return ret;
>  }

[Severity: High]
Does holding the raw irqs_pending_lock across kvm_riscv_vcpu_aia_has_interrupts()
cause a lock nesting issue and potential ABBA deadlock?

If kvm_riscv_vcpu_aia_has_interrupts() falls through to
kvm_riscv_vcpu_aia_imsic_has_interrupt(), that function acquires
imsic->vsfile_lock (an rwlock_t, which is sleepable on PREEMPT_RT).
Acquiring a sleepable lock while holding a raw spinlock triggers a
sleep-in-atomic bug and PROVE_RAW_LOCK_NESTING warnings.

Additionally, can this sequence deadlock against concurrent userspace
ioctl calls?

For example, a KVM_SET_DEVICE_ATTR call on the IMSIC triggers
kvm_riscv_aia_imsic_rw_attr(), which acquires imsic->vsfile_lock first:

kvm_riscv_aia_imsic_rw_attr()
  read_lock_irqsave(&imsic->vsfile_lock, flags);
  ...
  imsic_swfile_extirq_update(vcpu);
    kvm_riscv_vcpu_set_interrupt();
      raw_spin_lock_irqsave(&vcpu->arch.irqs_pending_lock, flags);

If a vCPU thread running kvm_riscv_vcpu_has_interrupts() takes
irqs_pending_lock first, wouldn't these paths deadlock?

> 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
[ ... ]
> @@ -298,6 +298,7 @@ static int kvm_riscv_vcpu_general_set_csr(struct kvm_vcpu *vcpu,
[ ... ]
> -	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;
> +	}

[Severity: High]
This is a pre-existing issue, but does this clear of irqs_pending_mask leave
stale bits in irqs_pending?

If userspace writes 0 to the sip CSR, this code clears irqs_pending_mask[0]
and updates csr->hvip, but it does not clear any previously injected
interrupts in irqs_pending[0]. 

Because kvm_riscv_vcpu_has_interrupts() checks irqs_pending directly, can
this cause the vCPU to endlessly wake from WFI, finding no hardware interrupt
in HVIP, and immediately sleeping again?

The same situation appears to exist for the siph CSR update in
kvm_riscv_vcpu_aia_set_csr().

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