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

riscv: Use macros to create non-PMP CSRs #2470

Closed
wants to merge 1 commit into from

Conversation

alistair23
Copy link
Contributor

Pull Request Overview

The current giant switch table for RISC-V CSRs is a pain to edit and extend.

With more RISC-V extensions on the way and with some RISC-V implementations using CSRs to modify the system the list of CSRs is only growing.

In an effort to try and cleanup and improve the RISC-V CSRs (which are also some of the largest functions in the Tock kernel) I have added a macro that generates RISC-V CSR access functions.

Unfortunately I can't figure out a way to do with for the array of PMP CSRs.

Testing Strategy

CI

TODO or Help Wanted

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

The current giant switch table for RISC-V CSRs is a pain to edit and
extend. Instead of relying on it, let's instead use macros with const
values to access CSRs/

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@github-actions github-actions bot added risc-v RISC-V architecture tock-libraries This affects libraries supported by the Tock project labels Mar 8, 2021
@alistair23 alistair23 mentioned this pull request Mar 8, 2021
2 tasks
@hudson-ayers
Copy link
Contributor

hudson-ayers commented Mar 8, 2021

Awhile back I suggested using const generics to pass the addresses of the non-PMP CSRs, as a substitute for the messy code in #1926 . A link to a commit that achieved this is here: hudson-ayers@ab4965c , though it is of course outdated. Now that min_const_generics is stable I think it would be reasonable to consider that approach. Do you think that would be preferable to what you have here? I haven't fully paged all this back in, but I think it would allow for the interface we want without using macros?

@alistair23
Copy link
Contributor Author

alistair23 commented Mar 8, 2021

I had a quick look at that, it's not that simple though as I get this error:

error: type parameters with a default must be trailing
   --> libraries/riscv-csr/src/csr.rs:106:42
    |
106 | pub struct ReadWriteRiscvCsr<T: IntLike, R: RegisterLongName = (), const V: usize> {
    |                                          ^
    |
    = note: using type defaults and const parameters in the same parameter list is currently not permitted

Which can be worked around by not using the default,

The problem is that doesn't help with the array problem

@hudson-ayers
Copy link
Contributor

It seems like this PR doesn't help with the array problem either though? Your other recent PR does, but the costs of dynamic dispatch are probably too high anyway

@alistair23
Copy link
Contributor Author

Yep, it doesn't either. I was originally hoping to get this working the the array (see #2471) but the overhead is too high.

I just got the const generic version working (good idea :) ) so I'm going to close this and use that instead.

@alistair23 alistair23 closed this Mar 8, 2021
bors bot added a commit that referenced this pull request Mar 15, 2021
2472: riscv: Convert CSRs to use const generics r=bradjc a=alistair23

### Pull Request Overview

The current giant switch table for RISC-V CSRs is a pain to edit and
extend.

With more RISC-V extensions on the way and with some RISC-V
implementations using CSRs to modify the system the list of CSRs
is only growing.

In an effort to try and cleanup and improve the RISC-V CSRs
(which are also some of the largest functions in the Tock kernel)
let's use const generics to create the CSR access functions.

This means we can drop the large switch case, but we at the same time
have to remove the PMP arrays. To save some sanity let's add PMP access
functions that can replicate some of the array work.

This has no impact on the size of the OpenTitan binary.

This replaces #2470 and #2471

Although I think #2471 is cleaner in terms of code, the overhead is too high so this ends up being the best compromise.

### Testing Strategy

CI

### TODO or Help Wanted

### Documentation Updated

- [X] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [X] Ran `make prepush`.


Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
lschuermann pushed a commit to lschuermann/tock that referenced this pull request Mar 20, 2021
2472: riscv: Convert CSRs to use const generics r=bradjc a=alistair23

### Pull Request Overview

The current giant switch table for RISC-V CSRs is a pain to edit and
extend.

With more RISC-V extensions on the way and with some RISC-V
implementations using CSRs to modify the system the list of CSRs
is only growing.

In an effort to try and cleanup and improve the RISC-V CSRs
(which are also some of the largest functions in the Tock kernel)
let's use const generics to create the CSR access functions.

This means we can drop the large switch case, but we at the same time
have to remove the PMP arrays. To save some sanity let's add PMP access
functions that can replicate some of the array work.

This has no impact on the size of the OpenTitan binary.

This replaces tock#2470 and tock#2471

Although I think tock#2471 is cleaner in terms of code, the overhead is too high so this ends up being the best compromise.

### Testing Strategy

CI

### TODO or Help Wanted

### Documentation Updated

- [X] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [X] Ran `make prepush`.


Co-authored-by: Alistair Francis <alistair.francis@wdc.com>
@alistair23 alistair23 deleted the alistair/csr-cleanup branch March 24, 2021 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
risc-v RISC-V architecture tock-libraries This affects libraries supported by the Tock project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants