[v2] sim: riscv: Fix Zicsr and fence instructions

  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        | 82 ++++++++++++++++++++++++++++++++-----
 sim/testsuite/riscv/fence.s | 17 ++++++++
 sim/testsuite/riscv/zicsr.s | 24 +++++++++++
 3 files changed, 112 insertions(+), 11 deletions(-)
 create mode 100644 sim/testsuite/riscv/fence.s
 create mode 100644 sim/testsuite/riscv/zicsr.s

v2: address review comments, make store_csr execution
only executed when rs1 != 0.
  

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        | 82 ++++++++++++++++++++++++++++++++-----
>  sim/testsuite/riscv/fence.s | 17 ++++++++
>  sim/testsuite/riscv/zicsr.s | 24 +++++++++++
>  3 files changed, 112 insertions(+), 11 deletions(-)
>  create mode 100644 sim/testsuite/riscv/fence.s
>  create mode 100644 sim/testsuite/riscv/zicsr.s
>
> v2: address review comments, make store_csr execution
> only executed when rs1 != 0.
>  
> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index 1815d7f2a6c..d556e6f5bdd 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -535,45 +535,100 @@ 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: \
> +	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
> +	  if (rs1) \

Could you be explicit about the comparisons added in this commit please,
e.g.:

  if (rs1 != 0)

this is the GDB style, and given sim/ is maintained on the same list I
figure the rules should carry over.


> +	    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_CSRRS:
> +      TRACE_INSN (cpu, "csrrs %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); \
> +	  if (rs1) \
> +	    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 %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); \

For CSRRW the check is on the CSR read.  If 'rd == 0' then no CSR read
is performed.

> +	  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, ...) \
>  	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 & !riscv_cpu->regs[rs1]); \
> +	  if (rs1) \
> +	    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_CSRRS:
> -      TRACE_INSN (cpu, "csrrs");
> +    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 | riscv_cpu->regs[rs1]); \
> +	  if (rs1) \
> +	    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_CSRRW:
> -      TRACE_INSN (cpu, "csrrw");
> +    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)); \

Here too the fetch_csr call should not be performed if 'rd == 0'.

Thanks,
Andrew


> -	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
> -		     riscv_cpu->regs[rs1]); \
> +	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, rs1); \
>  	  break;
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_CSR
> @@ -622,6 +677,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 +1407,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
  

On 5/1/24 17:39, 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        | 82 ++++++++++++++++++++++++++++++++-----
>>  sim/testsuite/riscv/fence.s | 17 ++++++++
>>  sim/testsuite/riscv/zicsr.s | 24 +++++++++++
>>  3 files changed, 112 insertions(+), 11 deletions(-)
>>  create mode 100644 sim/testsuite/riscv/fence.s
>>  create mode 100644 sim/testsuite/riscv/zicsr.s
>>
>> v2: address review comments, make store_csr execution
>> only executed when rs1 != 0.
>>  
>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
>> index 1815d7f2a6c..d556e6f5bdd 100644
>> --- a/sim/riscv/sim-main.c
>> +++ b/sim/riscv/sim-main.c
>> @@ -535,45 +535,100 @@ 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: \
>> +	  tmp = fetch_csr (cpu, #name, num, &riscv_cpu->csr.name); \
>> +	  if (rs1) \
> 
> Could you be explicit about the comparisons added in this commit please,
> e.g.:
> 
>   if (rs1 != 0)
> 
> this is the GDB style, and given sim/ is maintained on the same list I
> figure the rules should carry over.
> 
> 

I wanted to follow the style in store_rd where we have this:

static INLINE void
store_rd (SIM_CPU *cpu, int rd, unsigned_word val)
{
  struct riscv_sim_cpu *riscv_cpu = RISCV_SIM_CPU (cpu);

  if (rd)
    {
      riscv_cpu->regs[rd] = val;
      TRACE_REG (cpu, rd);
    }
}



>> +	    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_CSRRS:
>> +      TRACE_INSN (cpu, "csrrs %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); \
>> +	  if (rs1) \
>> +	    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 %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); \
> 
> For CSRRW the check is on the CSR read.  If 'rd == 0' then no CSR read
> is performed.
> 

That is a bit overkill, especially here.  If I add an if (rd != 0) here,
I need yet another if around the store_rd below, because the uninitalized
tmp will certainly generate warnings.  And here is also the reason why this
code got my attention in the first place: because previously a "csrrw a1, csr, a1"
could not work correctly, since rd == rs1 overwrote the rs1 register too early,
therefore I added the "tmp = ", and then I realized that the whole
instruction was not executable at all, due to the introduction of
INSN_CLASS_ZICSR.

But the spec says, there are no side effects except possible disallowed accesses:
"The CSRs defined so far do not have any architectural side effects on reads beyond raising illegal
instruction exceptions on disallowed accesses. Custom extensions might add CSRs with side
effects on reads. "

So since the write is unconditional that will create the exception
anyway, and custom extension CSRs are out of scope for the riscv simulator.


>> +	  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, ...) \
>>  	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 & !riscv_cpu->regs[rs1]); \
>> +	  if (rs1) \
>> +	    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_CSRRS:
>> -      TRACE_INSN (cpu, "csrrs");
>> +    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 | riscv_cpu->regs[rs1]); \
>> +	  if (rs1) \
>> +	    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_CSRRW:
>> -      TRACE_INSN (cpu, "csrrw");
>> +    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)); \
> 
> Here too the fetch_csr call should not be performed if 'rd == 0'.
> 

Same here.  I do not think that there will be any side effects that may be
worth this complication.


Thanks
Bernd.


> Thanks,
> Andrew
> 
> 
>> -	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, \
>> -		     riscv_cpu->regs[rs1]); \
>> +	  store_csr (cpu, #name, num, &riscv_cpu->csr.name, rs1); \
>>  	  break;
>>  #include "opcode/riscv-opc.h"
>>  #undef DECLARE_CSR
>> @@ -622,6 +677,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 +1407,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
>