Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise exception when RV32E instructions use x16-x31 #578

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eleanorLYJ
Copy link
Collaborator

@eleanorLYJ eleanorLYJ commented Feb 28, 2025

Modify rv_decode() to enforce RV32E register constraints by validating rd, rs1, rs2, and rs3, ensuring they remain within x0-x15. If an instruction attempts to use x16-x31, trigger an illegal instruction exception, aligning with RISC-V privileged architecture behavior.

According to The RISC-V Instruction Set Manual Volume I: Unprivileged Architecture, Version 20240411:

"RV32E and RV64E ... only registers x0-x15 are provided. All encodings specifying the other registers x16-x31 are reserved."

Summary by Bito

This pull request enhances the rv_decode() function by enforcing constraints on RV32E register usage, ensuring valid register ranges and triggering exceptions for invalid accesses. This improves instruction decoding robustness and aligns with RISC-V specifications.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks

Benchmark suite Current: f6af403 Previous: 05640e9 Ratio
Dhrystone 1331 Average DMIPS over 10 runs 1328 Average DMIPS over 10 runs 1.00
Coremark 946.757 Average iterations/sec over 10 runs 941.867 Average iterations/sec over 10 runs 0.99

This comment was automatically generated by workflow using github-action-benchmark.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't touch src/softfloat.

@eleanorLYJ eleanorLYJ force-pushed the avoid-using-over-16-regs-RV32E branch from 384a7fb to 22dc925 Compare February 28, 2025 13:03
src/decode.c Outdated
if (!op(ir, insn))
return false;

if (ir->rd > 15 || ir->rs1 > 15 || ir->rs2 > 15 || ir->rs3 > 15)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add unlikely and comments.

@visitorckw
Copy link
Collaborator

How about rewriting it this way to avoid repeating the same logic twice?

diff --git a/src/decode.c b/src/decode.c
index f65556d..a8ba70e 100644
--- a/src/decode.c
+++ b/src/decode.c
@@ -1983,6 +1983,8 @@ typedef bool (*decode_t)(rv_insn_t *ir, uint32_t insn);
 /* decode RISC-V instruction */
 bool rv_decode(rv_insn_t *ir, uint32_t insn)
 {
+    bool ret;
+
     assert(ir);
 
 #define OP_UNIMP op_unimp
@@ -2026,7 +2028,8 @@ bool rv_decode(rv_insn_t *ir, uint32_t insn)
         /* decode instruction (compressed instructions) */
         const decode_t op = rvc_jump_table[c_index];
         assert(op);
-        return op(ir, insn);
+        ret = op(ir, insn);
+        goto final;
     }
 #endif
 
@@ -2036,8 +2039,17 @@ bool rv_decode(rv_insn_t *ir, uint32_t insn)
     /* decode instruction */
     const decode_t op = rv_jump_table[index];
     assert(op);
-    return op(ir, insn);
+    ret = op(ir, insn);
 
 #undef OP_UNIMP
 #undef OP
+
+final:
+
+#if RV32_HAS(RV32E)
+    if (unlikely(ir->rd > 15 || ir->rs1 > 15 || ir->rs2 > 15 || ir->rs3 > 15))
+        return false;
+#endif
+
+    return ret;
 }

@eleanorLYJ eleanorLYJ force-pushed the avoid-using-over-16-regs-RV32E branch 2 times, most recently from b50212b to 05640e9 Compare February 28, 2025 13:29
@ChinYikMing
Copy link
Collaborator

The system emulation CI can be rerun and should be fixed.

Related: #579

@ChinYikMing
Copy link
Collaborator

The system emulation CI can be rerun and should be fixed.

Related: #579

@jserv Please merge #579 first, then rebase and the CI for this PR will be passed.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase the latest master branch as @ChinYikMing requested.

Modify rv_decode() to enforce RV32E register constraints by validating
rd, rs1, rs2, and rs3, ensuring they remain within x0-x15. If an
instruction attempts to use x16-x31, trigger an illegal instruction
exception, aligning with RISC-V privileged architecture behavior.

According to The RISC-V Instruction Set Manual Volume I: Unprivileged
Architecture, Version 20240411:

"RV32E and RV64E ...  only registers x0-x15 are provided. All encodings
specifying the other registers x16-x31 are reserved."
@eleanorLYJ eleanorLYJ force-pushed the avoid-using-over-16-regs-RV32E branch from 05640e9 to f6af403 Compare March 1, 2025 02:50
@jserv
Copy link
Contributor

jserv commented Mar 1, 2025

Can you check the failure of RISC-V Architectural Tests reported by CI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants