CSR rework #1

FawazTirmizi · 2022-10-11T22:38:08Z

This is an initial attempt to rework the register module to use a better set of APIs based on tock-registers. The goal here is to build something that both meets the needs of all of our teams and can be contributed back upstream.

If everyone is alright with the general gist of what I'm aiming for here, I'll go ahead and put out the rest of the CSRs over the next few weeks.

src/register/mcause.rs

Because the CSR modules are going to be rewritten, this was done to ensure that the crate can still build while this is happening.

This adds a couple of macros that will create a set of functions for a given CSR. Most of the time, only `ro_csr!` and `rw_csr!` will need to be invoked directly.

dgreid · 2022-10-14T20:46:07Z

src/register/macros.rs

-            assert!(index < 4);
+        pub fn atomic_replace(val_to_set: $size) -> $size {
+            let r: $size;
+            unsafe {


This and other unsafe blocks could use a comment about what makes them safe for all values written to all CSRs.

Should we look at enabling undocumented-unsafe-blocks

Agreed to both, I'll change it.

src/register/mcause.rs

beezow · 2022-10-14T22:00:21Z

src/register/addresses.rs

@@ -0,0 +1,678 @@
+//! Addresses for CSRs. Automatically generated by parse_opcodes.


A link to how this was generated would be nice.

I looked into parse_opcodes, but it doesn't actually seem like that is capable of generating CSR information. The official riscv-opcodes library does so I can see about using that to do the generation instead.

beezow · 2022-10-14T22:00:30Z

src/lib.rs


 pub mod asm;
-pub mod delay;
-pub mod interrupt;
+//pub mod delay;


commented code

beezow · 2022-10-14T22:00:57Z

rust-toolchain.toml

@@ -0,0 +1,7 @@
+[toolchain]
+channel = "nightly"


What nightly features do you need?

Also, do you want to pin this to a specific version of nightly?

Nightly is needed for the asm-const feature. In retrospect, I may be able to circumvent this as inline assembly is just implemented with format strings.

beezow · 2022-10-14T22:04:30Z

src/register/macros.rs

+    // example would be the PMP CSRs, there are multiple with the exact same
+    // layout. Using this allows the same bitfield to be used multiple times.
+    // Ex:
+    //   rw_csr!(pmpcfg0, pmpcfg, usize);


what type is pmpcfg in this example? is it generated by rw_csr?

I think it would be generated using register_bitfields Reference

Furquan is right, it would come from register_bitfields. I'll make that clearer in the documentation.

beezow · 2022-10-14T22:22:01Z

src/register/mcause.rs

+    #[cfg(target_pointer_width = "32")]
+    pub mcause [
+        cause OFFSET(0) NUMBITS(31) [],
+        is_interrupt OFFSET(31) NUMBITS(1) [],


Maybe call this interrupt, it is a bit confusing when seeing references to this field.

Agreed, I'll change it.

beezow · 2022-10-14T22:26:12Z

src/register/mcause.rs

+    ],
+
+    #[cfg(target_pointer_width = "64")]
+    pub mcause [


Could use mem::size_of::<usize>() rather than hardcode 32 or 64, but in this case I think it is simple enough it doesn't matter.

I like how tock defines XLEN and uses that instead of the #cfg in each module: https://github.com/tock/tock/blob/9cc3d42572ce1ad117895ad81767f671e906d3a6/arch/riscv/src/lib.rs

I didn't notice the XLEN thing, I like that too. I'll use that where I can.

beezow · 2022-10-14T22:29:36Z

src/register/mcause.rs

@@ -52,13 +93,10 @@ impl Interrupt {
    #[inline]
    pub fn from(nr: usize) -> Self {
        match nr {


Can you just give the nums values?

pub enum Interrupt { SupervisorSoft = 1, MachineSoft = 3, ...

Then when you want the integer value do Interrupt::SupervisorSoft as u32

This is for a usize -> Interrupt conversion. There is no core support for doing that as a cast, but I could use num-derive to automatically generate methods to cast between integers. I didn't do this initially as I'm a bit wary of adding new dependencies for this sort of crate, but I think in this case it may be justified, as having casts available to any integer size would be nice.

beezow · 2022-10-14T22:29:55Z

src/register/mod.rs

-pub mod uie;
-pub mod ustatus;
-pub mod utvec;
+//pub mod uie;


Why is this all commented out?

I get flooded with errors otherwise as I got rid of the macros. The alternative is to just outright delete everything and bring stuff back as I need it.

furquan-rivos · 2022-10-14T22:48:04Z

rust-toolchain.toml

@@ -0,0 +1,7 @@
+[toolchain]
+channel = "nightly"


Also, do you want to pin this to a specific version of nightly?

furquan-rivos · 2022-10-14T22:55:46Z

rust-toolchain.toml

+targets = [ "riscv32i-unknown-none-elf", "riscv32imc-unknown-none-elf",
+            "riscv32imac-unknown-none-elf", "riscv64imac-unknown-none-elf",
+            "riscv64gc-unknown-none-elf"]
+profile = "minimal"


Just curious: Why "minimal" and not just "default"? Is it because we don't want the components rust-docs and clippy?

I just copied the file from another riscv project, forgot to fix that. I definitely want rust-docs and clippy.

src/register/macros.rs

furquan-rivos · 2022-10-14T23:19:22Z

src/register/macros.rs

+            r
+        }
+
+        /// Atomically read a CSR and set bits specified in a bitmask


s/set/clear

furquan-rivos · 2022-10-14T23:19:39Z

src/register/macros.rs

+
+        /// Atomically read a CSR and set bits specified in a bitmask
+        ///
+        /// This method corresponds to the RISC-V `CSRRS rd, csr, rs1`


s/CSRRS/CSRRC

furquan-rivos · 2022-10-14T23:38:07Z

src/register/macros.rs

+    // example would be the PMP CSRs, there are multiple with the exact same
+    // layout. Using this allows the same bitfield to be used multiple times.
+    // Ex:
+    //   rw_csr!(pmpcfg0, pmpcfg, usize);


I think it would be generated using register_bitfields Reference

src/register/mcause.rs

furquan-rivos · 2022-10-14T23:54:42Z

src/register/mcause.rs

+    ],
+
+    #[cfg(target_pointer_width = "64")]
+    pub mcause [


I like how tock defines XLEN and uses that instead of the #cfg in each module: https://github.com/tock/tock/blob/9cc3d42572ce1ad117895ad81767f671e906d3a6/arch/riscv/src/lib.rs

furquan-rivos · 2022-10-15T00:03:46Z

src/register/macros.rs

+/// Generates the read functions for a CSR
+macro_rules! csr_reads {


Looks like we are generating these read, write and other functions for each csr that is defined. Why not define a CSR type and provide implementations for that type instead? I am just looking at the example of tock and trying to understand why this design was chosen as opposed to what tock does. [Reference]

I tried that at first, but that would require one of two things:

Implement each CSR as an instance of a generic type. This would mean that CSR-specific functionality would need to be wrapped in a module (in which the instance of the CSR would also need to be created), so an access to a generic function would look something like register::mcause.CSR.read(), and a CSR-specific function would look like register::mcause::is_interrupt(). Not a fan of this as it would mean there's different syntax for using the methods.

Implement each CSR as its own type that implements a generic CSR trait. This would allow CSR-specific functions to also be implemented on the type, so the syntax would be identical. However, this means creating a new type for every CSR, and we'd still need to either create an instance of it in the library (makes the codebase more confusing) or expect users to create their own (adds a needless burden to the users). I'm just in general not a fan of that pattern, but if we want to go with something using a CSR type then this option is, in my opinion, the best.

I was mostly looking at how tock instantiated the CSRs and thinking along the same lines. Salus seems to have inherited the same design. I think this kind of falls into the first category you defined. And it looks like the inconsistent access you described was solved by Tock by implementing functions directly as part of the CSR type e.g. https://github.com/tock/tock/blob/9cc3d42572ce1ad117895ad81767f671e906d3a6/arch/riscv/src/csr/mod.rs#L315

Since I haven't played with either of the interfaces much, I don't have a strong opinion on which way to go. It was more of a curiosity to understand why we are choosing this design.

The way it was written in tock/salus by introducing a generic type for RW-CSR (ReadWriteRiscvCsr) and an interface for it and then using that to define a struct CSR for all CSRs in RV seemed like a fine separation. But as you pointed out, there might be other limitations/drawbacks of this. Maybe @abrestic-rivos has more thoughts on this based on his usage of the tock registers.

I should also note that the first category makes it difficult to implement CSRs with layouts similar to the PMP configuration registers (The same pattern repeated). You can see here the impact of doing it this way. The bitfields are far more convoluted because of this setup.

And it looks like the inconsistent access you described was solved by Tock by implementing functions directly as part of the CSR type

The problem with this is that all functions are in the same namespace, so it's not as easy to add CSR-specific functions for each CSR without the names getting too convoluted.

Since I haven't played with either of the interfaces much, I don't have a strong opinion on which way to go. It was more of a curiosity to understand why we are choosing this design.

While I'm definitely in favor of the approach I've taken here, if there are notable benefits for the other method I don't mind switching.

I understand your point about having to define all the functions within the same namespace. That was one of the things that bothered me as well about the approach.

Anyways, I didn't mean to question your approach. I just wanted to understand your thoughts and reasoning behind choosing the approach. I trust your judgement and evaluation and we can go ahead with that. Thanks for the details! :)

The way it was written in tock/salus by introducing a generic type for RW-CSR (ReadWriteRiscvCsr) and an interface for it and then using that to define a struct CSR for all CSRs in RV seemed like a fine separation. But as you pointed out, there might be other limitations/drawbacks of this. Maybe @abrestic-rivos has more thoughts on this based on his usage of the tock registers.

FWIW, this works pretty well for us (with one exception¹) so if the eventual goal is for Salus to use this too, I'd like to see a compelling reason for why we should change it :)

The CSR-specific-methods issue can be handled by defining and implementing traits for ReadWriteRiscvCsr<CSR> or LocalRegisterCopy<CSR>. For example, if you wanted to define a cause() method like you've done in the subsequent patch, you could do something like:

trait XCauseHelpers { fn cause(&self) -> Trap; fn is_interrupt(&self) -> bool { matches!(self.cause(), Trap::Interrupt(_)) } } trait XCause: LongRegisterName {} impl XCause for mcause::Register {} impl XCause for scause::Register {} impl<R: XCause> XCauseHelpers for ReadWriteRiscvCsr<usize, R> { fn cause(&self) -> Trap { ... } }

And then you'd use it like this:

let trap = CSR.mcause.cause();

That doesn't seem any less ergonomic that what you're proposing, and involves less use of macros (which IMO are less readable than traits/generics).

Footnotes

The tock Readable/Writable traits aren't object safe, so things get annoying if you want to create trait objects from CSRs, which is useful when dealing with a range of identically-defined CSRs (think hpmcounter). ↩

... if the eventual goal is for Salus to use this too, I'd like to see a compelling reason for why we should change it :)

I would definitely like this PR to end up producing the one crate that rules them all, regardless of if it's what I've proposed here or something else.

The CSR-specific-methods issue can be handled by defining and implementing traits for ReadWriteRiscvCsr or LocalRegisterCopy. For example, if you wanted to define a cause() method like you've done in the subsequent patch, you could do something like:

trait XCauseHelpers { fn cause(&self) -> Trap; fn is_interrupt(&self) -> bool { matches!(self.cause(), Trap::Interrupt(_)) } } trait XCause: LongRegisterName {} impl XCause for mcause::Register {} impl XCause for scause::Register {} impl<R: XCause> XCauseHelpers for ReadWriteRiscvCsr<usize, R> { fn cause(&self) -> Trap { ... } }

And then you'd use it like this:

let trap = CSR.mcause.cause();

That doesn't seem any less ergonomic that what you're proposing, and involves less use of macros (which IMO are less readable than traits/generics).

I didn't think of that. This makes a struct/trait based approach a lot more palatable (although I still like modules more :)), especially because we can reuse some traits between CSRs for different privilege levels.

The tock Readable/Writable traits aren't object safe, so things get annoying if you want to create trait objects from CSRs, which is useful when dealing with a range of identically-defined CSRs (think hpmcounter)

This was another issue I had too. Having to make the array for HPM counters dyn (here) is not ideal (I believe this requires a vtable lookup?). I think that the easiest way around this is to instead provide indexed functions, where the caller just provides an integer for the CSR they want and the function uses a match to call the correct function. As long as the function calls in the match are inlined this should basically become a jump table.

My one remaining issue, however, is with CSRs like the PMPs. If we wanted to use something like ReadWriteRiscvCsr for everything, the PMP created by register_bitfields! would have to look like this, which is terrible. That being said, in 99% of cases the user will likely want to use an indexed function for CSRs like this so they don't have to be exposed to this interface directly. However, that still means the internals aren't going to be very pretty. An easy way around this would be to implement PMPs as a separate type with the exact same functions, so the user wouldn't be aware of the difference without looking at the code/docs. This would only cause an issue if the user wanted to implement some sort of generic function using a ReadWriteRiscvCsr, but that's not something my design makes possible either so I don't think it's a big deal.

Any other thoughts? With what Andrew's pointed out here I'm more willing to go with a struct/trait based setup than before, although I still think a module-based structure makes more sense for CSRs given that they're effectively singletons.

This was another issue I had too. Having to make the array for HPM counters dyn (here) is not ideal (I believe this requires a vtable lookup?). I think that the easiest way around this is to instead provide indexed functions, where the caller just provides an integer for the CSR they want and the function uses a match to call the correct function. As long as the function calls in the match are inlined this should basically become a jump table.

Yeah, it's a vtable lookup. There's definitely room to improve how we've handled the counter CSRs. I like the indexed approach if we care about the generated assembly for the particular CSRs. (It is effectively what Linux does for the counter CSRs, via a mess of cpp macros).

My one remaining issue, however, is with CSRs like the PMPs.

Yes, that's a bit of a mess; we don't have the PMP CSRs in Salus so I've ignored them :)

Given that it's not a pattern that's oft repeated in CSR definitions (CSR divided into independent, identical sub-registers), I would be tempted to skip the register_bitfields! and implement setting of an individual PMP entry via a helper trait.

Yeah, it's a vtable lookup.

Definitely not something we want on firmware, but I think the indexed approach is acceptable for us.

Yes, that's a bit of a mess; we don't have the PMP CSRs in Salus so I've ignored them :)

Can't blame you there.

Given that it's not a pattern that's oft repeated in CSR definitions (CSR divided into independent, identical sub-registers), I would be tempted to skip the register_bitfields! and implement setting of an individual PMP entry via a helper trait.

I think that's fine. I'll look further into a design that uses traits; I had already put some stuff together but got fed up with the problems I had initially and decided to switch. I'll probably push that out as a separate PR so we can review both separately.

abrestic-rivos · 2022-10-18T14:56:53Z

src/register/addresses.rs

+//!
+//! As the CSRs are implemented the `#[allow(unused)] around the respective
+//! index should be removed.
+#[allow(unused)]


Can you put a single #![allow(unused)] at the top of the file to avoid repeating this?

Did not realize I could do that. Will fix.

abrestic-rivos · 2022-10-18T15:20:21Z

src/register/macros.rs

+/// Generates the read functions for a CSR
+macro_rules! csr_reads {


The way it was written in tock/salus by introducing a generic type for RW-CSR (ReadWriteRiscvCsr) and an interface for it and then using that to define a struct CSR for all CSRs in RV seemed like a fine separation. But as you pointed out, there might be other limitations/drawbacks of this. Maybe @abrestic-rivos has more thoughts on this based on his usage of the tock registers.

FWIW, this works pretty well for us (with one exception¹) so if the eventual goal is for Salus to use this too, I'd like to see a compelling reason for why we should change it :)

The CSR-specific-methods issue can be handled by defining and implementing traits for ReadWriteRiscvCsr<CSR> or LocalRegisterCopy<CSR>. For example, if you wanted to define a cause() method like you've done in the subsequent patch, you could do something like:

trait XCauseHelpers { fn cause(&self) -> Trap; fn is_interrupt(&self) -> bool { matches!(self.cause(), Trap::Interrupt(_)) } } trait XCause: LongRegisterName {} impl XCause for mcause::Register {} impl XCause for scause::Register {} impl<R: XCause> XCauseHelpers for ReadWriteRiscvCsr<usize, R> { fn cause(&self) -> Trap { ... } }

And then you'd use it like this:

let trap = CSR.mcause.cause();

That doesn't seem any less ergonomic that what you're proposing, and involves less use of macros (which IMO are less readable than traits/generics).

Footnotes

The tock Readable/Writable traits aren't object safe, so things get annoying if you want to create trait objects from CSRs, which is useful when dealing with a range of identically-defined CSRs (think hpmcounter). ↩

FawazTirmizi self-assigned this Oct 11, 2022

FawazTirmizi force-pushed the dev/fawaz/csr-rewrite branch from c2cc3ce to 351acdf Compare October 11, 2022 22:42

FawazTirmizi force-pushed the rivos/main branch from 4564bfa to bb3945d Compare October 11, 2022 22:48

FawazTirmizi force-pushed the dev/fawaz/csr-rewrite branch 3 times, most recently from 713fe57 to ec6225b Compare October 12, 2022 20:12

FawazTirmizi changed the base branch from rivos/main to dev/fawaz/ci October 12, 2022 20:15

Base automatically changed from dev/fawaz/ci to rivos/main October 14, 2022 16:55

FawazTirmizi marked this pull request as ready for review October 14, 2022 16:55

FawazTirmizi requested review from abrestic-rivos, beezow, dgreid, furquan-rivos, zachreizner, a team and glg-rv October 14, 2022 16:57

ghost reviewed Oct 14, 2022

View reviewed changes

src/register/mcause.rs Show resolved Hide resolved

FawazTirmizi force-pushed the dev/fawaz/csr-rewrite branch from ec6225b to 920d41b Compare October 14, 2022 17:07

FawazTirmizi added 6 commits October 14, 2022 10:51

Added rust-toolchain.toml

f6b4c00

Removed all CSR macros

cf894aa

Because the CSR modules are going to be rewritten, this was done to ensure that the crate can still build while this is happening.

Add module of CSR addresses

374fc4f

Add macros for generating CSR functions

d1365ed

This adds a couple of macros that will create a set of functions for a given CSR. Most of the time, only `ro_csr!` and `rw_csr!` will need to be invoked directly.

Reimplement register::mcause

3910088

Reimplement register::mtvec

ebe2c27

FawazTirmizi force-pushed the dev/fawaz/csr-rewrite branch from 920d41b to ebe2c27 Compare October 14, 2022 17:51

dgreid reviewed Oct 14, 2022

View reviewed changes

beezow reviewed Oct 14, 2022

View reviewed changes

furquan-rivos reviewed Oct 15, 2022

View reviewed changes

abrestic-rivos reviewed Oct 18, 2022

View reviewed changes

		@@ -0,0 +1,678 @@
		//! Addresses for CSRs. Automatically generated by parse_opcodes.

		/// Generates the read functions for a CSR
		macro_rules! csr_reads {

CSR rework #1

Are you sure you want to change the base?

CSR rework #1

Conversation

FawazTirmizi commented Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FawazTirmizi Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FawazTirmizi Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

FawazTirmizi commented Oct 11, 2022 •

edited

Loading

FawazTirmizi Oct 17, 2022 •

edited

Loading

FawazTirmizi Oct 17, 2022 •

edited

Loading