Re: [PATCH v5] 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:

Pre-existing issues:
- [High] PMU overflow interrupts (`IRQ_PMU_OVF`) are never detected as pending in `kvm_riscv_vcpu_has_interrupts`, preventing the vCPU from waking up if it is the only pending interrupt.
- [High] KVM_SET_ONE_REG for CSR_SIP/CSR_HVIP and AIA SIPH zeroes `irqs_pending_mask` but fails to update `irqs_pending`, leading to stale interrupt tracking state and broken migration.
--

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

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

RISC-V KVM tracks guest interrupt state with two bitmaps:
irqs_pending and irqs_pending_mask. The current code updates those
bitmaps with independent atomic bitops and assumes a multiple-producer,
single-consumer protocol, which does not hold. This commit serializes
updates with a per-vCPU raw spinlock.

> diff --git a/arch/riscv/kvm/aia.c b/arch/riscv/kvm/aia.c
> index bafb009c5ce55..9a653b4ad40a5 100644
> --- a/arch/riscv/kvm/aia.c
> +++ b/arch/riscv/kvm/aia.c

[ ... ]

> @@ -219,8 +236,12 @@ int kvm_riscv_vcpu_aia_set_csr(struct kvm_vcpu *vcpu,
>  		((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);
> +			vcpu->arch.irqs_pending_mask[1] = 0;

[Severity: High]
This is a pre-existing issue, but does kvm_riscv_vcpu_aia_set_csr() in
arch/riscv/kvm/aia.c also need to synchronize vcpu->arch.irqs_pending[1]?

When KVM_SET_ONE_REG is used for AIA SIPH, this zeroes irqs_pending_mask[1]
but fails to update irqs_pending[1].

During a VM restore, if a vCPU is migrated with pending interrupts,
could it spuriously block in WFI forever? kvm_riscv_vcpu_has_interrupts()
relies exclusively on irqs_pending to evaluate pending interrupts.

> +			raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock,
> +						   flags);
> +		}
>  #endif
>  	}

[ ... ]

> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index cf6e231e76e25..0065a15c9aadc 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c

[ ... ]

> @@ -439,26 +458,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;

[Severity: High]
This is a pre-existing issue, but are PMU overflow interrupts (IRQ_PMU_OVF)
always undetected in kvm_riscv_vcpu_has_interrupts() in arch/riscv/kvm/vcpu.c?

The `ie` mask shifts the PMU enable bit to position 14 (due to
VSIP_TO_HVIP_SHIFT), while `irqs_pending[0]` has the PMU interrupt at
position 13.

Since their bitwise AND always evaluates to zero for the PMU interrupt,
does this prevent the vCPU from waking up if a guest configures a PMU
overflow interrupt and executes WFI?

> +	raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
>  
>  	/* Check AIA high interrupts */
> -	return kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
> +	if (!ret)
> +		ret = kvm_riscv_vcpu_aia_has_interrupts(vcpu, mask);
> +
> +	return ret;
>  }

[ ... ]

> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index bb920e8923c93..2031beb9bba6e 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c

[ ... ]

> @@ -311,8 +312,11 @@ static int kvm_riscv_vcpu_general_set_csr(struct kvm_vcpu *vcpu,
>  
>  	((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);
> +		vcpu->arch.irqs_pending_mask[0] = 0;

[Severity: High]
This is a pre-existing issue, but does kvm_riscv_vcpu_general_set_csr() in
arch/riscv/kvm/vcpu_onereg.c also need to synchronize
vcpu->arch.irqs_pending[0]?

When KVM_SET_ONE_REG is used for CSR_SIP/CSR_HVIP, this zeroes
irqs_pending_mask[0] to prevent clobbering the explicitly restored state,
but fails to update irqs_pending[0] with the new sip/hvip value.

If an interrupt is cleared via KVM_SET_ONE_REG, does it leave a stale bit
in irqs_pending[0] and prevent the vCPU from sleeping?

> +		raw_spin_unlock_irqrestore(&vcpu->arch.irqs_pending_lock, flags);
> +	}
>  
>  	return 0;
>  }

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