Add Mstatus helpers to allow setting fields in Mstatus #207

jsgf · 2024-05-07T00:52:32Z

Without needing to touch the CSR. This allows multiple changes in a single register write.

romancardenas · 2024-05-07T10:43:27Z

I find it confusing having methods that modify MStatus that do not modify the mstatus register, but a "copy". Alternatively, I like the idea of register builders (see #98). However, the PR looks stale...

Would you consider adapting the builders approach for this PR?

jsgf · 2024-05-07T15:09:23Z

I think there's a few things to pick apart here:

a type representing possible values of mstatus
the actual mstatus csr
updating the csr with a new value

The third one is what adds the complications, because of the special atomic bit set/clear operations in a single csr instruction. These are very useful for all the single-bit fields in mstatus so we wouldn't want to do without them. But in the general case - either updating a multi-bit field or updating multiple fields at once - requires a full read/modify/write.

So as I see it, the current "operate directly on mstatus" methods are the special case exception, and the common case is:

let mut mstatus = mstatus::read();
mstatus = mstatus.set_X().set_Y();
mstatus::write(mstatus);

Would you consider adapting the builders approach for this PR?

I'm not sure how that would really be different? As I said, my use-case here is to read mstatus, make some changes, then put it back, whereas a builder would imply building new Mstatus values from scratch.

But I agree with you about the potential for confusion. I should definitely add a clarifying line in the doc comment to say it updates the Mstatus value but not the csr, and maybe rename them to update_?

jsgf · 2024-05-07T15:17:00Z

I added a commit to:

add clarifying comment
rename the methods from set_ -> update_ to make them more distinct from the mstatus::set_ functions.

jsgf · 2024-05-07T15:59:39Z

Oh I just noticed there's no write_as for updating a csr from a type. And mstatus doesn't have write_as_usize...

jsgf · 2024-05-07T18:28:24Z

Hm, need to do more testing.

jsgf · 2024-05-07T20:53:13Z

Ugh, shift vs & precedence. Looks like this is correct now.

rmsyn · 2024-05-08T00:22:59Z

riscv/src/register/mstatus.rs

+    /// Note this updates the [`Mstatus`] value, but does not affect the mstatus
+    /// CSR itself. See [`set_sie`]/[`clear_sie`] to directly update the CSR.
+    #[inline]
+    pub fn update_sie(&self, sie: bool) -> Self {


Somewhat of a bikeshed, but methods that set a bit in a bitfield without clearing the rest of the bitfield typically use the modify_* convention, e.g. in pac crates generated with svd2rust.

In cortex-m, they use the set_* convention. I would align with them

I started with set but the concern was it would be confusing vs the functions which act directly on the csr. Also there's a semantic difference as the csr functions use parameterleas set/clear for single bit fields whereas the update/modify/... on Mstatus takes a parameter.

romancardenas · 2024-05-08T06:28:54Z

Whenever I have doubts about new features, I take a look at how cortex-m works. For instance, the FPSCR. They do have x methods to get bit fields and set_x methods to set bit fields in the read value. Next, they provide a write function that takes the register structure and writes its value in the register using inline assembly. This looks very similar to your proposal @jsgf

Let me take some time to review your changes, I'll try to provide a review ASAP.

riscv/src/register/macros.rs

romancardenas · 2024-05-08T14:13:04Z

riscv/src/register/mstatus.rs

@@ -72,6 +72,15 @@ impl From<bool> for Endianness {
 }

 impl Mstatus {


It would be nice to have macros for structs like Mstatus, as I expect a lot of duplicates with this

Maybe we could have a pub(crate) bit module somewhere with bitwise functions for usize. In this way, we avoid having methods in every register, and all registers use the same function. I'm not sure if this would help us optimize the binary size or if the compiler is smart enough to collapse all these methods (don't think so).

Yeah I saw a lot of opportunity to collapse some repeated code.

romancardenas

Let's follow the set_* approach as in cortex-m. Also, maybe it is a good moment to add a utility module with bit-wise functions that can be shared among all the registers (it feels like we will need it in the future)

romancardenas · 2024-05-08T14:15:20Z

riscv/src/register/mstatus.rs

+    /// Note this updates the [`Mstatus`] value, but does not affect the mstatus
+    /// CSR itself. See [`set_sie`]/[`clear_sie`] to directly update the CSR.
+    #[inline]
+    pub fn update_sie(&self, sie: bool) -> Self {


In cortex-m, they use the set_* convention. I would align with them

riscv/src/register/mstatus.rs

romancardenas · 2024-05-08T14:28:24Z

riscv/src/register/mstatus.rs

    /// Machine Interrupt Enable
    #[inline]
    pub fn mie(&self) -> bool {
        self.bits & (1 << 3) != 0
    }

+    /// Update Machine Interrupt Enable
+    ///
+    /// Note this updates the [`Mstatus`] value, but does not affect the mstatus


Suggested change

/// Note this updates the [`Mstatus`] value, but does not affect the mstatus

/// Note this updates the [`Mstatus`] value read previously, but does not affect the mstatus

romancardenas · 2024-05-08T14:32:27Z

riscv/src/register/mstatus.rs

@@ -72,6 +72,15 @@ impl From<bool> for Endianness {
 }

 impl Mstatus {


Maybe we could have a pub(crate) bit module somewhere with bitwise functions for usize. In this way, we avoid having methods in every register, and all registers use the same function. I'm not sure if this would help us optimize the binary size or if the compiler is smart enough to collapse all these methods (don't think so).

jsgf · 2024-05-08T19:39:07Z

@romancardenas Yeah, I think a good general structure for any register is to define a type to abstract all the bitfields, read/write operations in terms of that type, and operations on the type to get/set each field. And as an optimization, specific atomic set operations directly on the register if it makes a difference (as it does for riscv single bit fields, and multi-bit in some situations). But it's not really worth having those direct-access functions for anything that's going to require a full read/modify/write pattern anyway.

mstatus is just where I've run into this for my current project, but this pattern applies to pretty much everything in riscv::registers.

For registers which only contain one logical field - eg the pmp_addr registers - you could say that it's probably not necessary to define a new type for it. But that 2 bit shift keeps biting me, so I'd probably still go for it...

Without needing to touch the CSR. This allows multiple changes in a single register write.

And add a clarifying doc comment that this is an operation on Mstatus values, not on the CSR itself.

This adds a macro to generate `write(value:T)` function for writing a structured value to a CSR.

Allow `Mstatus` values to be directly written back to the CSR.

romancardenas

Thanks for the hard work! Have a look to make comments, especially the one regarding the setter approach.

romancardenas · 2024-05-09T06:01:40Z

riscv/src/lib.rs

@@ -36,6 +36,7 @@
 #![allow(clippy::missing_safety_doc)]

 pub mod asm;
+pub(crate) mod bits;


we can leave it as this, but now I don't find any reason why we should hide it from dependents. Maybe PACs find them handy

I'd hold off on that until we're confident it's a stable public api.

romancardenas · 2024-05-09T06:03:12Z

riscv/src/register/mstatus.rs

+    /// Helper to insert a bitfield into Mstatus
+    #[inline]
+    fn bf_insert(&self, bit: usize, width: usize, val: usize) -> Self {
+        Self {
+            bits: bf_insert(self.bits, bit, width, val),
+        }
+    }
+
+    /// Helper to extract a bitfield from Mstatus
+    #[inline]
+    fn bf_extract(&self, bit: usize, width: usize) -> usize {
+        bf_extract(self.bits, bit, width)
+    }
+


Suggested change

/// Helper to insert a bitfield into Mstatus

#[inline]

fn bf_insert(&self, bit: usize, width: usize, val: usize) -> Self {

Self {

bits: bf_insert(self.bits, bit, width, val),

}

}

/// Helper to extract a bitfield from Mstatus

#[inline]

fn bf_extract(&self, bit: usize, width: usize) -> usize {

bf_extract(self.bits, bit, width)

}

I personally prefer using the new utility functions instead of filling all the registers with new methods. But it is not a hard feeling, if you think this approach has advantages, please write them so we can choose the best option.

It's mostly to save on repeating Self { bits: ... } but if we're going with the &mut self approach there's no benefit.

romancardenas · 2024-05-09T06:04:02Z

riscv/src/register/mstatus.rs

@@ -81,46 +96,106 @@ impl Mstatus {
    /// Supervisor Interrupt Enable
    #[inline]
    pub fn sie(&self) -> bool {
-        self.bits & (1 << 1) != 0
+        self.bf_extract(1, 1) != 0


Suggested change

self.bf_extract(1, 1) != 0

bf_extract(self.bits, 1, 1) != 0

romancardenas · 2024-05-09T06:09:30Z

riscv/src/register/mstatus.rs

+    /// update the CSR.
+    #[inline]
+    pub fn set_sie(&self, sie: bool) -> Self {
+        self.bf_insert(1, 1, sie as usize)


Suggested change

self.bf_insert(1, 1, sie as usize)

self.bits = bf_insert(self.bits, 1, 1, sie as usize);

romancardenas · 2024-05-09T06:09:48Z

riscv/src/register/mstatus.rs

+    /// affect the mstatus CSR itself. See [`set_sie`]/[`clear_sie`] to directly
+    /// update the CSR.
+    #[inline]
+    pub fn set_sie(&self, sie: bool) -> Self {


Suggested change

pub fn set_sie(&self, sie: bool) -> Self {

pub fn set_sie(&mut self, sie: bool) {

From cortex-m:

#[inline] pub fn set_n(&mut self, n: bool) { let mask = 1 << 31; match n { true => self.bits |= mask, false => self.bits &= !mask, } }

So let's stick with the classic setter approach with mutable reference and no return value.

What about returning &mut Self so you can chain them?

I like your idea!

jsgf · 2024-05-09T16:25:13Z

BTW there's a lot of repetition here and in other modules - what do you think about a larger scale macro to generate all of this more automatically? It would make it easier to have all the different registers be consistent in form.

(I'm not a huge fan of big macros, but this crate already has a fair amount of macro-driven codegen and I think it tips the cost/benefit scales the right way for this.)

(In a separate PR of course.)

romancardenas

found a 🐛

romancardenas · 2024-05-09T20:24:23Z

riscv/src/register/mstatus.rs

    }

    /// Supervisor Previous Privilege Mode
    #[inline]
    pub fn spp(&self) -> SPP {
-        match self.bits & (1 << 8) != 0 {
+        match bf_extract(self.bits, 7, 1) != 0 {


Suggested change

match bf_extract(self.bits, 7, 1) != 0 {

match bf_extract(self.bits, 8, 1) != 0 {

Ugh oops. Definitely need a macro-based mechanism that can derive all this from a single source of truth.

romancardenas · 2024-05-09T20:27:55Z

I agree with you. We could explore using macros to "ease" the new changes to all the registers

Also remove local bf_* helpers

romancardenas

LGTM

Summary: Bring in: - rust-embedded/riscv#206 - Eq for Permission - rust-embedded/riscv#207 - Mstatus update operations Details of 207 are still under discussion so this will probably need another freshen (and go back to pulling from the riscv repo once its landed). Reviewed By: dtolnay Differential Revision: D57136236 fbshipit-source-id: 09e5365d3fd499c4dccae7d2ada333bf369fac47

jsgf requested a review from a team as a code owner May 7, 2024 00:52

jsgf force-pushed the mstatus-field-set branch 2 times, most recently from f766976 to a10e637 Compare May 7, 2024 01:20

jsgf force-pushed the mstatus-field-set branch from a10e637 to aad8f8d Compare May 7, 2024 15:16

jsgf force-pushed the mstatus-field-set branch 2 times, most recently from 37581f1 to 79d85c6 Compare May 7, 2024 15:33

jsgf force-pushed the mstatus-field-set branch from f1d4f48 to e79fdf7 Compare May 7, 2024 17:06

jsgf force-pushed the mstatus-field-set branch from e79fdf7 to ea489db Compare May 7, 2024 20:22

rmsyn reviewed May 8, 2024

View reviewed changes

romancardenas reviewed May 8, 2024

View reviewed changes

riscv/src/register/macros.rs Show resolved Hide resolved

romancardenas reviewed May 8, 2024

View reviewed changes

romancardenas requested changes May 8, 2024

View reviewed changes

jsgf force-pushed the mstatus-field-set branch from 0789776 to 6b4554a Compare May 8, 2024 19:30

jsgf added 6 commits May 8, 2024 12:39

Add Mstatus helpers to allow setting fields in Mstatus

2f9b748

Without needing to touch the CSR. This allows multiple changes in a single register write.

mstatus: use update_ for names

eaac5cd

And add a clarifying doc comment that this is an operation on Mstatus values, not on the CSR itself.

Add write_csr_as macro

b061e7e

This adds a macro to generate `write(value:T)` function for writing a structured value to a CSR.

Make mstatus::write(Mstatus) available

47ca030

Allow `Mstatus` values to be directly written back to the CSR.

Add some minimal tests

c05838a

Change back to set_ to match cortex and tweak docs a bit

8de908c

jsgf force-pushed the mstatus-field-set branch from 6b4554a to 4d8b361 Compare May 8, 2024 19:43

Add bitfield helpers and use them in mstatus

f968990

jsgf force-pushed the mstatus-field-set branch from 4d8b361 to f968990 Compare May 8, 2024 19:44

romancardenas requested changes May 9, 2024

View reviewed changes

Make set_* take &mut self

a2a9888

Also remove local bf_* helpers

jsgf force-pushed the mstatus-field-set branch from c94d235 to a2a9888 Compare May 9, 2024 22:15

romancardenas approved these changes May 10, 2024

View reviewed changes

romancardenas added this pull request to the merge queue May 12, 2024

Merged via the queue into rust-embedded:master with commit e9f43ae May 12, 2024
95 checks passed

jsgf deleted the mstatus-field-set branch May 12, 2024 16:13

		@@ -72,6 +72,15 @@ impl From<bool> for Endianness {
		}

		impl Mstatus {

	/// Note this updates the [`Mstatus`] value, but does not affect the mstatus
	/// Note this updates the [`Mstatus`] value read previously, but does not affect the mstatus

	self.bf_insert(1, 1, sie as usize)
	self.bits = bf_insert(self.bits, 1, 1, sie as usize);

	pub fn set_sie(&self, sie: bool) -> Self {
	pub fn set_sie(&mut self, sie: bool) {

	match bf_extract(self.bits, 7, 1) != 0 {
	match bf_extract(self.bits, 8, 1) != 0 {

Add Mstatus helpers to allow setting fields in Mstatus #207

Add Mstatus helpers to allow setting fields in Mstatus #207

Conversation

jsgf commented May 7, 2024

romancardenas commented May 7, 2024

jsgf commented May 7, 2024

jsgf commented May 7, 2024

jsgf commented May 7, 2024

jsgf commented May 7, 2024

jsgf commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romancardenas commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romancardenas left a 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

jsgf commented May 8, 2024

romancardenas left a 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

jsgf commented May 9, 2024 • edited Loading

romancardenas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romancardenas commented May 9, 2024

romancardenas left a comment

Choose a reason for hiding this comment

jsgf commented May 9, 2024 •

edited

Loading