Re: [PATCH v2 1/1] 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] 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