page_tables: Cleanup

Remove central CURRENT_PAGE_TABLE
Don't drop PageTables while their active

Author: sysheap

src/kernel/src/cpu.rs

@@ -21,3 +21,22 @@ pub fn read_sepc() -> usize {
     }
     sepc
 }
+
+pub unsafe fn write_satp_and_fence(satp_val: usize) {
+    unsafe {
+        asm!("csrw satp, {satp_val}", satp_val = in(reg) satp_val);
+        asm!("sfence.vma");
+    }
+}
+
+pub fn read_satp() -> usize {
+    if cfg!(miri) {
+        return 0;
+    }
+
+    let satp: usize;
+    unsafe {
+        asm!("csrr {}, satp", out(reg) satp);
+    }
+    satp
+}

src/kernel/src/interrupts/trap.rs

@@ -3,11 +3,15 @@ use core::panic;
 use common::syscalls::trap_frame::{Register, TrapFrame};
 
 use crate::{
-    cpu, debug,
+    cpu::{self, read_satp, write_satp_and_fence},
+    debug,
     interrupts::plic::{self, InterruptSource},
     io::{stdin_buf::STDIN_BUFFER, uart},
-    memory::page_tables,
-    processes::{scheduler, timer},
+    memory::page_tables::{activate_page_table, KERNEL_PAGE_TABLES},
+    processes::{
+        scheduler::{self, get_current_process_expect},
+        timer,
+    },
     syscalls::handle_syscall,
 };
 
@@ -21,6 +25,9 @@ extern "C" fn supervisor_mode_trap(
     sepc: usize,
     trap_frame: &mut TrapFrame,
 ) {
+    let old_tables = read_satp();
+    debug!("Activate KERNEL_PAGE_TABLES");
+    activate_page_table(&KERNEL_PAGE_TABLES.lock());
     debug!(
         "Supervisor mode trap occurred! (sepc: {:x?}) (cause: {:?})\nTrap Frame: {:?}",
         sepc,
@@ -32,6 +39,15 @@ extern "C" fn supervisor_mode_trap(
     } else {
         handle_exception(cause, stval, sepc, trap_frame);
     }
+
+    // Restore old page tables
+    // SAFTEY: They should be valid. If a process dies we don't come here
+    // because the scheduler returns with restore_user_context
+    // Hoewever: This is very ugly and prone to error.
+    // TODO: Find a better way to do this
+    unsafe {
+        write_satp_and_fence(old_tables);
+    }
 }
 
 fn handle_exception(cause: InterruptCause, stval: usize, sepc: usize, trap_frame: &mut TrapFrame) {
@@ -47,7 +63,7 @@ fn handle_exception(cause: InterruptCause, stval: usize, sepc: usize, trap_frame
                 cause.get_exception_code(),
                 stval,
                 sepc,
-                page_tables::is_userspace_address(sepc),
+                get_current_process_expect().borrow().get_page_table().is_userspace_address(sepc),
                 trap_frame
             );
         }
@@ -65,8 +81,7 @@ fn handle_interrupt(cause: InterruptCause, _stval: usize, _sepc: usize, _trap_fr
 }
 
 fn handle_supervisor_timer_interrupt() {
-    debug!("Supervisor timer interrupt occurred!");
-    timer::set_timer(10);
+    timer::set_timer(10000);
     scheduler::schedule();
 }
 

src/kernel/src/main.rs

@@ -11,17 +11,14 @@
 #![feature(non_null_convenience)]
 #![feature(pointer_is_aligned)]
 #![feature(exposed_provenance)]
+#![feature(lazy_cell)]
 #![test_runner(test::test_runner)]
 #![reexport_test_harness_main = "test_main"]
 
-use core::cell::RefCell;
-
-use alloc::rc::Rc;
-
 use crate::{
     interrupts::plic,
     io::uart::QEMU_UART,
-    memory::page_tables::{self, RootPageTableHolder},
+    memory::page_tables::{self},
     processes::{scheduler, timer},
 };
 
@@ -70,9 +67,8 @@ extern "C" fn kernel_init() {
     #[cfg(test)]
     test_main();
 
-    page_tables::activate_page_table(Rc::new(RefCell::new(
-        RootPageTableHolder::new_with_kernel_mapping(),
-    )));
+    page_tables::activate_page_table(&page_tables::KERNEL_PAGE_TABLES.lock());
+
     interrupts::set_sscratch_to_kernel_trap_frame();
 
     plic::init_uart_interrupt();

src/kernel/src/memory/page.rs

@@ -9,7 +9,7 @@ use crate::klibc::util::copy_slice;
 
 pub const PAGE_SIZE: usize = 4096;
 
-#[derive(Debug, PartialEq, Eq, Clone)]
+#[derive(PartialEq, Eq, Clone)]
 #[repr(C, align(4096))]
 pub struct Page([u8; PAGE_SIZE]);
 
@@ -27,6 +27,12 @@ impl DerefMut for Page {
     }
 }
 
+impl core::fmt::Debug for Page {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "Page({:p})", self.0.as_ptr())
+    }
+}
+
 impl Page {
     fn zero() -> Self {
         Self([0; PAGE_SIZE])

src/kernel/src/memory/page_tables.rs

@@ -1,10 +1,11 @@
-use core::{arch::asm, cell::RefCell, fmt::Debug, ptr::null_mut, u8};
+use core::{cell::LazyCell, fmt::Debug, ptr::null_mut, u8};
 
-use alloc::{boxed::Box, rc::Rc};
+use alloc::boxed::Box;
 use common::mutex::Mutex;
 
 use crate::{
     assert::static_assert_size,
+    cpu::{read_satp, write_satp_and_fence},
     debug,
     interrupts::plic,
     io::TEST_DEVICE_ADDRESSS,
@@ -18,7 +19,11 @@ use crate::{
 
 use super::page::Page;
 
-static CURRENT_PAGE_TABLE: Mutex<Option<Rc<RefCell<RootPageTableHolder>>>> = Mutex::new(None);
+pub static KERNEL_PAGE_TABLES: Mutex<LazyCell<&'static RootPageTableHolder>> =
+    Mutex::new(LazyCell::new(|| {
+        let page_tables = Box::new(RootPageTableHolder::new_with_kernel_mapping());
+        Box::leak(page_tables)
+    }));
 
 pub struct RootPageTableHolder {
     root_table: *mut PageTable,
@@ -33,6 +38,7 @@ impl Debug for RootPageTableHolder {
 
 impl Drop for RootPageTableHolder {
     fn drop(&mut self) {
+        assert!(!self.is_active(), "Page table is dropped while active");
         let table = self.table();
         for first_level_entry in table.0.iter() {
             if !first_level_entry.get_validity() {
@@ -127,6 +133,20 @@ impl RootPageTableHolder {
         unsafe { &mut *self.root_table }
     }
 
+    fn is_active(&self) -> bool {
+        let satp = read_satp();
+        let ppn = satp & 0xfffffffffff;
+        let page_table_address = ppn << 12;
+
+        let current_physical_address = self.table().get_physical_address();
+
+        debug!(
+            "is_active: satp: {:x}; page_table_address: {:x}",
+            satp, current_physical_address
+        );
+        page_table_address == current_physical_address
+    }
+
     pub fn new_with_kernel_mapping() -> Self {
         let mut root_page_table_holder = RootPageTableHolder::empty();
 
@@ -314,6 +334,26 @@ impl RootPageTableHolder {
             name,
         );
     }
+
+    pub fn is_userspace_address(&self, address: usize) -> bool {
+        self.get_page_table_entry_for_address(address)
+            .map_or(false, |entry| entry.get_user_mode_accessible())
+    }
+
+    pub fn translate_userspace_address_to_physical_address<T>(
+        &self,
+        address: *const T,
+    ) -> Option<*const T> {
+        let address = address as usize;
+        if !self.is_userspace_address(address) {
+            return None;
+        }
+
+        let offset_from_page_start = address % PAGE_SIZE;
+        self.get_page_table_entry_for_address(address).map(|entry| {
+            (entry.get_physical_address() as usize + offset_from_page_start) as *const T
+        })
+    }
 }
 
 #[repr(C, align(4096))]
@@ -471,8 +511,8 @@ impl PageTableEntry {
     }
 }
 
-pub fn activate_page_table(page_table_holder: Rc<RefCell<RootPageTableHolder>>) {
-    let page_table_address = page_table_holder.borrow().table().get_physical_address();
+pub fn activate_page_table(page_table_holder: &RootPageTableHolder) {
+    let page_table_address = page_table_holder.table().get_physical_address();
 
     debug!(
         "Activate new page mapping (Addr of page tables 0x{:x})",
@@ -483,42 +523,8 @@ pub fn activate_page_table(page_table_holder: Rc<RefCell<RootPageTableHolder>>)
     let satp_val = 8 << 60 | (page_table_address_shifted & 0xfffffffffff);
 
     unsafe {
-        asm!("csrw satp, {satp_val}", satp_val = in(reg) satp_val);
-        asm!("sfence.vma");
-    }
-
-    CURRENT_PAGE_TABLE.lock().replace(page_table_holder);
-}
-
-pub fn is_userspace_address(address: usize) -> bool {
-    let current_page_table = CURRENT_PAGE_TABLE.lock();
-    if let Some(ref current_page_table) = *current_page_table {
-        current_page_table
-            .borrow()
-            .get_page_table_entry_for_address(address)
-            .map_or(false, |entry| entry.get_user_mode_accessible())
-    } else {
-        false
-    }
-}
-
-pub fn translate_userspace_address_to_physical_address<T>(address: *const T) -> Option<*const T> {
-    let address = address as usize;
-    if !is_userspace_address(address) {
-        return None;
-    }
-    let current_page_table = CURRENT_PAGE_TABLE.lock();
-    if let Some(ref current_page_table) = *current_page_table {
-        let offset_from_page_start = address % PAGE_SIZE;
-        current_page_table
-            .borrow()
-            .get_page_table_entry_for_address(address)
-            .map(|entry| {
-                (entry.get_physical_address() as usize + offset_from_page_start) as *const T
-            })
-    } else {
-        None
-    }
+        write_satp_and_fence(satp_val);
+    };
 }
 
 #[cfg(test)]

src/kernel/src/processes/process.rs

@@ -1,6 +1,6 @@
-use core::{cell::RefCell, fmt::Debug};
+use core::fmt::Debug;
 
-use alloc::{boxed::Box, rc::Rc, vec::Vec};
+use alloc::{boxed::Box, vec::Vec};
 use common::{
     mutex::Mutex,
     syscalls::trap_frame::{Register, TrapFrame},
@@ -34,7 +34,7 @@ fn get_next_pid() -> Pid {
 pub struct Process {
     pid: Pid,
     register_state: Box<TrapFrame>,
-    page_table: Rc<RefCell<RootPageTableHolder>>,
+    page_table: RootPageTableHolder,
     program_counter: usize,
     allocated_pages: Vec<PinnedHeapPages>,
     state: ProcessState,
@@ -66,7 +66,7 @@ impl Debug for Process {
 impl Process {
     pub fn mmap_pages(&mut self, number_of_pages: usize) -> *mut u8 {
         let pages = PinnedHeapPages::new(number_of_pages);
-        self.page_table.borrow_mut().map_userspace(
+        self.page_table.map_userspace(
             self.free_mmap_address,
             pages.as_ptr() as usize,
             PAGE_SIZE * number_of_pages,
@@ -99,8 +99,8 @@ impl Process {
         self.state = state;
     }
 
-    pub fn get_page_table(&self) -> Rc<RefCell<RootPageTableHolder>> {
-        self.page_table.clone()
+    pub fn get_page_table(&self) -> &RootPageTableHolder {
+        &self.page_table
     }
 
     pub fn get_pid(&self) -> Pid {
@@ -116,7 +116,7 @@ impl Process {
 
         let LoadedElf {
             entry_address,
-            page_tables,
+            page_tables: page_table,
             allocated_pages,
         } = loader::load_elf(elf_file);
 
@@ -126,7 +126,7 @@ impl Process {
         Self {
             pid: get_next_pid(),
             register_state: Box::new(register_state),
-            page_table: Rc::new(RefCell::new(page_tables)),
+            page_table,
             program_counter: entry_address,
             allocated_pages,
             state: ProcessState::Runnable,

src/kernel/src/processes/scheduler.rs

@@ -25,7 +25,7 @@ extern "C" {
     fn restore_user_context() -> !;
 }
 
-pub fn get_current_process() -> Rc<RefCell<Process>> {
+pub fn get_current_process_expect() -> Rc<RefCell<Process>> {
     CURRENT_PROCESS
         .lock()
         .as_ref()

src/kernel/src/syscalls/mod.rs

@@ -9,9 +9,8 @@ use common::syscalls::{
 use crate::{
     debug,
     io::stdin_buf::STDIN_BUFFER,
-    memory::page_tables::translate_userspace_address_to_physical_address,
     print,
-    processes::scheduler::{self, get_current_process, let_current_process_wait_for},
+    processes::scheduler::{self, get_current_process_expect, let_current_process_wait_for},
 };
 
 struct SyscallHandler;
@@ -43,7 +42,11 @@ impl common::syscalls::kernel::Syscalls for SyscallHandler {
     #[allow(non_snake_case)]
     fn EXECUTE(&self, name: Userpointer<u8>, length: usize) -> isize {
         // Check validity of userspointer before using it
-        let physical_address = translate_userspace_address_to_physical_address(name.get());
+        let current_process = get_current_process_expect();
+        let current_process = current_process.borrow();
+        let physical_address = current_process
+            .get_page_table()
+            .translate_userspace_address_to_physical_address(name.get());
 
         if let Some(physical_address) = physical_address {
             let slice = unsafe { &*slice_from_raw_parts(physical_address, length) };
@@ -73,7 +76,7 @@ impl common::syscalls::kernel::Syscalls for SyscallHandler {
 
     #[allow(non_snake_case)]
     fn MMAP_PAGES(&self, number_of_pages: usize) -> isize {
-        let current_process = get_current_process();
+        let current_process = get_current_process_expect();
         let mut current_process = current_process.borrow_mut();
         current_process.mmap_pages(number_of_pages) as isize
     }