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

Make all CSR writes unsafe by default #251

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

Conversation

romancardenas
Copy link
Contributor

Closes #209

Work in Progress! I added a safe pattern to opt out of unsafety in cases where we consider there to be no safety issues. I will list all the different registers where we use this macro and nominate some to maintain write safety.

@romancardenas romancardenas requested a review from a team as a code owner December 19, 2024 08:19
@rmsyn
Copy link
Contributor

rmsyn commented Dec 20, 2024

Since most of these registers are WARL, what are we using for safe vs. unsafe criteria? Is it going to be related to side-effects, e.g. mtvec pointing to a potentially invalid memory region only triggered by an interrupt/exception?

Whatever criteria we come up with, maybe we should also add a blurb to the top-level register module about them?

Adding the safe opt-out is a good approach in my opinion. I agree that the CSR writes should be unsafe by default.

@romancardenas
Copy link
Contributor Author

what are we using for safe vs. unsafe criteria? Is it going to be related to side-effects, e.g. mtvec pointing to a potentially invalid memory region only triggered by an interrupt/exception?

Yes, let us leave all writes of CSRs that might trigger invalid states unsafe. mtvec is a good example. I guess that mstatus must also be unsafe, as it can break critical sections, for instance.

@romancardenas
Copy link
Contributor Author

romancardenas commented Jan 10, 2025

Registers built with write_csr_as_usize that could be write-safe

  • mepc
  • mhpmcounterx
  • mhpmeventx
  • mscratch
  • pmpaddrx
  • pmpcfgx
  • satp
  • sepc
  • sscratch

I personally think that all these registers should be unsafe to write

@romancardenas
Copy link
Contributor Author

I have been reading the ISA and I feel like most of the registers should be unsafe to write. I think it would be a better idea force all write functions to be unsafe and open a new issue/RFC to nominate registers to be safe. What do you think @rust-embedded/riscv ?

Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

I think it would be a better idea force all write functions to be unsafe and open a new issue/RFC to nominate registers to be safe.

I agree with this approach, and we should probably outline criteria for CSR write safety.

As we discussed before, most of the CSRs in the ISA are WARL (Write Anything Read Legal), so safety should be based on side-effects of writing to the registers.

For a rough set of attributes to consider:

  • does a CSR write introduce potential memory safety issues in safe code?
  • does a CSR write introduce potential undefined behavior in safe code?
  • does a CSR write invalidate invariants in safe code?

It may be a good idea to add some sort of entry in either the top-level or register documentation. Something we can at least point contributors to when a safe RFC is being reviewed.

@romancardenas
Copy link
Contributor Author

I drafted a quick RFC template to ease the nomination process in #256

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.

riscv: All the CSR write operations should be unsafe by default
2 participants