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

feat: add possibility for custom CSRs #1853

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

arrv-sc
Copy link
Contributor

@arrv-sc arrv-sc commented Nov 7, 2024

Currently in riscv-isa-sim there's no way to make a custom extension that adds new CSRs. This simple patch makes it possible via new virtual function in extension_t class.

@arrv-sc
Copy link
Contributor Author

arrv-sc commented Nov 7, 2024

@aswaterman @jerryz123, could you please take a look

@jerryz123
Copy link
Collaborator

This change looks good. @arrv-sc are you able to identify what might be causing the regression failure? It isn't obvious to me.

@arrv-sc
Copy link
Contributor Author

arrv-sc commented Nov 8, 2024

@jerryz123 I added new target to create-ci-binary-tarball script. Testing script fetches tar archive with binaries compiled for riscv and then executes them. Pipeline fails because archive does not contain executable compiled from my new customcsr.c file. As far as I know this tarball has to be updated manually and I cannot do that.

@jerryz123
Copy link
Collaborator

Ah right, I'll upload the updated tar soon.

Copy link
Collaborator

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

LGTM, too.

Currently in riscv-isa-sim there's no way to make a custom extension
that adds new CSRs. This simple patch makes it possible via new
virtual function in extension_t class.
@jerryz123 jerryz123 merged commit 4156e07 into riscv-software-src:master Nov 9, 2024
3 checks passed
@arrv-sc
Copy link
Contributor Author

arrv-sc commented Nov 9, 2024

@aswaterman there is also a question of whether or not we should check for overwriting standard CSRs with custom ones that use the same addresses. In this commit I didn't implemented this check as it turned out to require some refactoring and I didn't want to make this change too big. Should we implement it after all?
Also after consulting with RISC-V spec I couldn't understand if custom extensions can reuse standard CSR addresses if corresponding standard extension is not enabled. Can you shed some light on this please?

@aswaterman
Copy link
Collaborator

Non-conforming extensions can use standard CSR addresses, but only if the address is not used by a standard extension that the implementation claims to support. For example, the address of fcsr can be reused by a custom extension only if the F and Zfinx extensions are not implemented.

IMO, extending Spike is a feature for experts, so the error checking would be nice to have, but is not absolutely necessary. If the refactoring is very messy, I would be OK with skipping it.

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.

3 participants