Prepare for 64 PMP config registers #1926

alistair23 · 2020-06-10T19:10:23Z

Pull Request Overview

This PR was supposed to add support for 64 PMP config register on RISC-V now that it is supported by the spec.

Instead this patch has turned more info converting the riscv-csr library to use the new inline assembly macro and to allow dynamic array access to the RISC-V registers.

This PR does a few things:

Converts the PMP CSRs to an array (to allow simplifying the PMP code)
Uses the new inline assembly. Unfortunately Rust won't let use access CSR dynamically like we used to (see Inline assembly Unable to dynamically access RISC-V CSRs rust-lang/rust#73220) so we now have a giant if statement to see which one to access. I couldn't see any other way to make the value const.
We also implement the register functions on specific types (u32) instead of generics. This is because with generics we can't dynamically access arrays of CSRs with the inline assembly.
Finally we clean up some of the PMP code.

Testing Strategy

None yet.

TODO or Help Wanted

Comments please, this is uglier then I would like it to be.

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

a-pronin · 2020-06-13T02:02:32Z

arch/rv32i/src/pmp.rs


                                // Set access to end address
-                                csr::CSR.pmpcfg1.set(cfg_val << 8 | csr::CSR.pmpcfg0.get());
-                                csr::CSR.pmpaddr5.set((start as u32 + size as u32) >> 2);
+                                csr::CSR.pmpcfg[1].set(cfg_val << 8 | csr::CSR.pmpcfg[0].get());


should be pmpcfg[1]
please see all places where changes are needed in #1932

a-pronin

Let's merge in the changes from #1932

gendx

Just a couple comments that are not 100% related to this change, but it seems to me that there is some overlap with what's available in libraries/tock-register-interface. Are there any limitations to the tock-register-interface that prevent using it here?

gendx · 2020-06-15T12:35:43Z

libraries/riscv-csr/src/csr.rs

    #[cfg(not(any(target_arch = "riscv32", target_os = "none")))]
-    pub fn get(&self) -> T {
+    pub fn get(&self) -> u32 {
        unimplemented!("reading RISC-V CSR {}", self.value)
    }

    #[cfg(not(any(target_arch = "riscv32", target_os = "none")))]
-    pub fn set(&self, _val_to_set: T) {
+    pub fn set(&self, _val_to_set: u32) {
        unimplemented!("writing RISC-V CSR {}", self.value)
    }

    #[inline]
-    pub fn read(&self, field: Field<T, R>) -> T {
+    pub fn read(&self, field: Field<u32, R>) -> u32 {
        (self.get() & (field.mask << field.shift)) >> field.shift
    }

    #[inline]
-    pub fn read_as_enum<E: TryFromValue<T, EnumType = E>>(&self, field: Field<T, R>) -> Option<E> {
-        let val: T = self.read(field);
+    pub fn read_as_enum<E: TryFromValue<u32, EnumType = E>>(
+        &self,
+        field: Field<u32, R>,
+    ) -> Option<E> {
+        let val: u32 = self.read(field);

        E::try_from(val)
    }

    #[inline]
-    pub fn extract(&self) -> LocalRegisterCopy<T, R> {
+    pub fn extract(&self) -> LocalRegisterCopy<u32, R> {
        LocalRegisterCopy::new(self.get())
    }

    #[inline]
-    pub fn write(&self, field: FieldValue<T, R>) {
+    pub fn write(&self, field: FieldValue<u32, R>) {
        self.set(field.value);
    }

    #[inline]
-    pub fn modify(&self, field: FieldValue<T, R>) {
-        let reg: T = self.get();
+    pub fn modify(&self, field: FieldValue<u32, R>) {
+        let reg: u32 = self.get();
        self.set((reg & !field.mask) | field.value);
    }

    #[inline]
-    pub fn modify_no_read(&self, original: LocalRegisterCopy<T, R>, field: FieldValue<T, R>) {
+    pub fn modify_no_read(&self, original: LocalRegisterCopy<u32, R>, field: FieldValue<u32, R>) {
        self.set((original.get() & !field.mask) | field.value);
    }
    #[inline]
-    pub fn is_set(&self, field: Field<T, R>) -> bool {
-        self.read(field) != T::zero()
+    pub fn is_set(&self, field: Field<u32, R>) -> bool {
+        self.read(field) != u32::zero()
    }

    #[inline]
-    pub fn matches_any(&self, field: FieldValue<T, R>) -> bool {
-        self.get() & field.mask != T::zero()
+    pub fn matches_any(&self, field: FieldValue<u32, R>) -> bool {
+        self.get() & field.mask != u32::zero()
    }

    #[inline]
-    pub fn matches_all(&self, field: FieldValue<T, R>) -> bool {
+    pub fn matches_all(&self, field: FieldValue<u32, R>) -> bool {
        self.get() & field.mask == field.value
    }
 }


All of these functions look very similar to what's already available in libraries/tock-register-interface/src/registers.rs. Is there any reason not to use that?

Reading a CSR requires an assembly instruction, not accessing a memory location.

A lot of the logic still overlaps with methods such as FieldValue::matches_all. The only difference that I see is in the implementation of the set and get methods - all of the other methods are derived from it in the same manner as in the libraries/tock-register-interface library.

It looks like the ReadAble and WriteAble traits that I sketched in #1766 (comment) could be beneficial here as well.

gendx · 2020-06-15T12:37:13Z

arch/rv32i/src/csr/mod.rs

    pub minstret: ReadWriteRiscvCsr<u32, minstret::minstret::Register>,
    pub mcycleh: ReadWriteRiscvCsr<u32, mcycle::mcycleh::Register>,
    pub mcycle: ReadWriteRiscvCsr<u32, mcycle::mcycle::Register>,
-    pub pmpcfg0: ReadWriteRiscvCsr<u32, pmpconfig::pmpcfg::Register>,
-    pub pmpcfg1: ReadWriteRiscvCsr<u32, pmpconfig::pmpcfg::Register>,
-    pub pmpcfg2: ReadWriteRiscvCsr<u32, pmpconfig::pmpcfg::Register>,
-    pub pmpcfg3: ReadWriteRiscvCsr<u32, pmpconfig::pmpcfg::Register>,
-    pub pmpaddr0: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr1: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr2: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr3: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr4: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr5: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr6: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr7: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr8: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr9: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr10: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr11: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr12: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr13: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr14: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
-    pub pmpaddr15: ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>,
+    pub pmpcfg: [ReadWriteRiscvCsr<u32, pmpconfig::pmpcfg::Register>; 4],
+    pub pmpaddr: [ReadWriteRiscvCsr<u32, pmpaddr::pmpaddr::Register>; 16],
    pub mie: ReadWriteRiscvCsr<u32, mie::mie::Register>,
    pub mscratch: ReadWriteRiscvCsr<u32, mscratch::mscratch::Register>,
    pub mepc: ReadWriteRiscvCsr<u32, mepc::mepc::Register>,


It looks like this definition could use the register_structs macro defined in libraries/tock-register-interface/src/macros.rs. Is there any reason not to use it?

Alistair is just updating what is already there, I think it is fine to not add additional changes.

Note: my question was beyond the scope of this PR, to see if there's any improvement that could be made in libraries/tock-register-interface to improve adoption of this library, not only in chips/ but in arch/ as well.

ppannuto · 2020-06-17T17:09:04Z

bors d=alistair23,gendx

bors · 2020-06-17T17:09:08Z

✌️ alistair23 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

bors · 2020-06-17T17:09:09Z

✌️ gendx can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

1939: Make the Field::mask and FieldValue::mask fields private. r=alistair23 a=gendx ### Pull Request Overview The `mask` fields don't need to be exposed publicly, as their use cases overlap with the existing functions to manipulate fields. - This pull request makes them private. - As a side effect, the `libraries/riscv-csr/src/csr.rs` is refactored to use the unified arithmetic methods introduced in #1766. ### Testing Strategy This pull request was tested by CI. ### TODO or Help Wanted - These changes currently overlap with #1926, although are compatible with that PR, as discussed in #1926 (comment). - The `Field::shift` field should also be made private, but is currently used in `chips/lowrisc/src/gpio.rs`. That code should probably be refactored so that the shift is not exposed as-is. https://github.com/tock/tock/blob/555b3a0441697d111a0330c01d392c15b36712f6/chips/lowrisc/src/gpio.rs#L94-L104 - Likewise, the `FieldValue::value` field should be made private, but is currently used in https://github.com/tock/tock/blob/555b3a0441697d111a0330c01d392c15b36712f6/libraries/riscv-csr/src/csr.rs#L76-L79 ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Guillaume Endignoux <guillaumee@google.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

We need to manually split these out in order to access them from inline assembly as part of the next patch. Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

alistair23 · 2020-06-17T18:23:35Z

bors r+

alistair23 · 2020-06-17T18:39:39Z

bors r+

hudson-ayers · 2020-06-17T19:24:45Z

bors r+

bors · 2020-06-17T19:32:47Z

Build succeeded:

bradjc added the WG-OpenTitan In the purview of the OpenTitan working group. label Jun 11, 2020

alistair23 force-pushed the alistair/pmp-64 branch from 985026a to 11c9a25 Compare June 11, 2020 15:37

bradjc previously approved these changes Jun 12, 2020

View reviewed changes

alistair23 mentioned this pull request Jun 12, 2020

[Tracking] Convert from llvm_asm!() to new asm!() #1923

Closed

11 tasks

a-pronin mentioned this pull request Jun 13, 2020

rv32i: fix PMP configuration #1932

Closed

a-pronin reviewed Jun 13, 2020

View reviewed changes

a-pronin suggested changes Jun 13, 2020

View reviewed changes

gendx reviewed Jun 15, 2020

View reviewed changes

gendx mentioned this pull request Jun 15, 2020

Make the Field::mask and FieldValue::mask fields private. #1939

Merged

2 tasks

ppannuto mentioned this pull request Jun 17, 2020

[arch, rv32i] Compilation error with "opt-level" set to 0 or 1 #1948

Closed

alistair23 added 5 commits June 17, 2020 10:45

rv32i: pmp: Convert the CSRs to an array

03e8507

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

lib: riscv-csr: Convert to use the new inline asm

4d4b01e

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

riscv-csr: Implement register sizes manually

cf88e0a

We need to manually split these out in order to access them from inline assembly as part of the next patch. Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

rv32i: pmp: Split out the pmpaddr set from the match

a7b1800

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

rv32i: pmp: Reduce the disable match length

a40c3ff

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

alistair23 dismissed bradjc’s stale review via 464d3f2 June 17, 2020 17:56

alistair23 force-pushed the alistair/pmp-64 branch 2 times, most recently from 464d3f2 to a40c3ff Compare June 17, 2020 17:58

hudson-ayers approved these changes Jun 17, 2020

View reviewed changes

bors bot merged commit dbe8af5 into tock:master Jun 17, 2020

alistair23 deleted the alistair/pmp-64 branch June 22, 2020 15:53

hudson-ayers mentioned this pull request Mar 8, 2021

riscv: Use macros to create non-PMP CSRs #2470

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for 64 PMP config registers #1926

Prepare for 64 PMP config registers #1926

alistair23 commented Jun 10, 2020

a-pronin Jun 13, 2020

a-pronin left a comment

gendx left a comment

gendx Jun 15, 2020

bradjc Jun 15, 2020

gendx Jun 15, 2020

gendx Jun 15, 2020

bradjc Jun 15, 2020

gendx Jun 15, 2020

ppannuto commented Jun 17, 2020

bors bot commented Jun 17, 2020

bors bot commented Jun 17, 2020

alistair23 commented Jun 17, 2020

alistair23 commented Jun 17, 2020

hudson-ayers commented Jun 17, 2020

bors bot commented Jun 17, 2020

Prepare for 64 PMP config registers #1926

Prepare for 64 PMP config registers #1926

Conversation

alistair23 commented Jun 10, 2020

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

a-pronin Jun 13, 2020

Choose a reason for hiding this comment

a-pronin left a comment

Choose a reason for hiding this comment

gendx left a comment

Choose a reason for hiding this comment

gendx Jun 15, 2020

Choose a reason for hiding this comment

bradjc Jun 15, 2020

Choose a reason for hiding this comment

gendx Jun 15, 2020

Choose a reason for hiding this comment

gendx Jun 15, 2020

Choose a reason for hiding this comment

bradjc Jun 15, 2020

Choose a reason for hiding this comment

gendx Jun 15, 2020

Choose a reason for hiding this comment

ppannuto commented Jun 17, 2020

bors bot commented Jun 17, 2020

bors bot commented Jun 17, 2020

alistair23 commented Jun 17, 2020

alistair23 commented Jun 17, 2020

hudson-ayers commented Jun 17, 2020

bors bot commented Jun 17, 2020