Re: [PATCH v3 4/4] RISC-V: KVM: Mark the reset callback of FWFT extension always dirties CSR

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The addition of `vcpu->arch.csr_dirty = true` in `kvm_sbi_ext_fwft_reset()` is entirely redundant, contradicting the commit message premise.

Pre-existing issues:
- [High] Missing `vcpu_load()` around `KVM_SET_ONE_REG` for timer state causes physical CPU hardware CSR corruption.
--

--- Patch [4]: [PATCH v3 4/4] RISC-V: KVM: Mark the reset callback of FWFT extension always dirties CSR ---
commit 316b183f1319c7935d993137c0fbff524d4ecc07
Author: Inochi Amaoto <inochiama@gmail.com>

RISC-V: KVM: Mark the reset callback of FWFT extension always dirties CSR

As the CSR is only flushed when the csr_dirty is set, always set the
csr_dirty field in kvm_sbi_ext_fwft_reset() so the CSR change can take
effect immediately.

> diff --git a/arch/riscv/kvm/vcpu_sbi_fwft.c b/arch/riscv/kvm/vcpu_sbi_fwft.c
> index 2794e9e5b495e..8748df921f101 100644
> --- a/arch/riscv/kvm/vcpu_sbi_fwft.c
> +++ b/arch/riscv/kvm/vcpu_sbi_fwft.c
> @@ -420,6 +420,8 @@ static void kvm_sbi_ext_fwft_reset(struct kvm_vcpu *vcpu)
>  		if (feature->reset)
>  			feature->reset(vcpu);
>  	}
> +
> +	vcpu->arch.csr_dirty = true;

[Severity: Low]
Is this assignment actually needed?

The commit message states csr_dirty = true is added so the CSR change takes
effect immediately. However, kvm_sbi_ext_fwft_reset() is executed exclusively
via the .reset callback in two paths: riscv_vcpu_set_sbi_ext_single() (which
returns -EBUSY if the vCPU has already run) and kvm_riscv_vcpu_sbi_reset()
(called immediately after last_exit_cpu is explicitly set to -1).

In both scenarios, vcpu->arch.last_exit_cpu is guaranteed to be -1.
When kvm_arch_vcpu_load() later evaluates the fast-path condition:

arch/riscv/kvm/vcpu.c:kvm_arch_vcpu_load() {
    ...
	else if (vcpu->arch.last_exit_cpu == cpu && !vcpu->arch.csr_dirty)
		goto csr_restore_done;
    ...
}

It will always evaluate to false (since physical cpu >= 0), meaning KVM
unconditionally bypasses the skip and flushes the CSRs anyway. Does this make
the csr_dirty assignment redundant?

>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential hardware CSR
corruption when setting the timer state via ioctl.

Unlike x86, s390, MIPS, and LoongArch, the RISC-V implementation of
kvm_arch_vcpu_ioctl() handles KVM_SET_ONE_REG without acquiring
vcpu_load(vcpu):

arch/riscv/kvm/vcpu.c:kvm_arch_vcpu_ioctl() {
    ...
	if (ioctl == KVM_SET_ONE_REG)
		r = kvm_riscv_vcpu_set_reg(vcpu, &reg);
    ...
}

When an unprivileged userspace process sets the timer state (e.g.,
KVM_REG_RISCV_TIMER_REG(state) to KVM_RISCV_TIMER_STATE_ON) on a system with
the SSTC extension, it eventually invokes kvm_riscv_vcpu_update_vstimecmp(),
which executes a direct hardware write:

arch/riscv/kvm/vcpu_timer.c:kvm_riscv_vcpu_update_vstimecmp() {
    ...
	ncsr_write(CSR_VSTIMECMP, ncycles);
    ...
}

Because the vCPU is not loaded onto the host CPU and preempt notifiers are
not active, does this directly corrupt the physical CPU's VSTIMECMP hardware
register?

If this destroys the -1UL safeguard, could it result in spurious guest timer
interrupts on the host, degraded power-savings (where WFI becomes a NOP), or
corruption of another running vCPU's timer state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729232527.139183-1-inochiama@gmail.com?part=4