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: redefine CSR registers with new helper macros #229

Open
9 of 21 tasks
rmsyn opened this issue Oct 19, 2024 · 7 comments
Open
9 of 21 tasks

riscv: redefine CSR registers with new helper macros #229

rmsyn opened this issue Oct 19, 2024 · 7 comments
Milestone

Comments

@rmsyn
Copy link
Contributor

rmsyn commented Oct 19, 2024

Existing CSR registers could be redefined using the new helper macros to gain extra functionality, and consistent definitions across the crate.

Part of the 0.13.0 milestone.

Registers to redefine:

  • marchid (*)
  • mcause
  • mcounteren
  • medeleg
  • mideleg
  • mie
  • mimpid (*)
  • mip
  • misa (*)
  • mstatus
  • mstatush
  • mtvec
  • mvendorid (*)
  • pmpcfgx
  • satp
  • scause
  • scounteren
  • sie
  • sip
  • sstatus
  • stvec

(*) Currently uses NonZeroUsize

  • use CSR macros arm with sentinel value for checking implementation status

Remaining registers are all simple value registers with no bitfields.

@romancardenas
Copy link
Contributor

I would like to do a release soon with all the new changes so people can start playing with it and detect any potential issues. We can leave these changes for the next release (hopefully before 2025). In fact, what do you think about nominating issues and working on them for the next milestone?

@rmsyn
Copy link
Contributor Author

rmsyn commented Oct 19, 2024

In fact, what do you think about nominating issues and working on them for the next milestone?

That sounds great! Would you like me to create an issue for each CSR register, just to keep things modular?

Alternatively, I could add checkboxes to this issue with each CSR, and check them off as implementations are completed.

@romancardenas romancardenas added this to the `riscv` 0.13.0 milestone Oct 20, 2024
@romancardenas
Copy link
Contributor

I nominated a few issues to be part of the riscv 0.13.0 milestone.

In each issue, we should write a checkbox list to keep track of advances. We can also open a new issue (e.g., riscv 0.13.0), and each of us can assign one issue to work on.

Another option is using GitHub projects. It is a kind of Trello but strongly coupled to the repo.

Let me know what you prefer!

@rmsyn
Copy link
Contributor Author

rmsyn commented Oct 20, 2024

In each issue, we should write a checkbox list to keep track of advances.

This works for me. I'll go through the crate, and add a checkbox for each CSR.

@romancardenas
Copy link
Contributor

Currently, when compiling riscv for RV32 targets, I get the following warnings:

warning: unused variable: `uxl`
   --> riscv/src/register/mstatus.rs:418:31
    |
418 |     pub fn set_uxl(&mut self, uxl: XLEN) {
    |                               ^^^ help: if this is intentional, prefix it with an underscore: `_uxl`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `sxl`
   --> riscv/src/register/mstatus.rs:443:31
    |
443 |     pub fn set_sxl(&mut self, sxl: XLEN) {
    |                               ^^^ help: if this is intentional, prefix it with an underscore: `_sxl`

warning: unused variable: `endianness`
   --> riscv/src/register/mstatus.rs:468:31
    |
468 |     pub fn set_sbe(&mut self, endianness: Endianness) {
    |                               ^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_endianness`

warning: unused variable: `endianness`
   --> riscv/src/register/mstatus.rs:492:31
    |
492 |     pub fn set_mbe(&mut self, endianness: Endianness) {
    |                               ^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_endianness`

warning: `riscv` (lib) generated 4 warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.65s

I think all these fields, in their try_ variant, should return an Unimplemented error, as they are only included in RV64 devices

@rmsyn
Copy link
Contributor Author

rmsyn commented Nov 5, 2024

I think all these fields, in their try_ variant, should return an Unimplemented error, as they are only included in RV64 devices

I agree, thanks for bringing this to my attention. When I get to the mstatus register, I'll make sure to handle this edge case.

We can either extend the helper macros, or use conditional compilation like:

#[cfg(target_arch = "riscv64")]
read_write_csr_field! {
    Mstatus,
    /// MBE docs
    mbe: 37,
}

impl Mstatus {
    #[cfg(target_arch = "riscv32")]
    pub fn set_mbe(&mut self, _endianness: Endianess) {
        unimplemented!("only available on RV64");
    }

    // ...
}

@romancardenas
Copy link
Contributor

I would go for something more like:

impl Mstatus {
    #[cfg_attr(not(target_arch = "riscv64"), allow(unused_variables))]
    pub fn try_set_mbe(&mut self, endianness: Endianess)Result<()> {
        match () {
            #[cfg(target_arch = "riscv64")]
            _ => {...},
            #[cfg(not(target_arch = "riscv64"))]
            _ => Err(Error::Unimplemented),
        }
    }
    pub fn set_mbe(&mut self, endianness: Endianess) { try_set_mbe(endianness).unwrap(); }
}

I wouldn't bother to add this to macros, I think it is a special case that should be handled by a manual implementation.

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

No branches or pull requests

2 participants