riscv: define mstatus using CSR macro

[rmsyn]

Adds an optional field documentation macro argument to allow adding documentation to CSR field enum variants.

Current macros for CSR field enum accessors allow for the MSB and LSB to hold the same value (for single-bit enum fields).

Clippy defaults to deny(clippy::eq_op). The simplest solution is to allow the lint. A more robust solution is to special-case equal operands with new macro arm.

Uses CSR macro helpers to define the mstatus CSR register.

Adds a CSR test field macro arm for enum fields.

Adds more unit test cases for Mstatus field accessors.

[rmsyn]

Missed renaming the test-case. Should probably change to test_mstatus for clarity, and will update with any review changes requested.

[romancardenas]

minor changes. Thanks!

[romancardenas]

Suggested change

	///
	/// **NOTE**: panics on RISCV-32 platforms.

[romancardenas]

Suggested change

	#[inline]
	///
	/// **NOTE**: panics on RISCV-32 platforms.
	#[inline]

[romancardenas]

Suggested change

	#[inline]
	///
	/// **NOTE**: panics on RISCV-32 platforms.
	#[inline]

[romancardenas]

Maybe in this kind of methods, for RV32, we should redirect users to use mstatush

[rmsyn]

we should redirect users to use mstatush

I agree, that would be helpful. I'll add it for sbe and mbe. These are the only two fields with overlap, right?

[rmsyn]

we should redirect users to use mstatush

So, I just use the free functions in mstatush here, and for try_set_mbe. These are kind of a special-case.

I think when I make the changes for mstatush, I'll convert these functions to consume the Mstatus register, and return an Mstatush instance with the relevant field set.

It may be overengineering at that point, though. Especially since there are only these two fields. It may make more sense to just handle it internally here with:

let mut m = mstatush::read();
m.set_sbe(endianness);
mstatush::write(m);
Ok(())

I removed the panic note, because these try_set functions aren't fallible now. It may even make sense to remove the try_ variants.

What do you think @romancardenas?

[romancardenas]

I am not sure about this because there is a difference in behavior between RV64 and RV32. In RV64, you modify a register copy that will later be applied to the real register. However, in RV32, you are directly modifying the register.

We can either:

1. Use the read_composite_csr! macro to deal with mstatus and mstatush together (check mcycle, for instance)
2. Just panic in mstatus and refer to mstatush in the docs

[rmsyn]

Use the read_composite_csr! macro to deal with mstatus and mstatush together (check mcycle, for instance)

So, the read_{m,s}be is already handled, and there is no corresponding write_composite_csr!.

However, in RV32, you are directly modifying the register.

I also had issue with this, which is why I suggested a function that consumes Mstatus, and returns an Mstatus and Mstatush instance. It would look something like:

pub fn set_mbe(self, endianness: Endianness) -> (Mstatus, Mstatush) {
    match () {
        #[cfg(not(target_arch = "riscv32"))]
        () => (
            Mstatus { bits: bf_insert(self.bits, 37, 1, endianness as usize) },
            Mstatush::new()
        ),
        #[cfg(target_arch = "riscv32")]
        () => (
            self,
            Mstatush { bits: bf_insert(0, 5, 1, endianness as usize },
        )
    }
}

Alternatively, to have a more idiomatic set function:

pub fn set_mbe(&mut self, endianness: Endianness) -> Mstatush {
    match () {
        #[cfg(not(target_arch = "riscv64"))]
        () => {
            self.bits = bf_insert(self.bits, 37, 1, endianness as usize);
            Mstatush::new()
        }
        #[cfg(target_arch = "riscv32")]
        () => Mstatush { bits: bf_insert(0, 5, 1, endianness as usize },
    }
}

It's a little cumbersome, but it avoids an unnecessary panic.

We could also explain the slightly awkward interface in the documentation, and point RISCV-32 users to Mstatush.

Just panic in mstatus and refer to mstatush in the docs

I would truly like to avoid as many runtime panics as possible.

[romancardenas]

The thing is that Mstatush does not exist in RV64, so returning a non-existing register does not make sense for these targets. A regular setter works.

In RV32, you cannot set MBE nor SBE, as these fields do not belong to Mstatus, but to Mstatush. Thus, I am more inclined to return an Unimplemented error than provide a weird API that returns another register. I would rather write in the docs that in RV64 these methods are infallible but in RV32 they fail, as the MSTATUS register does not include those fields.

We can not include the setters for RISCV32 and only leave them for RV64. While the API will be slightly different depending on your target, well, this is in fact different depending on your target and we are making this difference explicit.

Note that, even if we remove these methods for RV32, we should add a brief explanation in the docs to make users aware of it. After all, docs.rs is built for a RV64 target.

[rmsyn]

The thing is that Mstatush does not exist in RV64, so returning a non-existing register does not make sense for these targets. A regular setter works.

Right, which is why it returns the Option. On RISCV-64, it always returns None. Regardless, it is a pretty wonky setter API, so I'll change it for consistency.

We can not include the setters for RISCV32 and only leave them for RV64. While the API will be slightly different depending on your target, well, this is in fact different depending on your target and we are making this difference explicit.

I like this approach, because other than the platform difference, there is no reason to have a try variant (it's an infallible API). Also, any users targetting RISCV-64 and RISCV-32 with the same code base will get an explicit "this function does not exist" error at compile-time (instead of the runtime error/panic).

[romancardenas]

I like the idea of adding these methods to RV32 but only to throw a compile-time error redirecting users to mstatush!

[rmsyn]

I like the idea of adding these methods to RV32 but only to throw a compile-time error redirecting users to mstatush!

So, I tried using a compile_error macro on the RV32 arm of the relevant functions, but that just causes the RV32 build to fail. Not sure how to produce a compile-error-on-use for a function. In other words, define the function for RV64 and RV32, but produce a compile-time error if RV32 users try to use the function.

The best I can think of is not defining the function for RV32 targets, and provide a note in the docs pointing users to Mstatush (which is what the current set of changes does). The other alternative is defining the function, but always returning the Unimplemented error.

[romancardenas]

I think the best approach is to not define these methods for RV32, as you did in your latest commit.

Maybe we can do the same with set_uxl and set_sxl? They are in a very similar situation. The only difference is that these methods are not part of another CSR, they just do not exist.

[rmsyn]

Maybe we can do the same with set_uxl and set_sxl?

Alright, I added a commit with those changes. I left the getters, because it makes sense semantically to return the fixed XLEN values on RV32 (even if the fields technically don't exist).