Re: [PATCH v3 15/17] riscv: kvm: Use generated instruction headers for csr emulation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] RVC (compressed) MMIO loads and stores read from and write to the wrong guest registers due to a missing architectural offset calculation.
- [Low] The CSR emulation code uses the `csrrwi` immediate extractor macro for a `csrrsi` instruction.
--

--- Patch [15]: [PATCH v3 15/17] riscv: kvm: Use generated instruction headers for csr emulation ---
commit 1deda6264dba206b7c5ef34e0d344daa8c535ddc
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: kvm: Use generated instruction headers for csr emulation

This commit migrates the CSR 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 c3e4cf2cd44b7..51eb2c717bff3 100644
> --- a/arch/riscv/kvm/vcpu_insn.c
> +++ b/arch/riscv/kvm/vcpu_insn.c
[ ... ]
> @@ -166,26 +162,32 @@ static int csr_insn(struct kvm_vcpu *vcpu, struct kvm_run *run, ulong insn)
[ ... ]
>  	} else if (riscv_insn_is_csrrsi(insn)) {
>  		wr_mask = riscv_insn_csrrwi_extract_imm(insn);
>  		new_val = -1UL;
>  		csr_num = riscv_insn_csrrsi_extract_csr(insn);
> +		rd = riscv_insn_csrrsi_extract_xd(insn);

[Severity: Low]
This isn't a bug, but is riscv_insn_csrrwi_extract_imm() mistakenly used here
for a csrrsi instruction instead of riscv_insn_csrrsi_extract_imm()?

>  	} else if (riscv_insn_is_csrrci(insn)) {
[ ... ]


commit 8d80090faef41d1000ad8ee985dcdaa3df7c12d3
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: kvm: Use generated instruction headers for mmio emulation

This commit migrates 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 a1898ab8bbe85..78adc28308193 100644
> --- a/arch/riscv/kvm/vcpu_insn.c
> +++ b/arch/riscv/kvm/vcpu_insn.c
[ ... ]
> @@ -411,44 +411,47 @@ int kvm_riscv_vcpu_mmio_load(struct kvm_vcpu *vcpu, struct kvm_run *run,
[ ... ]
> -	} else if ((insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD) {
> +	} else if (riscv_insn_is_c_ld(insn)) {
>  		len = 8;
>  		shift = 8 * (sizeof(ulong) - len);
> -		insn = RVC_RS2S(insn) << SH_RD;
> +		rd = riscv_insn_c_ld_extract_xd(insn);

[Severity: High]
Does this code miss the + 8 offset calculation when extracting the register?

Previously, RVC_RS2S correctly added the 8 offset so compressed MMIO loads
would write to x8-x15. With the raw 3-bit extractor macro
riscv_insn_c_ld_extract_xd(insn), rd will be a raw 0-7 value, causing
compressed MMIO loads like c.ld to overwrite x0-x7 (corrupting SP, GP, etc.)
instead.

> -	} else if ((insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
[ ... ]
> @@ -537,35 +536,30 @@ int kvm_riscv_vcpu_mmio_store(struct kvm_vcpu *vcpu, struct kvm_run *run,
[ ... ]
> -	} else if ((insn & INSN_MASK_C_SD) == INSN_MATCH_C_SD) {
> +	} else if (riscv_insn_is_c_sd(insn)) {
>  		len = 8;
> -		data64 = GET_RS2S(insn, &vcpu->arch.guest_context);
> +		rs2 = riscv_insn_c_sd_extract_xs2(insn);

[Severity: High]
Does this code leak the wrong register state to the emulated MMIO device?

Similar to the load case above, GET_RS2S correctly added the 8 offset, but
riscv_insn_c_sd_extract_xs2(insn) extracts only the raw 3-bit value. This
causes compressed MMIO stores like c.sd to read from x0-x7 instead of x8-x15,
leaking unintended guest state.

> -	} else if ((insn & INSN_MASK_C_SDSP) == INSN_MATCH_C_SDSP &&
[ ... ]

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