Re: [PATCH v2 12/16] riscv: kvm: Add emulated test csr

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `vsscratch` test CSR emulation handler is structurally unreachable, rendering the feature dead code because accesses to VS-level CSRs from VS-mode do not trap to KVM.
- [High] The emulated test CSR unconditionally introduces a silently-on guest-visible feature and unmigratable state, violating KVM API invariants.
- [High] The patch hijacks the architectural `vsscratch` CSR, backing it with disconnected software state and causing split-brain register semantics.
- [Medium] The emulated test CSR state is not cleared during VCPU reset, leaking state across warm resets.
--

commit 783cfef4904ebb8469e4e71c63a55936a9c8e148
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: kvm: Add emulated test csr

This commit adds support to emulate the vsscratch CSR when
CONFIG_RISCV_KVM_TEST_CSR is set. It introduces a new kvm_test_csr
struct and hooks into the CSR instruction handling to reflect changes
when the vsscratch register is accessed.

> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 75b0a951c1bc6..4d30d30ebefc7 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
[ ... ]
> @@ -265,6 +271,10 @@ struct kvm_vcpu_arch {
>  		gpa_t shmem;
>  		u64 last_steal;
>  	} sta;
> +
> +#ifdef CONFIG_RISCV_KVM_TEST_CSR
> +	struct kvm_test_csr test_csr;
> +#endif
>  };

[Severity: High]
Does this new guest-visible state need to be explicitly enabled by userspace
with a runtime capability check rather than just a compile-time config? 

Also, since this state isn't exposed via the KVM_GET_ONE_REG or KVM_SET_ONE_REG
API, will it break live migration because it is completely invisible to
userspace?

[Severity: Medium]
Is this test_csr state cleared during a VCPU reset? It looks like it is missing
from the architecture-specific reset path kvm_riscv_vcpu_context_reset(),
which might allow the modified value to leak across warm resets.

> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> index 1d8741d022425..c5a70de4a5790 100644
> --- a/arch/riscv/kvm/vcpu_insn.c
> +++ b/arch/riscv/kvm/vcpu_insn.c
[ ... ]
> @@ -112,6 +114,9 @@ static int seed_csr_rmw(struct kvm_vcpu *vcpu, unsigned int csr_num,
>  static const struct csr_func csr_funcs[] = {
>  	KVM_RISCV_VCPU_AIA_CSR_FUNCS
>  	KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
> +#ifdef CONFIG_RISCV_KVM_TEST_CSR
> +	KVM_RISCV_VCPU_TEST_CSR_FUNCS
> +#endif
>  	{ .base = CSR_SEED, .count = 1, .func = seed_csr_rmw },
>  };

[Severity: High]
Will this CSR emulation handler ever be reached?

KVM's CSR emulation path kvm_riscv_vcpu_virtual_insn() is only invoked for
Virtual Instruction exceptions (EXC_VIRTUAL_INST_FAULT, cause 22). However,
accessing an HS-level CSR like vsscratch from VS-mode natively generates an
Illegal Instruction exception (EXC_INST_ILLEGAL, cause 2).

Since KVM explicitly delegates these exceptions directly to the guest's
VS-mode exception handler, it seems this handler gets bypassed entirely.

> diff --git a/arch/riscv/kvm/vcpu_test_csr.c b/arch/riscv/kvm/vcpu_test_csr.c
> new file mode 100644
> index 0000000000000..b8aa503cdaba7
> --- /dev/null
> +++ b/arch/riscv/kvm/vcpu_test_csr.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kvm_host.h>
> +#include <asm/kvm_vcpu_insn.h>
> +#include <asm/kvm_vcpu_test_csr.h>
> +
> +#define vcpu_to_test_csr(vcpu) (&(vcpu)->arch.test_csr)
> +
> +int kvm_riscv_vcpu_test_csr(struct kvm_vcpu *vcpu, unsigned int csr_num,
> +			    unsigned long *val, unsigned long new_val,
> +			    unsigned long wr_mask)
> +{
> +	struct kvm_test_csr *test_csr = vcpu_to_test_csr(vcpu);
> +
> +	*val = test_csr->val;
> +
> +	if (wr_mask)
> +		test_csr->val = (test_csr->val & ~wr_mask) | (new_val & wr_mask);
> +
> +	return KVM_INSN_CONTINUE_NEXT_SEPC;
> +}

[Severity: High]
Does using the architectural vsscratch CSR for this test cause a split-brain
state?

In a RISC-V guest, accessing sscratch implicitly accesses the hardware
vsscratch register, which KVM context-switches via its standard guest context
structures.

By intercepting explicit guest accesses to the vsscratch address and directing
them to this disjoint test_csr.val variable, the guest will see different
values depending on whether it accesses the register implicitly via sscratch
or explicitly via vsscratch. Would it be safer for a test CSR to use a
reserved or custom CSR address?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-riscv_insn_table-v2-0-7f0810ee2274@gmail.com?part=12