Re: [PATCH v2 11/16] riscv: kvm: Use generated instruction headers for mmio emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The migration to generated instruction headers missed the required '+8' offset for compressed instructions that use 3-bit register fields, causing KVM to access the wrong architectural registers.
- [High] Removal of `#ifdef CONFIG_64BIT` guards exposes 32-bit KVM hosts to Undefined Behavior (negative bit shift) when a guest triggers 64-bit instruction emulation.
--

commit 5795a15cde75d07e7edad3d6b52a93e9aa4d9d86
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: kvm: Use generated instruction headers for mmio emulation
    
Migrate the mmio emulation code to use the generated instruction headers
instead of the hand-written instruction composition functions.

> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> index 8ccf6ec722f03..1d8741d022425 100644
> --- a/arch/riscv/kvm/vcpu_insn.c
> +++ b/arch/riscv/kvm/vcpu_insn.c
[ ... ]
> @@ -408,44 +408,47 @@ int kvm_riscv_vcpu_mmio_load(struct kvm_vcpu *vcpu, struct kvm_run *run,
[ ... ]
> -#ifdef CONFIG_64BIT
> -	} else if ((insn & INSN_MASK_LD) == INSN_MATCH_LD) {
> +	} else if (riscv_insn_is_ld(insn)) {
>  		len = 8;
>  		shift = 8 * (sizeof(ulong) - len);
> -	} else if ((insn & INSN_MASK_LWU) == INSN_MATCH_LWU) {

[Severity: High]
By removing these CONFIG_64BIT guards, does this expose 32-bit KVM hosts
to undefined behavior?

If a guest triggers emulation of a 64-bit load, len becomes 8, and shift
is calculated as 8 * (sizeof(ulong) - len). On a 32-bit host where
sizeof(ulong) is 4, shift becomes -32.

This would result in a negative bit shift when kvm_riscv_vcpu_mmio_return()
later evaluates (long)data << shift >> shift.

> +		rd = riscv_insn_ld_extract_xd(insn);
> +	} else if (riscv_insn_is_lwu(insn)) {
>  		len = 4;
> -#endif
[ ... ]
> -	} else if ((insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW) {
> +	} else if (riscv_insn_is_c_lw(insn)) {
>  		len = 4;
>  		shift = 8 * (sizeof(ulong) - len);
> -		insn = RVC_RS2S(insn) << SH_RD;
> -	} else if ((insn & INSN_MASK_C_LWSP) == INSN_MATCH_C_LWSP &&
> -		   ((insn >> SH_RD) & 0x1f)) {
> +		rd = riscv_insn_c_lw_extract_xd(insn);

[Severity: High]
Does this code read or write to the wrong guest register?

Compressed instructions like c.lw and c.sw use 3-bit register fields
corresponding to general purpose registers x8 through x15. The generated
extraction macros (like riscv_insn_c_lw_extract_xd) return the raw 0-7
value.

Without an explicit +8 offset mapping, won't KVM incorrectly use registers
x0-x7 as the index into the guest_context array?

> +	} else if (riscv_insn_is_c_lwsp(insn)) {
>  		len = 4;
>  		shift = 8 * (sizeof(ulong) - len);
> +		rd = riscv_insn_c_lwsp_extract_xd(insn);
>  	} else {
>  		return -EOPNOTSUPP;
>  	}

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