Closed jsgf closed 3 months ago
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?
I think there's a few things to pick apart here:
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_
?
I added a commit to:
mstatus::set_
functions.Oh I just noticed there's no write_as
for updating a csr from a type. And mstatus doesn't have write_as_usize...
Hm, need to do more testing.
Ugh, shift vs &
precedence. Looks like this is correct now.
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.
@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...
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.)
I agree with you. We could explore using macros to "ease" the new changes to all the registers
Without needing to touch the CSR. This allows multiple changes in a single register write.