add a bunch of setter for mstatus

[jwnhy]

As the inner structure of Mstatus is private, it's inconvient to prepare/keep track of a mstatus for a certain kind of supervisor runtime.

Especially when prepare for mstatus, if the interrupt bit like MIE is enabled, then a interrupt maybe triggered unwantedly.

So I think it's maybe a good idea to add some setter for these fields.

But I do concern about the amibigiouty of these instance method comparing to the register setters.

[almindor]

I'm very rusty atm (pun not intended) not being working on these things for some time, but shouldn't these setters be unsafe to not hide the fact?

[jwnhy]

emmm, these does not actually change the mstatus register, these just change the bits field in the Mstatus structure.
I do not think an unsafe is necessary.

I'm not sure if I followed you correctly, could you please elaborate?

[jwnhy]

This is for the use case like, you want to store mstatus in a context, but you do not want to actually change the mstatus register.

So you can set up Mstatus struct and use asmebly or perhaps a new function to write the Mstatus into the mstatus register later.

[almindor]

Ah I see, your original statement But I do concern about the amibigiouty of these instance method comparing to the register setters. makes perfect sense now :D I guess I fell into that trap nicely.

[jwnhy]

XD, So do you think this is a good add-on?
I'm not so sure, but I do feel this useful and makes my code more clear when writing code.
After all, without this, you can only write something like

let later_mstatus = flag_A | flag_B;
/* a bunch of code */
asm!("
    csrw mstatus, a0
    mret
")

which is tedious, and too much like C.
Perhaps we need a better naming for this?

[almindor]

I think we need to de-confuse this somehow.

Right now the mstatus::set_mpp(MPP) and your mstatus::set_mpp(&mut self, MPP) are just too confusing (just to provide an example).

Maybe the methods that don't apply should be called something else, more obvious to keep things clear. I'm not quite sure about the naming tho. Perhaps bitset instead of set prefix?

[jwnhy]

I think bitset is pretty good, as it is similar to the bit_field naming.

I also added a new method called apply to apply the changes in self.bits to the actual mstatus register and added a bunch of comments in the latest commit.

If this is ok, I'd like to add this bitset for other registers, like mideleg and medeleg.

[almindor]

@Disasm any strong opinions? I'm inclined to think this is good now

[Disasm]

I think these methods are confusing as they don't modify the CSR until you call apply() and you can easily forget to call it (look at the example in your very first message). I'd rather use mstatus::set_<something>() for the setters for two reasons:

1. consistency with the rest of the riscv crate
2. these functions have immediate effect

[jwnhy]

emmmm, I hope to have something that will not take immediate effect in the mstatus register.
For example, I hope to run multiple supervisor kernels on the same machine, each of them may need different mstatus.
I just want a way to store these Mstatus and let them take effect only after these kernels are up.

[jwnhy]

@Disasm
I just exactly want to avoid immediate effect, so that I can store configuration somewhere in memory.
And swap it up when running supervisor programs.
For example, I may run two individual kernel, A, B. They need different mstatus setting.
With this PR, I can init and store configuration of mstatus for A and B in their context without taking immediate effect, like mstatus.MIE may interrupt with machine mode program.

[Disasm]

Sorry, I don't understand your use case. Why do you need to store the mstatus value for a long time? Why not set all the bits when you need it? And if you need to save/restore mstatus, you don't even need the setters, you can save the raw value.

[jwnhy]

@Disasm
Because I may need multiple supervisor programs. Actually I am working on a trusted execution environment project. And I hope to have not only Linux but also other small kernels running on the same machine. So when I want to swap between Linux and another kernel, I will need to change the mstatus.

Yes, I can save the raw value but it will look like

let mstatus = Flag_A | Flag_B;

and that is what I hope to avoid as it is easy to make mistake setting these raw values.

[Disasm]

I'm not sure we should use bitset_ suffix instead of set_ because, for example, bitset_fs sets two bits.

[jwnhy]

I don't know, is there similar cases like ours?
I think bit set here means to set the bit inside the mstatus, not exactly one bit.
But I am not a native speaker...so I'm not quite sure.

[jwnhy]

Is flagset_ a proper prefix? What's you guy's opinion?

[almindor]

Is flagset_ a proper prefix? What's you guy's opinion?

What if we introduce a MstatusValue newtype (or a better name, can't come up with something nice tbh.) that has From<Mstatus> as well as Default implementations, does not touch Mstatus itself and provides all the nice setters and getters needed?

We won't need to change Mstatus itself at all apart from a basic setter from value, and still get all that's needed. You can then just do MstatusValue::from(mstatus::read()) (or ::default() for empty) and set all you need. Once done, just call mstatus::set_value(mstatus_value) to apply (or call it apply perhaps).

[jwnhy]

Is flagset_ a proper prefix? What's you guy's opinion?

What if we introduce a MstatusValue newtype (or a better name, can't come up with something nice tbh.) that has From<Mstatus> as well as Default implementations, does not touch Mstatus itself and provides all the nice setters and getters needed?

We won't need to change Mstatus itself at all apart from a basic setter from value, and still get all that's needed. You can then just do MstatusValue::from(mstatus::read()) (or ::default() for empty) and set all you need. Once done, just call mstatus::set_value(mstatus_value) to apply (or call it apply perhaps).

Great Idea, I'll see what I can do.

[jwnhy]

Is flagset_ a proper prefix? What's you guy's opinion?

What if we introduce a MstatusValue newtype (or a better name, can't come up with something nice tbh.) that has From<Mstatus> as well as Default implementations, does not touch Mstatus itself and provides all the nice setters and getters needed?

We won't need to change Mstatus itself at all apart from a basic setter from value, and still get all that's needed. You can then just do MstatusValue::from(mstatus::read()) (or ::default() for empty) and set all you need. Once done, just call mstatus::set_value(mstatus_value) to apply (or call it apply perhaps).

I'm not sure if it changes too much, if we take this approach, the Mstatus no longer need to contains any value.
the Mstatus::read() can just return a MstatusValue instead of Mstatus to avoid confusion.

But I do concern this will have compatibility issue.

[duskmoon314]

MstatusValue sounds great.

May be a PR for other registers like sstatus xip xie is also needed.

[duskmoon314]

I'm not sure if it changes too much, if we take this approach, the Mstatus no longer need to contains any value.
the Mstatus::read() can just return a MstatusValue instead of Mstatus to avoid confusion.

But I do concern this will have compatibility issue.

It might not be a good idea to remove bits from Mstatus due to compatibility. I think implement From trait for Mstatus and MstatusValue might be less disruptive. Maybe something like:

let mut mstatus_value: MstatusValue = mstatus::read().into();
mstatus_value.set_mie();
mstatus::write(mstatus_value.into());

[almindor]

I'm not sure if it changes too much, if we take this approach, the Mstatus no longer need to contains any value.
the Mstatus::read() can just return a MstatusValue instead of Mstatus to avoid confusion.
But I do concern this will have compatibility issue.

It might not be a good idea to remove bits from Mstatus due to compatibility. I think implement From trait for Mstatus and MstatusValue might be less disruptive. Maybe something like:

let mut mstatus_value: MstatusValue = mstatus::read().into();
mstatus_value.set_mie();
mstatus::write(mstatus_value.into());

Yes that's what I was aiming for. As a side note, maybe name it MStatusBuilder? I'm still not sure what a really "good" name would be. I'm also a bit concerned with consistency of this pattern wrt. other CSRs.

[jwnhy]

I'm not sure if it changes too much, if we take this approach, the Mstatus no longer need to contains any value.
the Mstatus::read() can just return a MstatusValue instead of Mstatus to avoid confusion.
But I do concern this will have compatibility issue.

It might not be a good idea to remove bits from Mstatus due to compatibility. I think implement From trait for Mstatus and MstatusValue might be less disruptive. Maybe something like:

let mut mstatus_value: MstatusValue = mstatus::read().into();
mstatus_value.set_mie();
mstatus::write(mstatus_value.into());

Yes that's what I was aiming for. As a side note, maybe name it MStatusBuilder? I'm still not sure what a really "good" name would be. I'm also a bit concerned with consistency of this pattern wrt. other CSRs.

If it's for compatibility, then I think MstatusBuilder is more suitable. BTW, we may need a macro to auto-generate these stuff?

[jwnhy]

I will rework on this issue.
Aiming two goals

1. Add MstatusBuilder-like structure to store these information
2. Keep this clear and consistent wrt other register.