OpenSBI Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn
@ 2024-07-01  2:45 zhangdongdong
  2024-07-01  9:20 ` Andrew Jones
  0 siblings, 1 reply; 5+ messages in thread
From: zhangdongdong @ 2024-07-01  2:45 UTC (permalink / raw)
  To: opensbi

From: Dongdong Zhang <zhangdongdong@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@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)) {
+		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



^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn
  2024-07-01  2:45 [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn zhangdongdong
@ 2024-07-01  9:20 ` Andrew Jones
  2024-07-03  1:45   ` DongdongZhang
  2024-07-03 11:00   ` Vivian Wang
  0 siblings, 2 replies; 5+ messages in thread
From: Andrew Jones @ 2024-07-01  9:20 UTC (permalink / raw)
  To: opensbi

On Mon, Jul 01, 2024 at 10:45:16AM GMT, zhangdongdong at eswincomputing.com wrote:
> From: Dongdong Zhang <zhangdongdong@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@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

> +		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


^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn
  2024-07-01  9:20 ` Andrew Jones
@ 2024-07-03  1:45   ` DongdongZhang
  2024-07-03 11:00   ` Vivian Wang
  1 sibling, 0 replies; 5+ messages in thread
From: DongdongZhang @ 2024-07-03  1:45 UTC (permalink / raw)
  To: opensbi




> -----????-----???:"Andrew Jones" <ajones@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@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@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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn
  2024-07-01  9:20 ` Andrew Jones
  2024-07-03  1:45   ` DongdongZhang
@ 2024-07-03 11:00   ` Vivian Wang
  2024-07-04  7:33     ` DongdongZhang
  1 sibling, 1 reply; 5+ messages in thread
From: Vivian Wang @ 2024-07-03 11:00 UTC (permalink / raw)
  To: opensbi

(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@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@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



^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn
  2024-07-03 11:00   ` Vivian Wang
@ 2024-07-04  7:33     ` DongdongZhang
  0 siblings, 0 replies; 5+ messages in thread
From: DongdongZhang @ 2024-07-04  7:33 UTC (permalink / raw)
  To: opensbi




> -----????-----???:"Vivian Wang" <uwu@dram.page>????:2024-07-03 19:00:27 (???)???:"Andrew Jones" <ajones@ventanamicro.com>, 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 <zhangdongdong@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@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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-07-04  7:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-01  2:45 [PATCH] lib:sbi: Enhance CSR Handling in system_opcode_insn zhangdongdong
2024-07-01  9:20 ` Andrew Jones
2024-07-03  1:45   ` DongdongZhang
2024-07-03 11:00   ` Vivian Wang
2024-07-04  7:33     ` DongdongZhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox