From mboxrd@z Thu Jan 1 00:00:00 1970 From: DongdongZhang Date: Thu, 4 Jul 2024 15:33:49 +0800 (GMT+08:00) Subject: [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn In-Reply-To: <303914d8-ac58-46f2-9960-1296d52654df@dram.page> References: <20240701024516.20320-1-zhangdongdong@eswincomputing.com> <20240701-843601b1dc18aee9a657be49@orel> <303914d8-ac58-46f2-9960-1296d52654df@dram.page> Message-ID: <21137bb3.477e.1907ca9ec65.Coremail.zhangdongdong@eswincomputing.com> List-Id: To: opensbi@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit > -----????-----???:"Vivian Wang" ????:2024-07-03 19:00:27 (???)???:"Andrew Jones" , zhangdongdong at eswincomputing.com??:opensbi at lists.infradead.org??:Re: [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn > > (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 > >> > >> - 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 > >> --- > >> 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 > >> > >> #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 You are right. Your code is more concise and clear. Thanks, 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