diff mbox series

sim: riscv: Fix Zicsr and fence instructions

Message ID AS8P193MB128527EA5FB6B2D7A4A96B1DE41B2@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM
State New
Headers
Series sim: riscv: Fix Zicsr and fence instructions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Bernd Edlinger April 29, 2024, 4:38 a.m. UTC 
  The Zicsr instructions were totally broken, and
some instructions like fence.tso were missing.

Since the test coverage is not very good, add some
basic tests for fence and csrrw instructions.
---
 sim/riscv/sim-main.c        | 74 ++++++++++++++++++++++++++++++++-----
 sim/testsuite/riscv/fence.s | 17 +++++++++
 sim/testsuite/riscv/zicsr.s | 24 ++++++++++++
 3 files changed, 106 insertions(+), 9 deletions(-)
 create mode 100644 sim/testsuite/riscv/fence.s
 create mode 100644 sim/testsuite/riscv/zicsr.s

Comments

Andrew Burgess April 29, 2024, 10:01 a.m. UTC | #1 
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> The Zicsr instructions were totally broken, and
> some instructions like fence.tso were missing.
>
> Since the test coverage is not very good, add some
> basic tests for fence and csrrw instructions.
> ---
>  sim/riscv/sim-main.c        | 74 ++++++++++++++++++++++++++++++++-----
>  sim/testsuite/riscv/fence.s | 17 +++++++++
>  sim/testsuite/riscv/zicsr.s | 24 ++++++++++++
>  3 files changed, 106 insertions(+), 9 deletions(-)
>  create mode 100644 sim/testsuite/riscv/fence.s
>  create mode 100644 sim/testsuite/riscv/zicsr.s
>
> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index 1815d7f2a6c..69007d3108e 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -535,37 +535,57 @@ execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>        break;
>  
>      case MATCH_CSRRC:
> -      TRACE_INSN (cpu, "csrrc");
> +      TRACE_INSN (cpu, "csrrc %s, %#x, %s;",
> +		  rd_name, csr, rs1_name);
>        switch (csr)
>  	{
>  #define DECLARE_CSR(name, num, ...) \
>  	case num: \
> -	  store_rd (cpu, rd, \
> -		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
> +	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
>  	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
> -		     riscv_cpu->csr.name & !riscv_cpu->regs[rs1]); \
> +		     riscv_cpu->csr.name & ~riscv_cpu->regs[rs1]); \
> +	  store_rd (cpu, rd, tmp); \

I know that store_csr doesn't support many CSRs, and doesn't do any
checks for things like writing to read-only CSRs, but ....

... I think it might be worth adding a check here for rs1 == x0.  The
docs say:

  For both CSRRS and CSRRC, if rs1=x0, then the instruction will not
  write to the CSR at all, and so shall not cause any of the side
  effects that might otherwise occur on a CSR write, such as raising
  illegal instruction exceptions on accesses to read-only CSRs.

Adding these checks now might mean things "just work" if we add support
for more CSRs at a later date.

>  	  break;
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_CSR
>  	}
>        break;
>      case MATCH_CSRRS:
> -      TRACE_INSN (cpu, "csrrs");
> +      TRACE_INSN (cpu, "csrrs %s, %#x, %s;",
> +		  rd_name, csr, rs1_name);
>        switch (csr)
>  	{
>  #define DECLARE_CSR(name, num, ...) \
>  	case num: \
> -	  store_rd (cpu, rd, \
> -		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
> +	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
>  	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
>  		     riscv_cpu->csr.name | riscv_cpu->regs[rs1]); \
> +	  store_rd (cpu, rd, tmp); \
>  	  break;
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_CSR
>  	}
>        break;
>      case MATCH_CSRRW:
> -      TRACE_INSN (cpu, "csrrw");
> +      TRACE_INSN (cpu, "csrrw %s, %#x, %s;",
> +		  rd_name, csr, rs1_name);
> +      switch (csr)
> +	{
> +#define DECLARE_CSR(name, num, ...) \
> +	case num: \
> +	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
> +	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
> +		     riscv_cpu->regs[rs1]); \
> +	  store_rd (cpu, rd, tmp); \
> +	  break;
> +#include "opcode/riscv-opc.h"
> +#undef DECLARE_CSR
> +	}
> +      break;
> +
> +    case MATCH_CSRRCI:
> +      TRACE_INSN (cpu, "csrrci %s, %#x, %#x;",
> +		  rd_name, csr, rs1);
>        switch (csr)
>  	{
>  #define DECLARE_CSR(name, num, ...) \
> @@ -573,7 +593,38 @@ execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>  	  store_rd (cpu, rd, \
>  		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
>  	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
> -		     riscv_cpu->regs[rs1]); \
> +		     riscv_cpu->csr.name & ~rs1); \

I think there should be a similar check around the store_csr call for
the case where rs1 == 0:

  For CSRRSI and CSRRCI, if the uimm[4:0] field is zero, then these
  instructions will not write to the CSR, and shall not cause any of the
  side effects that might otherwise occur on a CSR write.

Thanks,
Andrew

> +	  break;
> +#include "opcode/riscv-opc.h"
> +#undef DECLARE_CSR
> +	}
> +      break;
> +    case MATCH_CSRRSI:
> +      TRACE_INSN (cpu, "csrrsi %s, %#x, %#x;",
> +		  rd_name, csr, rs1);
> +      switch (csr)
> +	{
> +#define DECLARE_CSR(name, num, ...) \
> +	case num: \
> +	  store_rd (cpu, rd, \
> +		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
> +	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
> +		     riscv_cpu->csr.name | rs1); \
> +	  break;
> +#include "opcode/riscv-opc.h"
> +#undef DECLARE_CSR
> +	}
> +      break;
> +    case MATCH_CSRRWI:
> +      TRACE_INSN (cpu, "csrrwi %s, %#x, %#x;",
> +		  rd_name, csr, rs1);
> +      switch (csr)
> +	{
> +#define DECLARE_CSR(name, num, ...) \
> +	case num: \
> +	  store_rd (cpu, rd, \
> +		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
> +	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, rs1); \
>  	  break;
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_CSR
> @@ -622,6 +673,9 @@ execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>      case MATCH_FENCE_I:
>        TRACE_INSN (cpu, "fence.i;");
>        break;
> +    case MATCH_FENCE_TSO:
> +      TRACE_INSN (cpu, "fence.tso;");
> +      break;
>      case MATCH_EBREAK:
>        TRACE_INSN (cpu, "ebreak;");
>        sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_stopped, SIM_SIGTRAP);
> @@ -1349,6 +1403,8 @@ execute_one (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>  			   SIM_SIGILL);
>  	}
>      case INSN_CLASS_I:
> +    case INSN_CLASS_ZICSR:
> +    case INSN_CLASS_ZIFENCEI:
>        return execute_i (cpu, iw, op);
>      case INSN_CLASS_M:
>      case INSN_CLASS_ZMMUL:
> diff --git a/sim/testsuite/riscv/fence.s b/sim/testsuite/riscv/fence.s
> new file mode 100644
> index 00000000000..25200891161
> --- /dev/null
> +++ b/sim/testsuite/riscv/fence.s
> @@ -0,0 +1,17 @@
> +# Check that various fence instructions run without any faults.
> +# mach: riscv32 riscv64
> +
> +.include "testutils.inc"
> +
> +	start
> +
> +	fence
> +	fence	rw,rw
> +	fence	rw,w
> +	fence	r,r
> +	fence	w,w
> +	fence	r,rw
> +	fence.i
> +	fence.tso
> +
> +	pass
> diff --git a/sim/testsuite/riscv/zicsr.s b/sim/testsuite/riscv/zicsr.s
> new file mode 100644
> index 00000000000..7f1bd740230
> --- /dev/null
> +++ b/sim/testsuite/riscv/zicsr.s
> @@ -0,0 +1,24 @@
> +# Check that the Zicsr instructions run without any faults.
> +# mach: riscv32 riscv64
> +
> +.include "testutils.inc"
> +
> +	start
> +
> +	csrrs	a0,frm,x0
> +	csrrw	a1,frm,a0
> +	bne	a1,a0,bad
> +	csrrc	a2,frm,a1
> +	bne	a2,x0,bad
> +	csrrsi	a0,frm,1
> +	bne	a0,x0,bad
> +	csrrci	a0,frm,1
> +	li	a1,1
> +	bne	a0,a1,bad
> +	csrrwi	a0,frm,1
> +	bne	a0,x0,bad
> +
> +	pass
> +
> +bad:
> +	fail
> -- 
> 2.39.2
Bernd Edlinger April 29, 2024, 1:30 p.m. UTC | #2 
On 4/29/24 12:01, Andrew Burgess wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
>> The Zicsr instructions were totally broken, and
>> some instructions like fence.tso were missing.
>>
>> Since the test coverage is not very good, add some
>> basic tests for fence and csrrw instructions.
>> ---
>>  sim/riscv/sim-main.c        | 74 ++++++++++++++++++++++++++++++++-----
>>  sim/testsuite/riscv/fence.s | 17 +++++++++
>>  sim/testsuite/riscv/zicsr.s | 24 ++++++++++++
>>  3 files changed, 106 insertions(+), 9 deletions(-)
>>  create mode 100644 sim/testsuite/riscv/fence.s
>>  create mode 100644 sim/testsuite/riscv/zicsr.s
>>
>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
>> index 1815d7f2a6c..69007d3108e 100644
>> --- a/sim/riscv/sim-main.c
>> +++ b/sim/riscv/sim-main.c
>> @@ -535,37 +535,57 @@ execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>        break;
>>  
>>      case MATCH_CSRRC:
>> -      TRACE_INSN (cpu, "csrrc");
>> +      TRACE_INSN (cpu, "csrrc %s, %#x, %s;",
>> +		  rd_name, csr, rs1_name);
>>        switch (csr)
>>  	{
>>  #define DECLARE_CSR(name, num, ...) \
>>  	case num: \
>> -	  store_rd (cpu, rd, \
>> -		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
>> +	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
>>  	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
>> -		     riscv_cpu->csr.name & !riscv_cpu->regs[rs1]); \
>> +		     riscv_cpu->csr.name & ~riscv_cpu->regs[rs1]); \
>> +	  store_rd (cpu, rd, tmp); \
> 
> I know that store_csr doesn't support many CSRs, and doesn't do any
> checks for things like writing to read-only CSRs, but ....
> 
> ... I think it might be worth adding a check here for rs1 == x0.  The
> docs say:
> 
>   For both CSRRS and CSRRC, if rs1=x0, then the instruction will not
>   write to the CSR at all, and so shall not cause any of the side
>   effects that might otherwise occur on a CSR write, such as raising
>   illegal instruction exceptions on accesses to read-only CSRs.
> 
> Adding these checks now might mean things "just work" if we add support
> for more CSRs at a later date.
> 

Yeah, good point.

will add those checks and send a v2 version.

Thanks
Bernd.
diff mbox series

Patch

diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
index 1815d7f2a6c..69007d3108e 100644
--- a/sim/riscv/sim-main.c
+++ b/sim/riscv/sim-main.c
@@ -535,37 +535,57 @@  execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
       break;
 
     case MATCH_CSRRC:
-      TRACE_INSN (cpu, "csrrc");
+      TRACE_INSN (cpu, "csrrc %s, %#x, %s;",
+		  rd_name, csr, rs1_name);
       switch (csr)
 	{
 #define DECLARE_CSR(name, num, ...) \
 	case num: \
-	  store_rd (cpu, rd, \
-		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
+	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
 	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
-		     riscv_cpu->csr.name & !riscv_cpu->regs[rs1]); \
+		     riscv_cpu->csr.name & ~riscv_cpu->regs[rs1]); \
+	  store_rd (cpu, rd, tmp); \
 	  break;
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR
 	}
       break;
     case MATCH_CSRRS:
-      TRACE_INSN (cpu, "csrrs");
+      TRACE_INSN (cpu, "csrrs %s, %#x, %s;",
+		  rd_name, csr, rs1_name);
       switch (csr)
 	{
 #define DECLARE_CSR(name, num, ...) \
 	case num: \
-	  store_rd (cpu, rd, \
-		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
+	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
 	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
 		     riscv_cpu->csr.name | riscv_cpu->regs[rs1]); \
+	  store_rd (cpu, rd, tmp); \
 	  break;
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR
 	}
       break;
     case MATCH_CSRRW:
-      TRACE_INSN (cpu, "csrrw");
+      TRACE_INSN (cpu, "csrrw %s, %#x, %s;",
+		  rd_name, csr, rs1_name);
+      switch (csr)
+	{
+#define DECLARE_CSR(name, num, ...) \
+	case num: \
+	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
+	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
+		     riscv_cpu->regs[rs1]); \
+	  store_rd (cpu, rd, tmp); \
+	  break;
+#include "opcode/riscv-opc.h"
+#undef DECLARE_CSR
+	}
+      break;
+
+    case MATCH_CSRRCI:
+      TRACE_INSN (cpu, "csrrci %s, %#x, %#x;",
+		  rd_name, csr, rs1);
       switch (csr)
 	{
 #define DECLARE_CSR(name, num, ...) \
@@ -573,7 +593,38 @@  execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
 	  store_rd (cpu, rd, \
 		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
 	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
-		     riscv_cpu->regs[rs1]); \
+		     riscv_cpu->csr.name & ~rs1); \
+	  break;
+#include "opcode/riscv-opc.h"
+#undef DECLARE_CSR
+	}
+      break;
+    case MATCH_CSRRSI:
+      TRACE_INSN (cpu, "csrrsi %s, %#x, %#x;",
+		  rd_name, csr, rs1);
+      switch (csr)
+	{
+#define DECLARE_CSR(name, num, ...) \
+	case num: \
+	  store_rd (cpu, rd, \
+		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
+	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
+		     riscv_cpu->csr.name | rs1); \
+	  break;
+#include "opcode/riscv-opc.h"
+#undef DECLARE_CSR
+	}
+      break;
+    case MATCH_CSRRWI:
+      TRACE_INSN (cpu, "csrrwi %s, %#x, %#x;",
+		  rd_name, csr, rs1);
+      switch (csr)
+	{
+#define DECLARE_CSR(name, num, ...) \
+	case num: \
+	  store_rd (cpu, rd, \
+		    fetch_csr (cpu, #name, num, &riscv_cpu->csr.name)); \
+	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, rs1); \
 	  break;
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR
@@ -622,6 +673,9 @@  execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
     case MATCH_FENCE_I:
       TRACE_INSN (cpu, "fence.i;");
       break;
+    case MATCH_FENCE_TSO:
+      TRACE_INSN (cpu, "fence.tso;");
+      break;
     case MATCH_EBREAK:
       TRACE_INSN (cpu, "ebreak;");
       sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_stopped, SIM_SIGTRAP);
@@ -1349,6 +1403,8 @@  execute_one (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
 			   SIM_SIGILL);
 	}
     case INSN_CLASS_I:
+    case INSN_CLASS_ZICSR:
+    case INSN_CLASS_ZIFENCEI:
       return execute_i (cpu, iw, op);
     case INSN_CLASS_M:
     case INSN_CLASS_ZMMUL:
diff --git a/sim/testsuite/riscv/fence.s b/sim/testsuite/riscv/fence.s
new file mode 100644
index 00000000000..25200891161
--- /dev/null
+++ b/sim/testsuite/riscv/fence.s
@@ -0,0 +1,17 @@ 
+# Check that various fence instructions run without any faults.
+# mach: riscv32 riscv64
+
+.include "testutils.inc"
+
+	start
+
+	fence
+	fence	rw,rw
+	fence	rw,w
+	fence	r,r
+	fence	w,w
+	fence	r,rw
+	fence.i
+	fence.tso
+
+	pass
diff --git a/sim/testsuite/riscv/zicsr.s b/sim/testsuite/riscv/zicsr.s
new file mode 100644
index 00000000000..7f1bd740230
--- /dev/null
+++ b/sim/testsuite/riscv/zicsr.s
@@ -0,0 +1,24 @@ 
+# Check that the Zicsr instructions run without any faults.
+# mach: riscv32 riscv64
+
+.include "testutils.inc"
+
+	start
+
+	csrrs	a0,frm,x0
+	csrrw	a1,frm,a0
+	bne	a1,a0,bad
+	csrrc	a2,frm,a1
+	bne	a2,x0,bad
+	csrrsi	a0,frm,1
+	bne	a0,x0,bad
+	csrrci	a0,frm,1
+	li	a1,1
+	bne	a0,a1,bad
+	csrrwi	a0,frm,1
+	bne	a0,x0,bad
+
+	pass
+
+bad:
+	fail