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

Compressed instruction decoder edge case not handled #796

Closed
mikaelsky opened this issue Feb 8, 2024 · 3 comments · Fixed by #797
Closed

Compressed instruction decoder edge case not handled #796

mikaelsky opened this issue Feb 8, 2024 · 3 comments · Fixed by #797
Assignees
Labels
bug Something isn't working risc-v compliance Modification to comply with official RISC-V specs.

Comments

@mikaelsky
Copy link
Collaborator

Describe the bug
The compressed instruction decoder doesn't handle all the edge cases correctly.

Examples:
For C1 funct3 = 011 we have a few corners that seem to be mishandled.
We are not checking that imm5/shamt5 is 0:

     --              |             FUNCT6                      |
      --     FMT | OP | FUNCT3 | imm5/shamt5 | FUNCT2 | FUNCT 6 | Inst       | Notes
      --  -------------------------------------------------------------------------------------------
      --  CS/C1  | 01 | 100       0              11   |   11    | c.and      | fully populated
      --  CS/C1  | 01 | 100       0              11   |   10    | c.ot       | fully populated
      --  CS/C1  | 01 | 100       0              11   |   01    | c.xor      | fully populated
      --  CS/C1  | 01 | 100       0              11   |   00    | c.sub      | fully populated

For C1 funct3 = 100 we have some further corners that aren't detected. Specifically rs2 isn't checked.

      --   CR/C2  | 10 | 100    | 0  | c.jr           | only valid for  rs2=0 and rs1/=0
      --   CR/C2  | 10 | 100    | 1  | c.jalr         | only valid for  rs2=0 and rs1/=0
      --   CR/C2  | 10 | 100    | 0  | c.mv           | only valid for rs2/=0. RD=0 is a HINT
      --   CR/C2  | 10 | 100    | 1  | c.add          | only valid for rs2/=0. RD=0 is a HINT
      --   CR/C2  | 10 | 100    | 1  | c.ebreak       | only valid for  rs2=0 and rs1=0

For C2 we are not checking valid register values for some instructions

      -- For OP=10 we have the following funct options
      --               |   FUNCT4    |
      --     FMT  | OP | FUNCT3 |    | Inst           | Notes
      --  ------------------------------------------------------------------------
      --   CI/C2  | 10 | 010    | NA | c.lwsp         | fully populated, valid for RD/=0
      --   CR/C2  | 10 | 100    | 0  | c.jr           | only valid for  rs2=0 and rs1/=0

To Reproduce
Any of the above instructions with an illegal setting, like c.jr with rs1 =0 should trigger a trap.

Example opcode:
0x4072

The paths got triggered by riscvDV randomized illegal instruction generation.

Expected behavior
Illegal instruction traps should be taken when a non-HINT reserved instruction space is hit

Screenshots
NA

Environment:

  • RHEL7
  • GCC Version 13.2
  • Libraries used newlib

Hardware:

  • Hardware version (1.9.3.0) bug patched to main

Additional context
I have a locally patched neorv32_cpu_decompressed.vhd that passes riscv-arch-test C and riscvDV randomized illegal instructions test.
My version has a lot more comments that helped me figure out what was going on :)
The patched version is a lot more explicit and the decoding. I do use case? for one of the cases, which allows insertion of the don't care '-' which is very useful for C1 funct2=100 decoding.
I haven't done a lot of cleaning of what would be redundant code, was mainly focused on getting the right decoding functionality in there.

@stnolting
Copy link
Owner

Hey @mikaelsky. Thank you for reporting this. I just had another look at the C spec.

For C1 funct3 = 011 we have a few corners that seem to be mishandled.
We are not checking that imm5/shamt5 is 0: [...]

I agree. Not all bits of nzuimm are checked.

For C1 C2 funct3 = 100 we have some further corners that aren't detected. Specifically rs2 isn't checked.
[...]
For C2 we are not checking valid register values for some instructions

I also agree. The rs1/rd and rs2 fields are not checked if they are (not) zero.

I think this can be fixed quite easily. I'll draft a PR for this.

@stnolting stnolting self-assigned this Feb 9, 2024
@stnolting stnolting added bug Something isn't working risc-v compliance Modification to comply with official RISC-V specs. labels Feb 9, 2024
@stnolting stnolting linked a pull request Feb 9, 2024 that will close this issue
@stnolting
Copy link
Owner

This was easy to fix. I think #797 addresses all the issues you have identified.

Thanks again for finding these loopholes! 👍

@mikaelsky
Copy link
Collaborator Author

I will take a look when I get into the office and put #797 through the riscvDV wringer :) If it works I'll shelve my patch :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working risc-v compliance Modification to comply with official RISC-V specs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants