[PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn

[Vivian Wang]

(Resend, forgot to CC mailing list last time, sorry.)

On 7/1/24 17:20, Andrew Jones wrote:

> On Mon, Jul 01, 2024 at 10:45:16AM GMT, zhangdongdong at eswincomputing.com wrote:
>> From: Dongdong Zhang <zhangdongdong at eswincomputing.com>
>>
>> - Completed TODO in `system_opcode_insn` to ensure CSR read/write
>>   instruction handling.
>> - Refactored to use new macros `GET_RS1_NUM` and `GET_CSR_NUM`.
>> - Updated `GET_RM` macro and replaced hardcoded funct3 values with
>>   constants (`CSRRW`, `CSRRS`, `CSRRC`, etc.).
>> - Removed redundant `GET_RM` from `riscv_fp.h`.
>> - Improved validation and error handling for CSR instructions.
>>
>> This patch enhances the clarity and correctness of CSR handling
>> in `system_opcode_insn`.
>>
>> Signed-off-by: Dongdong Zhang <zhangdongdong at eswincomputing.com>
>> ---
>>  include/sbi/riscv_encoding.h | 19 +++++++++++++++++-
>>  include/sbi/riscv_fp.h       |  1 -
>>  lib/sbi/sbi_illegal_insn.c   | 37 +++++++++++++++++++++++-------------
>>  3 files changed, 42 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
>> index 477fa3a..5146654 100644
>> --- a/include/sbi/riscv_encoding.h
>> +++ b/include/sbi/riscv_encoding.h
>> @@ -947,7 +947,10 @@
>>  #define REG_PTR(insn, pos, regs)	\
>>  	(ulong *)((ulong)(regs) + REG_OFFSET(insn, pos))
>>  
>> -#define GET_RM(insn)			(((insn) >> 12) & 7)
>> +#define GET_RM(insn) ((insn & MASK_FUNCT3) >> SHIFT_FUNCT3)
>> +
>> +#define GET_RS1_NUM(insn)		((insn & MASK_RS1) >> 15)
>> +#define GET_CSR_NUM(insn)		((insn & MASK_CSR) >> SHIFT_CSR)
>>  
>>  #define GET_RS1(insn, regs)		(*REG_PTR(insn, SH_RS1, regs))
>>  #define GET_RS2(insn, regs)		(*REG_PTR(insn, SH_RS2, regs))
>> @@ -959,7 +962,21 @@
>>  #define IMM_I(insn)			((s32)(insn) >> 20)
>>  #define IMM_S(insn)			(((s32)(insn) >> 25 << 5) | \
>>  					 (s32)(((insn) >> 7) & 0x1f))
>> +
>>  #define MASK_FUNCT3			0x7000
>> +#define MASK_RS1			0xf8000
>> +#define MASK_CSR            0xfff
>> +
>> +#define SHIFT_FUNCT3    12
>> +#define SHIFT_CSR       20
>> +
>> +
>> +#define CSRRW 1
>> +#define CSRRS 2
>> +#define CSRRC 3
>> +#define CSRRWI 5
>> +#define CSRRSI 6
>> +#define CSRRCI 7
>>  
>>  /* clang-format on */
>>  
>> diff --git a/include/sbi/riscv_fp.h b/include/sbi/riscv_fp.h
>> index 3141c1c..f523c56 100644
>> --- a/include/sbi/riscv_fp.h
>> +++ b/include/sbi/riscv_fp.h
>> @@ -15,7 +15,6 @@
>>  #include <sbi/sbi_types.h>
>>  
>>  #define GET_PRECISION(insn) (((insn) >> 25) & 3)
>> -#define GET_RM(insn) (((insn) >> 12) & 7)
>>  #define PRECISION_S 0
>>  #define PRECISION_D 1
>>  
>> diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
>> index ed6f111..7086a5e 100644
>> --- a/lib/sbi/sbi_illegal_insn.c
>> +++ b/lib/sbi/sbi_illegal_insn.c
>> @@ -48,9 +48,10 @@ static int misc_mem_opcode_insn(ulong insn, struct sbi_trap_regs *regs)
>>  
>>  static int system_opcode_insn(ulong insn, struct sbi_trap_regs *regs)
>>  {
>> -	int do_write, rs1_num = (insn >> 15) & 0x1f;
>> -	ulong rs1_val = GET_RS1(insn, regs);
>> -	int csr_num   = (u32)insn >> 20;
>> +	bool do_write	= false;
>> +	int rs1_num	= GET_RS1_NUM(insn);
>> +	ulong rs1_val	= GET_RS1(insn, regs);
>> +	int csr_num	= GET_CSR_NUM((u32)insn);
>>  	ulong prev_mode = (regs->mstatus & MSTATUS_MPP) >> MSTATUS_MPP_SHIFT;
>>  	ulong csr_val, new_csr_val;
>>  
>> @@ -60,32 +61,42 @@ static int system_opcode_insn(ulong insn, struct sbi_trap_regs *regs)
>>  		return SBI_EFAIL;
>>  	}
>>  
>> -	/* TODO: Ensure that we got CSR read/write instruction */
>> +	/* Ensure that we got CSR read/write instruction */
>> +	int funct3 = GET_RM(insn);
>> +	if ((funct3 < 1 || funct3 > 3) && (funct3 < 5 || funct3 > 7)) {
> This looks complicated. Isn't it just
>
>  (func3 < 1 || func3 > 7 || func3 == 4)
>
> Thanks,
> drew

Getting warmer but funct3 is just 3 bits, so it can't be > 7, right?
IIUC this is just (funct3 == 0 || funct3 == 4).

Vivian "dram" Wang

>> +		sbi_printf("%s: Invalid opcode for CSR read/write instruction",
>> +			   __func__);
>> +		return truly_illegal_insn(insn, regs);
>> +	}
>>  
>>  	if (sbi_emulate_csr_read(csr_num, regs, &csr_val))
>>  		return truly_illegal_insn(insn, regs);
>>  
>>  	do_write = rs1_num;
>> -	switch (GET_RM(insn)) {
>> -	case 1:
>> +	switch (funct3) {
>> +	case CSRRW:
>>  		new_csr_val = rs1_val;
>> -		do_write    = 1;
>> +		do_write    = true;
>>  		break;
>> -	case 2:
>> +	case CSRRS:
>>  		new_csr_val = csr_val | rs1_val;
>> +		do_write    = (rs1_num != 0);
>>  		break;
>> -	case 3:
>> +	case CSRRC:
>>  		new_csr_val = csr_val & ~rs1_val;
>> +		do_write    = (rs1_num != 0);
>>  		break;
>> -	case 5:
>> +	case CSRRWI:
>>  		new_csr_val = rs1_num;
>> -		do_write    = 1;
>> +		do_write    = true;
>>  		break;
>> -	case 6:
>> +	case CSRRSI:
>>  		new_csr_val = csr_val | rs1_num;
>> +		do_write    = (rs1_num != 0);
>>  		break;
>> -	case 7:
>> +	case CSRRCI:
>>  		new_csr_val = csr_val & ~rs1_num;
>> +		do_write    = (rs1_num != 0);
>>  		break;
>>  	default:
>>  		return truly_illegal_insn(insn, regs);
>> -- 
>> 2.17.1
>>
>>
>> -- 
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi