* [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).