diff --git a/kernel-rs/src/kalloc.rs b/kernel-rs/src/kalloc.rs index 6f6ac361c..9a34bbd76 100644 --- a/kernel-rs/src/kalloc.rs +++ b/kernel-rs/src/kalloc.rs @@ -1,10 +1,12 @@ //! Physical memory allocator, for user processes, //! kernel stacks, page-table pages, //! and pipe buffers. Allocates whole 4096-byte pages. -use core::mem; -use core::ptr; +use core::{mem, pin::Pin}; + +use pin_project::pin_project; use crate::{ + list::{List, ListEntry, ListNode}, lock::Spinlock, memlayout::PHYSTOP, page::Page, @@ -17,23 +19,61 @@ extern "C" { pub static mut end: [u8; 0]; } +#[repr(transparent)] +#[pin_project] struct Run { - next: *mut Run, + #[pin] + entry: ListEntry, +} + +impl Run { + /// # Safety + /// + /// It must be used only after initializing it with `Run::init`. + unsafe fn new() -> Self { + Self { + entry: unsafe { ListEntry::new() }, + } + } + + fn init(self: Pin<&mut Self>) { + self.project().entry.init(); + } +} + +// SAFETY: `Run` owns a `ListEntry`. +unsafe impl ListNode for Run { + fn get_list_entry(&self) -> &ListEntry { + &self.entry + } + + fn from_list_entry(list_entry: *const ListEntry) -> *const Self { + list_entry as _ + } } /// # Safety /// -/// - This singly linked list does not have a cycle. -/// - If head is null, then it is an empty list. Ohterwise, it is nonempty, and -/// head is its first element, which is a valid page. +/// The address of each `Run` in `runs` can become a `Page` by `Page::from_usize`. +// This implementation defers from xv6. Kmem of xv6 uses intrusive singly linked list, while this +// Kmem uses List, which is a intrusive doubly linked list type of rv6. In a intrusive singly +// linked list, it is impossible to automatically remove an entry from a list when it is dropped. +// Therefore, it is nontrivial to make a general intrusive singly linked list type in a safe way. +// For this reason, we use a doubly linked list instead. It adds runtime overhead, but the overhead +// seems negligible. +#[pin_project] pub struct Kmem { - head: *mut Run, + #[pin] + runs: List, } impl Kmem { - pub const fn new() -> Self { + /// # Safety + /// + /// It must be used only after initializing it with `Kmem::init`. + pub const unsafe fn new() -> Self { Self { - head: ptr::null_mut(), + runs: unsafe { List::new() }, } } @@ -43,7 +83,9 @@ impl Kmem { /// /// There must be no existing pages. It implies that this method should be /// called only once. - pub unsafe fn init(&mut self) { + pub unsafe fn init(mut self: Pin<&mut Self>) { + self.as_mut().project().runs.init(); + // SAFETY: safe to acquire only the address of a static variable. let pa_start = pgroundup(unsafe { end.as_ptr() as usize }); let pa_end = pgrounddown(PHYSTOP); @@ -53,35 +95,31 @@ impl Kmem { // * end <= pa < PHYSTOP // * the safety condition of this method guarantees that the // created page does not overlap with existing pages - self.free(unsafe { Page::from_usize(pa) }); + self.as_ref() + .get_ref() + .free(unsafe { Page::from_usize(pa) }); } } - pub fn free(&mut self, mut page: Page) { + pub fn free(&self, mut page: Page) { // Fill with junk to catch dangling refs. page.write_bytes(1); - let pa = page.into_usize(); - debug_assert!( - // SAFETY: safe to acquire only the address of a static variable. - pa % PGSIZE == 0 && (unsafe { end.as_ptr() as usize }..PHYSTOP).contains(&pa), - "Kmem::free" - ); - let mut r = pa as *mut Run; - // SAFETY: By the invariant of Page, it does not create a cycle in this list and - // thus is safe. - unsafe { (*r).next = self.head }; - self.head = r; + + let run = page.as_uninit_mut(); + // SAFETY: `run` will be initialized by the following `init`. + let run = run.write(unsafe { Run::new() }); + let mut run = unsafe { Pin::new_unchecked(run) }; + run.as_mut().init(); + self.runs.push_front(run.as_ref().get_ref()); + + // Since the page has returned to the list, forget the page. + mem::forget(page); } - pub fn alloc(&mut self) -> Option { - if self.head.is_null() { - return None; - } - // SAFETY: head is not null and the structure of this list - // is maintained by the invariant. - let next = unsafe { (*self.head).next }; - // SAFETY: the first element is a valid page by the invariant. - let mut page = unsafe { Page::from_usize(mem::replace(&mut self.head, next) as _) }; + pub fn alloc(&self) -> Option { + let run = self.runs.pop_front()?; + // SAFETY: the invariant of `Kmem`. + let mut page = unsafe { Page::from_usize(run as _) }; // fill with junk page.write_bytes(5); Some(page) diff --git a/kernel-rs/src/kernel.rs b/kernel-rs/src/kernel.rs index 246adb010..c514ac8b5 100644 --- a/kernel-rs/src/kernel.rs +++ b/kernel-rs/src/kernel.rs @@ -74,6 +74,7 @@ pub struct KernelBuilder { pub printer: Spinlock, + #[pin] pub kmem: Spinlock, /// The kernel's memory manager. @@ -132,7 +133,7 @@ impl KernelBuilder { console: Sleepablelock::new("CONS", Console::new()), uart: Uart::new(), printer: Spinlock::new("PRINTLN", Printer::new()), - kmem: Spinlock::new("KMEM", Kmem::new()), + kmem: Spinlock::new("KMEM", unsafe { Kmem::new() }), memory: MaybeUninit::uninit(), ticks: Sleepablelock::new("time", 0), procs: ProcsBuilder::zero(), @@ -217,7 +218,7 @@ pub unsafe fn kernel_main() -> ! { static STARTED: AtomicBool = AtomicBool::new(false); if cpuid() == 0 { - let kernel = unsafe { kernel_builder_unchecked_pin().project() }; + let mut kernel = unsafe { kernel_builder_unchecked_pin().project() }; // Initialize the kernel. @@ -230,10 +231,11 @@ pub unsafe fn kernel_main() -> ! { println!(); // Physical page allocator. - unsafe { kernel.kmem.get_mut().init() }; + unsafe { kernel.kmem.as_mut().get_pin_mut().init() }; // Create kernel memory manager. - let memory = KernelMemory::new(kernel.kmem).expect("PageTable::new failed"); + let memory = + KernelMemory::new(kernel.kmem.as_ref().get_ref()).expect("PageTable::new failed"); // Turn on paging. unsafe { kernel.memory.write(memory).init_hart() }; @@ -260,7 +262,7 @@ pub unsafe fn kernel_main() -> ! { kernel.file_system.log.disk.get_mut().init(); // First user process. - procs.user_proc_init(kernel.kmem); + procs.user_proc_init(kernel.kmem.as_ref().get_ref()); STARTED.store(true, Ordering::Release); } else { diff --git a/kernel-rs/src/page.rs b/kernel-rs/src/page.rs index 54ba9820e..df9747498 100644 --- a/kernel-rs/src/page.rs +++ b/kernel-rs/src/page.rs @@ -1,11 +1,12 @@ use core::{ mem, + mem::MaybeUninit, ops::{Deref, DerefMut}, ptr, ptr::NonNull, }; -use crate::{println, riscv::PGSIZE, vm::PAddr}; +use crate::{riscv::PGSIZE, vm::PAddr}; /// Page type. #[repr(align(4096))] @@ -71,6 +72,23 @@ impl Page { inner: unsafe { NonNull::new_unchecked(addr as *mut _) }, } } + + pub fn as_uninit_mut(&mut self) -> &mut MaybeUninit { + // TODO(https://github.com/kaist-cp/rv6/issues/471): + // Use const_assert! (or equivalent) instead. Currently, use of T inside const_assert! + // incurs a compile error: "can't use generic parameters from outer function". + // Also, there's a workaround using feature(const_generics) and + // feature(const_evaluatable_checked). However, using them makes the compiler panic. + // When the compiler becomes updated, we will fix the following lines to use static checks. + assert!(mem::size_of::() <= PGSIZE); + assert_eq!(PGSIZE % mem::align_of::(), 0); + + // SAFETY: self.inner is an array of length PGSIZE aligned with PGSIZE bytes. + // The above assertions show that it can contain a value of T. As it contains arbitrary + // data, we cannot treat it as &mut T. Instead, we use &mut MaybeUninit. It's ok because + // T and MaybeUninit have the same size, alignment, and ABI. + unsafe { &mut *(self.inner.as_ptr() as *mut MaybeUninit) } + } } impl Deref for Page { @@ -91,7 +109,6 @@ impl Drop for Page { fn drop(&mut self) { // HACK(@efenniht): we really need linear type here: // https://github.com/rust-lang/rfcs/issues/814 - println!("page must never drop."); panic!("Page must never drop."); } } diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index 0594e2b31..267a3cf05 100644 --- a/kernel-rs/src/pipe.rs +++ b/kernel-rs/src/pipe.rs @@ -1,14 +1,11 @@ use core::{mem, ops::Deref, ptr::NonNull}; -use static_assertions::const_assert; - use crate::{ file::{FileType, RcFile}, kernel::Kernel, lock::Spinlock, page::Page, proc::{CurrentProc, WaitChannel}, - riscv::PGSIZE, vm::UVAddr, }; @@ -135,16 +132,12 @@ impl Deref for AllocatedPipe { impl Kernel { pub fn allocate_pipe(&self) -> Result<(RcFile, RcFile), ()> { let page = self.kmem.alloc().ok_or(())?; - // SAFETY: by the invariant of `Page`, `page` is always non-null. - let mut ptr = unsafe { NonNull::new_unchecked(page.into_usize() as *mut Pipe) }; - - // `Pipe` must be aligned with `Page`. - const_assert!(mem::size_of::() <= PGSIZE); + let mut page = scopeguard::guard(page, |page| self.kmem.free(page)); + let ptr = page.as_uninit_mut(); - //TODO(https://github.com/kaist-cp/rv6/issues/367): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`. - // SAFETY: `ptr` holds a valid, unique page allocated from `self.alloc()`, - // and the pipe size and alignment are compatible with the page. - let _ = unsafe { ptr.as_uninit_mut() }.write(Pipe { + // TODO(https://github.com/kaist-cp/rv6/issues/367): + // Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`. + let ptr = NonNull::from(ptr.write(Pipe { inner: Spinlock::new( "pipe", PipeInner { @@ -157,36 +150,24 @@ impl Kernel { ), read_waitchannel: WaitChannel::new(), write_waitchannel: WaitChannel::new(), - }); - let f0 = self - .ftable - .alloc_file( - FileType::Pipe { - pipe: AllocatedPipe { ptr }, - }, - true, - false, - ) - // SAFETY: ptr is an address of a page obtained by alloc(). - .map_err(|_| { - self.kmem - .free(unsafe { Page::from_usize(ptr.as_ptr() as _) }) - })?; - let f1 = self - .ftable - .alloc_file( - FileType::Pipe { - pipe: AllocatedPipe { ptr }, - }, - false, - true, - ) - // SAFETY: ptr is an address of a page obtained by alloc(). - .map_err(|_| { - self.kmem - .free(unsafe { Page::from_usize(ptr.as_ptr() as _) }) - })?; - + })); + let f0 = self.ftable.alloc_file( + FileType::Pipe { + pipe: AllocatedPipe { ptr }, + }, + true, + false, + )?; + let f1 = self.ftable.alloc_file( + FileType::Pipe { + pipe: AllocatedPipe { ptr }, + }, + false, + true, + )?; + + // Since files have been created successfully, prevent the page from being deallocated. + mem::forget(scopeguard::ScopeGuard::into_inner(page)); Ok((f0, f1)) } } @@ -197,7 +178,7 @@ impl AllocatedPipe { // SAFETY: // If `Pipe::close()` returned true, this means all `AllocatedPipe`s were closed. // Hence, we can free the `Pipe`. - // Also, the following is safe since `ptr` holds a `Pipe` stored in a valid page allocated from `kernel().alloc()`. + // Also, the following is safe since `ptr` holds a `Pipe` stored in a valid page allocated from `Kmem::alloc`. Some(unsafe { Page::from_usize(self.ptr.as_ptr() as _) }) } else { None