Re: [PATCH v3 14/17] KVM: riscv: selftests: Add csr emulation test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unaligned trap vector in `check_test_csr_guest` leads to an infinite exception loop.
- [Medium] Missing check for the RISC-V F (Floating-Point) extension causes spurious test crashes on non-F systems.
--

--- Patch [14]: [PATCH v3 14/17] KVM: riscv: selftests: Add csr emulation test ---
commit de141f953203428fe244492005f127de5ea224f3
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

KVM: riscv: selftests: Add csr emulation test

Introduce a kvm test that uses the emulated test csr to validate that
all emulated reads/writes to csrs function as expected.

> diff --git a/tools/testing/selftests/kvm/riscv/csr_test.c b/tools/testing/selftests/kvm/riscv/csr_test.c
> new file mode 100644
> index 0000000000000..bdedf7c84001c
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/riscv/csr_test.c
> @@ -0,0 +1,133 @@
[ ... ]
> +/*
> + * Use the fcsr as a U-mode accesible csr and compare against the custom 'test'
> + * hypervisor csr (currently using vsscratch)
> + */
> +#define test_csr(write, initial, mode)								\
> +static void test_##write(void)									\
> +{												\
> +	unsigned long hypervisor_result, reference_result, old_hypervisor;			\
> +	asm volatile (										\
> +		"csrs	sstatus, %[enable_fp]\n"						\
> +		"csrw	fcsr, %[init]\n"							\
> +		#write"	zero, fcsr, %[mask]\n"							\
> +		"csrr	%[ref_res], fcsr\n"							\

[Severity: Medium]
Will this test crash on systems lacking the RISC-V F (Floating-Point)
extension?

The test unconditionally writes to and reads from fcsr in test_csr(). On
hosts without the F extension, accessing fcsr will generate an illegal
instruction trap. Since this trap isn't caught, the selftest exception
handler will terminate the test rather than skipping gracefully.

Should main() verify KVM_RISCV_ISA_EXT_F is available before executing
the tests that depend on it?

[ ... ]
> +static void check_test_csr_guest(void)
> +{
> +	unsigned long scause, stvec;
> +
> +	asm volatile(
> +		".option push\n"
> +		".option norvc\n"
> +		"la	%[stvec], 1f\n"
> +		"csrw	stvec, %[stvec]\n"
> +		"csrwi	%[test_csr], 0x0\n"
> +		"1:\n"

[Severity: High]
Could this lead to an infinite exception loop due to an unaligned trap
vector?

The label 1: is used as the exception vector in check_test_csr_guest(),
but it lacks a .align 2 or .balign 4 directive.

The stvec register requires 4-byte alignment. If the compiler aligns the
inline assembly block such that 1: lands on a 2-byte boundary, writing
to stvec will mask the lower bits. When the csrwi instruction traps, the
CPU will jump to the masked address, which could land in the middle of
the instruction.

This immediately triggers another illegal instruction trap, leading to a
soft lockup and hanging the test suite.

Can a .balign 4 be added before the label to ensure proper alignment?

> +		"csrr	%[scause], scause\n"
> +		".option pop\n"
> +		: [scause] "=&r" (scause), [stvec] "=&r" (stvec)
> +		: [test_csr] "i" (CSR_TEST)
> +	);

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