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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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.