Re: [PATCH] sim: riscv: Fix Zicsr and fence instructions

[Andrew Burgess <aburgess@redhat.com>]

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