Re: [PATCH v3 13/17] riscv: kvm: Add emulated test csr

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hijacking the architectural `CSR_VSSCRATCH` for test emulation creates dual disjoint states, violating architecture invariants and breaking nested virtualization.
- [Medium] The KVM CSR emulation test for `vsscratch` is non-functional and unreachable. Accessing an HS-mode CSR (0x240) from a guest (VS-mode) generates an Illegal Instruction exception (Cause 2), not a Virtual Instruction fault, completely bypassing KVM's emulation logic.
- [Medium] The emulated vsscratch CSR is a silently-on guest-visible feature that lacks state serialization, violating KVM live migration invariants.
--

--- Patch [13]: [PATCH v3 13/17] riscv: kvm: Add emulated test csr ---
commit c90a2625de316ca2ae3dfca0aa91c4a00b0acfef
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: kvm: Add emulated test csr

This commit introduces an emulated test CSR (vsscratch) for KVM on RISC-V.
It adds a new kernel config option CONFIG_RISCV_KVM_TEST_CSR to enable
interception and emulation of the vsscratch register for testing purposes.

> 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: Medium]
Since this newly added guest state is not exposed to userspace via the
KVM_GET_ONE_REG or KVM_SET_ONE_REG ioctls, will it be silently lost during
live migration?

> diff --git a/arch/riscv/include/asm/kvm_vcpu_test_csr.h b/arch/riscv/include/asm/kvm_vcpu_test_csr.h
> new file mode 100644
> index 0000000000000..a844fccaafc34
> --- /dev/null
> +++ b/arch/riscv/include/asm/kvm_vcpu_test_csr.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __KVM_VCPU_RISCV_TEST_CSR_H
> +#define __KVM_VCPU_RISCV_TEST_CSR_H
> +
> +#include <asm/kvm_vcpu_insn.h>
> +
> +#define KVM_RISCV_VCPU_TEST_CSR_FUNCS \
> +	{.base = CSR_VSSCRATCH,	.count = 1,	.func = kvm_riscv_vcpu_test_csr },

[Severity: Medium]
Will this emulation callback ever be reached?

CSR_VSSCRATCH (0x240) is an HS-mode CSR. When a guest in VS-mode attempts
to access it, the hardware generates an Illegal Instruction exception
(Cause 2) rather than a Virtual Instruction fault (Cause 22).

In kvm_riscv_vcpu_exit(), KVM directly injects EXC_INST_ILLEGAL back to the
guest:

arch/riscv/kvm/vcpu_exit.c:kvm_riscv_vcpu_exit() {
    ...
    case EXC_INST_ILLEGAL:
        kvm_riscv_vcpu_pmu_incr_fw(vcpu, SBI_PMU_FW_ILLEGAL_INSN);
        vcpu->stat.instr_illegal_exits++;
        ret = vcpu_redirect(vcpu, trap);
        break;
    ...
}

Because this bypasses the instruction decoder entirely, it seems the test
CSR emulation logic won't be executed.

> +
> +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);
> +
> +#endif /* !__KVM_VCPU_RISCV_TEST_CSR_H */
> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> index 78adc28308193..c3e4cf2cd44b7 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

[Severity: Medium]
Does unconditionally enabling this guest-visible feature for all VMs when the
kernel config is set bypass KVM capability negotiation?

KVM features should generally be discoverable and opt-in by userspace to
ensure predictable migration behavior and compatibility.

>  	{ .base = CSR_SEED, .count = 1, .func = seed_csr_rmw },
>  };
>  
> 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);

[Severity: High]
Does hijacking the architectural CSR_VSSCRATCH create disjoint states that
could corrupt nested virtualization?

Even if the emulation were reachable, it reads and writes to a dummy field
rather than the actual hardware state in vcpu->arch.guest_csr.vsscratch.

KVM natively context-switches the architectural CSR to hardware:

arch/riscv/kvm/vcpu.c:kvm_arch_vcpu_load() {
    ...
    csr_write(CSR_VSSCRATCH, csr->vsscratch);
    ...
}

Redirecting explicit accesses to this separate test variable decouples the
software-emulated state from the hardware state, which might break nested
hypervisors that rely on the architectural vsscratch holding the L2 guest's
sscratch.

> +
> +	return KVM_INSN_CONTINUE_NEXT_SEPC;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-riscv_insn_table-v3-0-3f8e5f75ba97@gmail.com?part=13