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.

Sign up for GitHub

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

Jump to bottom

Control and Status Register issues #2

Closed
TheThirdOne opened this issue Aug 28, 2017 · 3 comments
Closed

Control and Status Register issues #2

TheThirdOne opened this issue Aug 28, 2017 · 3 comments
Labels
bug

Comments

@TheThirdOne
Copy link
Owner

There are a few issues with the control and status register implementation. All of these are solvable, but there is a little awkwardness involved with each.

  • CSRs are only accessible by number in the instruction set. This can be fixed with gratuitous psuedo-ops or defining a new type of register token that can be a number or a name.
  • CSRs can not have read only sections. Many CSRs such as uip have bits which cannot be modified (but change value in hardware). The adding read only stuff should be pretty easy, but also making it so it updates properly is where some awkwardness comes in.
  • RDCYCLE[H], RDTIME[H], RDINSTRET[H] don't exist. RDCYCLE AND RDINSTRET should be easy to implement given the above issue is done. RDTIME is a bit more problematic, because it is not clear if it should always be updating or not. It shouldn't be a big issue though; these should not be commonly used.
@Sustrak
Copy link

Sustrak commented May 14, 2019

Realted with CSR instructions I have found a bug with the compilation of the instruction CSRW. Given the following code:

csrrw t1, 0x0, t0
csrrw t3, 0x0, t2

it should be compiled to (compiled with riscv32-unknown-elf-as):

00029373          	csrrw	t1,ustatus,t0
00039e73          	csrrw	t3,ustatus,t2

but with the RARS compiler we get:

00501373          	csrrw	t1,ustatus,t0
00701e73          	csrrw	t3,ustatus,t2

@TheThirdOne
Copy link
Owner Author

Thanks for the report. I think I have fixed the problem in c762e55.

All of the CSRR instructions had the first and second arguments flipped.

TheThirdOne added a commit that referenced this issue May 17, 2019
@TheThirdOne
Add cycle(h) and instret(h)
522ebbd 
Partial fix to #2
TheThirdOne added a commit that referenced this issue Oct 4, 2019
@TheThirdOne
Allow CSRs numbers larger than 0xFF to be assembled
c40643a 
Progress towards #52. I would like to include tests for CSR instructions in
this, but I realized that RARS has possibly incorrect behaviour in several ways.
Writing a test that ensures correctness might be kindof hard so I am pushing
this out to solve th3e immediate problems.

Problems I think RARS currently has:
  - writing to intret (and others) should be an error
  - uip (and others) should have write protection on some bits
  - csrrs might set the value of rd to the wrong version of the CSR eg after the instruction rather than before

These should be added to #2.
TheThirdOne added a commit that referenced this issue Oct 6, 2019
@TheThirdOne
Introduce WPRI bits and read-only CSRs
3879649 
Major progress towards #2
@TheThirdOne
Copy link
Owner Author

I don't know of any remaining issues with the way CSRs are handled. The Zicsr extension should now be fully and correctly implemented.

The issue of RDTIME still exists, but that is not an issue with the CSR part, but rather with the time counter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@TheThirdOne @Sustrak