Merge #1985

1985: Update Systick trait, implement it for e310x and ibex r=bradjc a=hudson-ayers

### Pull Request Overview

This PR updates the SysTick trait to be less specific to the ARM systick peripheral, to instead represent a System Timer abstraction that can be more effectively used by chips without this optional peripheral. It also adds an implementation of this trait for two RISC-V chips, ibex and the e310x, by virtualizing the single available hardware timer available on each such that it can be used by both the scheduler and by kernel capsules/userspace apps.

This includes renaming SysTick to SystemTimer (name subject to bikeshedding) and modifying the functions and documentation of the trait slightly. This PR also removes the reliance on a dedicated interrupt handler for system timer interrupts, instead relying on there simply being some interrupt that fires, and calling systick.expired() whenever userspace processes are interrupted to determine if there has been a timeslice expiration. This change is critical for enabling the ability to implement the system timer on top of a hardware peripheral that is shared with other timer functionality.

This PR also includes a new `VirtualSystemTimer` type which can be constructed by passing anything implementing the `Alarm` trait, allowing for `VirtualMuxAlarm`s to be used to create a systick. Notably, the design of `MuxAlarm` is such that there is still constant-time overhead on all systick operations except for handling the systick interrupt when it fires (which is O(N) where N is the number of virtual alarms). This seems like a reasonable tradeoff to me, given that systick expirations already lead to the 
higher overhead of a context switch, and a well designed system (and scheduler) should minimize the frequency of app preemption.

This PR moves opentitan and the hifive1 to use the new `VirtualSystemTimer`.

This PR also removes the reliance of the ARM systick on modifying a global static mut variable to signal timeslice expirations.

Finally, this PR also implements `SystemTimer` directly on the RISC-V mtimer, but this will only work if a different hardware timer is used to provide alarms to the kernel/userspace, and this implementation is untested given that we do not support doing so on any of our current RISC-V chips.


### Testing Strategy

This pull request (well, a slightly earlier version) was tested on HiFive1 in QEMU and appeared to work (apps still functioned, and putting a busy loop in apps led to timeslice expirations that preempted the app).

I also tested it on OpenTitan in verilator, but have run into issues where after a timeslice expirations I receive spurious interrupts. I still need to debug this to determine whether it is an issue with this PR or the tock implementation of `rv_timer()`.

I still need to test on existing ARM boards.


### TODO or Help Wanted

This pull request still needs feedback on names, and feedback on the changes to the trait. 
I learned today that @rajivr has also been working on modifying the SysTick trait, and he has taken a somewhat different approach, so it may be useful to compare and contrast these approaches.

More testing will be needed.


### Documentation Updated

- [ ] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make prepush`.


Co-authored-by: Hudson Ayers <hayers@stanford.edu>
Co-authored-by: Brad Campbell <bradjc5@gmail.com>

arch/cortex-m/src/syscall.rs

@@ -21,13 +21,6 @@ pub static mut SYSCALL_FIRED: usize = 0;
 #[used]
 pub static mut APP_HARD_FAULT: usize = 0;
 
-/// This is called in the systick handler. When set to 1 this means the process
-/// exceeded its timeslice. Marked `pub` because it is used in the cortex-m*
-/// specific handler.
-#[no_mangle]
-#[used]
-pub static mut SYSTICK_EXPIRED: usize = 0;
-
 /// This is used in the hardfault handler. When an app faults, the hardfault
 /// handler stores the value of the SCB registers in this static array. This
 /// makes them available to be displayed in a diagnostic fault message.
@@ -157,10 +150,6 @@ impl kernel::syscall::UserspaceKernelBoundary for SysCall {
         let syscall_fired = read_volatile(&SYSCALL_FIRED);
         write_volatile(&mut SYSCALL_FIRED, 0);
 
-        // Check to see if the systick timer for the process expired.
-        let systick_expired = read_volatile(&SYSTICK_EXPIRED);
-        write_volatile(&mut SYSTICK_EXPIRED, 0);
-
         // Now decide the reason based on which flags were set.
         let switch_reason = if app_fault == 1 {
             // APP_HARD_FAULT takes priority. This means we hit the hardfault
@@ -195,8 +184,6 @@ impl kernel::syscall::UserspaceKernelBoundary for SysCall {
                 Some(s) => kernel::syscall::ContextSwitchReason::SyscallFired { syscall: s },
                 None => kernel::syscall::ContextSwitchReason::Fault,
             }
-        } else if systick_expired == 1 {
-            kernel::syscall::ContextSwitchReason::TimesliceExpired
         } else {
             // If none of the above cases are true its because the process was interrupted by an
             // ISR for a hardware event

arch/cortex-m/src/systick.rs

@@ -116,8 +116,8 @@ impl SysTick {
     }
 }
 
-impl kernel::SysTick for SysTick {
-    fn set_timer(&self, us: u32) {
+impl kernel::SchedulerTimer for SysTick {
+    fn start(&self, us: u32) {
         let reload = {
             // We need to convert from microseconds to native tics, which could overflow in 32-bit
             // arithmetic. So we convert to 64-bit. 64-bit division is an expensive subroutine, but
@@ -128,31 +128,26 @@ impl kernel::SysTick for SysTick {
 
             hertz * us / 1_000_000
         };
+        let clock_source: FieldValue<u32, self::ControlAndStatus::Register> = if self.external_clock
+        {
+            // CLKSOURCE 0 --> external clock
+            ControlAndStatus::CLKSOURCE::CLEAR
+        } else {
+            // CLKSOURCE 1 --> internal clock
+            ControlAndStatus::CLKSOURCE::SET
+        };
 
         // n.b.: 4.4.5 'hints and tips' suggests setting reload before value
         SYSTICK_BASE
             .syst_rvr
             .write(ReloadValue::RELOAD.val(reload as u32));
         SYSTICK_BASE.syst_cvr.set(0);
+        SYSTICK_BASE
+            .syst_csr
+            .write(ControlAndStatus::ENABLE::SET + clock_source);
     }
 
-    fn greater_than(&self, us: u32) -> bool {
-        let tics = {
-            // We need to convert from microseconds to native tics, which could overflow in 32-bit
-            // arithmetic. So we convert to 64-bit. 64-bit division is an expensive subroutine, but
-            // if `us` is a power of 10 the compiler will simplify it with the 1_000_000 divisor
-            // instead.
-            let us = us as u64;
-            let hertz = self.hertz() as u64;
-
-            (hertz * us / 1_000_000) as u32
-        };
-
-        let value = SYSTICK_BASE.syst_cvr.read(CurrentValue::CURRENT);
-        value > tics
-    }
-
-    fn overflowed(&self) -> bool {
+    fn has_expired(&self) -> bool {
         SYSTICK_BASE.syst_csr.is_set(ControlAndStatus::COUNTFLAG)
     }
 
@@ -162,7 +157,7 @@ impl kernel::SysTick for SysTick {
         SYSTICK_BASE.syst_cvr.set(0);
     }
 
-    fn enable(&self, with_interrupt: bool) {
+    fn arm(&self) {
         let clock_source: FieldValue<u32, self::ControlAndStatus::Register> = if self.external_clock
         {
             // CLKSOURCE 0 --> external clock
@@ -172,14 +167,36 @@ impl kernel::SysTick for SysTick {
             ControlAndStatus::CLKSOURCE::SET
         };
 
-        if with_interrupt {
-            SYSTICK_BASE.syst_csr.write(
-                ControlAndStatus::ENABLE::SET + ControlAndStatus::TICKINT::SET + clock_source,
-            );
+        // We really just need to set the TICKINT bit here, but can't use modify() because
+        // readying the CSR register will throw away evidence of expiration if one
+        // occurred, so we re-write entire value instead.
+        SYSTICK_BASE
+            .syst_csr
+            .write(ControlAndStatus::TICKINT::SET + ControlAndStatus::ENABLE::SET + clock_source);
+    }
+
+    fn disarm(&self) {
+        let clock_source: FieldValue<u32, self::ControlAndStatus::Register> = if self.external_clock
+        {
+            // CLKSOURCE 0 --> external clock
+            ControlAndStatus::CLKSOURCE::CLEAR
         } else {
-            SYSTICK_BASE
-                .syst_csr
-                .write(ControlAndStatus::ENABLE::SET + clock_source);
-        }
+            // CLKSOURCE 1 --> internal clock
+            ControlAndStatus::CLKSOURCE::SET
+        };
+
+        // We really just need to set the TICKINT bit here, but can't use modify() because
+        // readying the CSR register will throw away evidence of expiration if one
+        // occurred, so we re-write entire value instead.
+        SYSTICK_BASE
+            .syst_csr
+            .write(ControlAndStatus::TICKINT::CLEAR + ControlAndStatus::ENABLE::SET + clock_source);
+    }
+
+    fn get_remaining_us(&self) -> u32 {
+        // use u64 in case of overflow when multiplying by 1,000,000
+        let tics = SYSTICK_BASE.syst_cvr.read(CurrentValue::CURRENT) as u64;
+        let hertz = self.hertz() as u64;
+        ((tics * 1_000_000) / hertz) as u32
     }
 }

arch/cortex-m3/src/lib.rs

@@ -40,12 +40,6 @@ pub unsafe extern "C" fn systick_handler() {
 pub unsafe extern "C" fn systick_handler() {
     llvm_asm!(
         "
-    /* Mark that the systick handler was called meaning that the process */
-    /* stopped executing because it has exceeded its timeslice. */
-    ldr r0, =SYSTICK_EXPIRED
-    mov r1, #1
-    str r1, [r0, #0]
-
     /* Set thread mode to privileged */
     mov r0, #0
     msr CONTROL, r0

arch/cortex-m4/src/lib.rs

@@ -36,21 +36,15 @@ pub unsafe extern "C" fn systick_handler() {
 }
 
 /// The `systick_handler` is called when the systick interrupt occurs, signaling
-/// that an application executed for longer than its timeslice. If this is
-/// called we want to return to the scheduler.
+/// that an application executed for longer than its timeslice. This interrupt
+/// handler is no longer responsible for signaling to the kernel thread that
+/// an interrupt has occurred, but is slightly more efficient than the generic_isr
+/// handler on account of not needing to mark the interrupt as pending.
 #[cfg(all(target_arch = "arm", target_os = "none"))]
 #[naked]
 pub unsafe extern "C" fn systick_handler() {
     llvm_asm!(
         "
-    // Mark that the systick handler was called meaning that the process stopped
-    // executing because it has exceeded its timeslice. This is a global
-    // variable that the `UserspaceKernelBoundary` code uses to determine why
-    // the application stopped executing.
-    ldr r0, =SYSTICK_EXPIRED
-    mov r1, #1
-    str r1, [r0, #0]
-
     // Set thread mode to privileged to switch back to kernel mode.
     mov r0, #0
     msr CONTROL, r0

arch/rv32i/src/machine_timer.rs

@@ -5,6 +5,7 @@ use kernel::common::cells::OptionalCell;
 use kernel::common::registers::{register_bitfields, ReadOnly, ReadWrite};
 use kernel::common::StaticRef;
 use kernel::hil;
+use kernel::hil::time::{Alarm, Frequency, Time};
 
 #[repr(C)]
 pub struct MachineTimerRegisters {
@@ -71,6 +72,13 @@ impl<'a> hil::time::Alarm<'a> for MachineTimer<'a> {
     }
 
     fn set_alarm(&self, tics: u32) {
+        // TODO: Need to avoid generating spurious interrupts from
+        // it taking 2 instructions to write 64 bit registers, see RISC-V Priveleged ISA
+        //
+        // Correct approach will require seperating MTIMECMP into separate registers. Then,
+        // first, set the upper register to the largest value possible without setting
+        // off the alarm; this way, we can set the lower register without setting
+        // off the alarm, then set the upper register to the correct value.
         self.registers
             .mtimecmp
             .write(MTimeCmp::MTIMECMP.val(tics as u64));
@@ -91,3 +99,45 @@ impl<'a> hil::time::Alarm<'a> for MachineTimer<'a> {
         self.registers.mtimecmp.get() != 0xFFFF_FFFF_FFFF_FFFF
     }
 }
+
+/// SchedulerTimer Implementation for RISC-V mtimer. Notably, this implementation should only be
+/// used by a chip if that chip has multiple hardware timer peripherals such that a different
+/// hardware timer can be used to provide alarms to capsules and userspace. This
+/// implementation will not work alongside other uses of the machine timer.
+impl kernel::SchedulerTimer for MachineTimer<'_> {
+    fn start(&self, us: u32) {
+        let tics = {
+            // We need to convert from microseconds to native tics, which could overflow in 32-bit
+            // arithmetic. So we convert to 64-bit. 64-bit division is an expensive subroutine, but
+            // if `us` is a power of 10 the compiler will simplify it with the 1_000_000 divisor
+            // instead.
+            let us = us as u64;
+            let hertz = <Self as Time>::Frequency::frequency() as u64;
+
+            hertz * us / 1_000_000
+        };
+        self.registers.mtimecmp.write(MTimeCmp::MTIMECMP.val(tics));
+    }
+
+    fn get_remaining_us(&self) -> u32 {
+        let tics = self.get_alarm().wrapping_sub(self.now()) as u64;
+        let hertz = <Self as Time>::Frequency::frequency() as u64;
+        ((tics * 1_000_000) / hertz) as u32
+    }
+
+    fn has_expired(&self) -> bool {
+        self.now() < self.get_alarm()
+    }
+
+    fn reset(&self) {
+        self.disable_machine_timer();
+    }
+
+    fn arm(&self) {
+        csr::CSR.mie.modify(csr::mie::mie::mtimer::SET);
+    }
+
+    fn disarm(&self) {
+        csr::CSR.mie.modify(csr::mie::mie::mtimer::CLEAR);
+    }
+}

boards/hifive1/src/main.rs

@@ -14,6 +14,8 @@ use kernel::capabilities;
 use kernel::common::dynamic_deferred_call::{DynamicDeferredCall, DynamicDeferredCallClientState};
 use kernel::component::Component;
 use kernel::hil;
+use kernel::hil::time::Alarm;
+use kernel::Chip;
 use kernel::Platform;
 use kernel::{create_capability, debug, static_init};
 use rv32i::csr;
@@ -26,15 +28,17 @@ static mut PROCESSES: [Option<&'static dyn kernel::procs::ProcessType>; 4] =
     [None, None, None, None];
 
 // Reference to the chip for panic dumps.
-static mut CHIP: Option<&'static e310x::chip::E310x> = None;
+static mut CHIP: Option<
+    &'static e310x::chip::E310x<VirtualMuxAlarm<'static, rv32i::machine_timer::MachineTimer>>,
+> = None;
 
 // How should the kernel respond when a process faults.
 const FAULT_RESPONSE: kernel::procs::FaultResponse = kernel::procs::FaultResponse::Panic;
 
 /// Dummy buffer that causes the linker to reserve enough space for the stack.
 #[no_mangle]
 #[link_section = ".stack_buffer"]
-pub static mut STACK_MEMORY: [u8; 0x800] = [0; 0x800];
+pub static mut STACK_MEMORY: [u8; 0x900] = [0; 0x900];
 
 /// A structure representing this platform that holds references to all
 /// capsules for this platform. We've included an alarm and console.
@@ -109,18 +113,6 @@ pub unsafe fn reset_handler() {
         None,
     );
 
-    let chip = static_init!(e310x::chip::E310x, e310x::chip::E310x::new());
-    CHIP = Some(chip);
-
-    // Need to enable all interrupts for Tock Kernel
-    chip.enable_plic_interrupts();
-
-    // enable interrupts globally
-    csr::CSR
-        .mie
-        .modify(csr::mie::mie::mext::SET + csr::mie::mie::msoft::SET + csr::mie::mie::mtimer::SET);
-    csr::CSR.mstatus.modify(csr::mstatus::mstatus::mie::SET);
-
     // Create a shared UART channel for the console and for kernel debug.
     let uart_mux = components::console::UartMuxComponent::new(
         &e310x::uart::UART0,
@@ -162,6 +154,10 @@ pub unsafe fn reset_handler() {
         VirtualMuxAlarm<'static, rv32i::machine_timer::MachineTimer>,
         VirtualMuxAlarm::new(mux_alarm)
     );
+    let systick_virtual_alarm = static_init!(
+        VirtualMuxAlarm<'static, rv32i::machine_timer::MachineTimer>,
+        VirtualMuxAlarm::new(mux_alarm)
+    );
     let alarm = static_init!(
         capsules::alarm::AlarmDriver<
             'static,
@@ -174,6 +170,22 @@ pub unsafe fn reset_handler() {
     );
     hil::time::Alarm::set_client(virtual_alarm_user, alarm);
 
+    let chip = static_init!(
+        e310x::chip::E310x<VirtualMuxAlarm<'static, rv32i::machine_timer::MachineTimer>>,
+        e310x::chip::E310x::new(systick_virtual_alarm)
+    );
+    systick_virtual_alarm.set_client(chip.scheduler_timer());
+    CHIP = Some(chip);
+
+    // Need to enable all interrupts for Tock Kernel
+    chip.enable_plic_interrupts();
+
+    // enable interrupts globally
+    csr::CSR
+        .mie
+        .modify(csr::mie::mie::mext::SET + csr::mie::mie::msoft::SET + csr::mie::mie::mtimer::SET);
+    csr::CSR.mstatus.modify(csr::mstatus::mstatus::mie::SET);
+
     // Setup the console.
     let console = components::console::ConsoleComponent::new(board_kernel, uart_mux).finalize(());
     // Create the debugger object that handles calls to `debug!()`.