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

[DongdongZhang]

> -----原始邮件-----发件人:"Andrew Jones" <ajones at ventanamicro.com>发送时间:2024-07-01 17:20:37 (星期一)收件人:zhangdongdong at eswincomputing.com抄送:opensbi at lists.infradead.org主题:Re: [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn
> 
> 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
> 
Hi Drew,

Thank you for your feedback. You are right. I will update the patch as suggested.

Best regards,
Dongdong
> > +		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