* [PATCH] sim: riscv: Fix Zicsr and fence instructions @ 2024-04-29 4:38 Bernd Edlinger 2024-04-29 10:01 ` Andrew Burgess 0 siblings, 1 reply; 3+ messages in thread From: Bernd Edlinger @ 2024-04-29 4:38 UTC (permalink / raw) To: gdb-patches 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); \ 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 -- 2.39.2 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] sim: riscv: Fix Zicsr and fence instructions 2024-04-29 4:38 [PATCH] sim: riscv: Fix Zicsr and fence instructions Bernd Edlinger @ 2024-04-29 10:01 ` Andrew Burgess 2024-04-29 13:30 ` Bernd Edlinger 0 siblings, 1 reply; 3+ messages in thread From: Andrew Burgess @ 2024-04-29 10:01 UTC (permalink / raw) To: Bernd Edlinger, gdb-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] sim: riscv: Fix Zicsr and fence instructions 2024-04-29 10:01 ` Andrew Burgess @ 2024-04-29 13:30 ` Bernd Edlinger 0 siblings, 0 replies; 3+ messages in thread From: Bernd Edlinger @ 2024-04-29 13:30 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-29 13:28 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-29 4:38 [PATCH] sim: riscv: Fix Zicsr and fence instructions Bernd Edlinger 2024-04-29 10:01 ` Andrew Burgess 2024-04-29 13:30 ` Bernd Edlinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).