implement OwnedPhysicalMemory which deallocates physical memory on drop (#164)

* add OwnedPhysicalMemory which deallocates phys mem on drop

The OwnedPhysicalMemory is the logical pendant to the OwnedSegment,
with the difference that we assume that there is only one global
physical memory, so we don't need to worry about lifetimes and
back references to the allocator.
Upon allocation, such an owned object is handed out, and the caller
must keep it, leak it or drop it. If the caller decides to leak it (because for
example a third party library needs to take ownership over the memory -
see HalImpl in this commit), he is responsible for deallocation.

This design decision may or may not be beneficial for future decisions,
in part because it may be too simple or even incorrect.

* fix unnecessary dereference

* fix some compiler warnings that show up in my IDE

* fix some clippy warnings in tests

* run clippy with `--tests` in CI

* remove clone impl from OwnedPhysicalMemory

Author: tpdenk

.github/workflows/build.yml

@@ -36,7 +36,7 @@ jobs:
           cargo fmt -- --check
       - name: Clippy
         run: |
-          cargo clippy -- -D clippy::all
+          cargo clippy --tests -- -D clippy::all
 
   test:
     name: "Test"

kernel/crates/kernel_vfs/src/path/absolute.rs

@@ -200,7 +200,7 @@ mod tests {
     #[test]
     fn test_deref_to_path() {
         let abs_path: &AbsolutePath = "/foo/bar".try_into().unwrap();
-        let path: &Path = &**abs_path;
+        let path: &Path = abs_path;
         assert_eq!(&**path, "/foo/bar");
     }
 

kernel/crates/kernel_vfs/src/path/absolute_owned.rs

@@ -164,7 +164,7 @@ mod tests {
     #[test]
     fn test_deref() {
         let abs_path = AbsoluteOwnedPath::new();
-        let owned: &OwnedPath = &*abs_path;
+        let owned: &OwnedPath = &abs_path;
         assert_eq!(owned.as_str(), "/");
     }
 

kernel/crates/kernel_vfs/src/path/filenames.rs

@@ -274,19 +274,22 @@ mod tests {
         let path = Path::new("/foo/bar/baz/qux");
         let mut iter = path.filenames();
 
-        assert_eq!(iter.nth(0), Some("foo"));
-        assert_eq!(iter.nth(0), Some("bar"));
-        assert_eq!(iter.nth(0), Some("baz"));
-        assert_eq!(iter.nth(0), Some("qux"));
-        assert_eq!(iter.nth(0), None);
+        assert_eq!(iter.next(), Some("foo"));
+        assert_eq!(iter.next(), Some("bar"));
+        assert_eq!(iter.next(), Some("baz"));
+        assert_eq!(iter.next(), Some("qux"));
+        assert_eq!(iter.next(), None);
     }
 
     #[test]
     fn test_filenames_last() {
-        assert_eq!(Path::new("/foo/bar/baz").filenames().last(), Some("baz"));
-        assert_eq!(Path::new("/foo").filenames().last(), Some("foo"));
-        assert_eq!(Path::new("").filenames().last(), None);
-        assert_eq!(Path::new("/").filenames().last(), None);
+        assert_eq!(
+            Path::new("/foo/bar/baz").filenames().next_back(),
+            Some("baz")
+        );
+        assert_eq!(Path::new("/foo").filenames().next_back(), Some("foo"));
+        assert_eq!(Path::new("").filenames().next_back(), None);
+        assert_eq!(Path::new("/").filenames().next_back(), None);
     }
 
     #[test]

kernel/crates/kernel_vfs/src/path/mod.rs

@@ -399,6 +399,7 @@ mod tests {
     }
 
     #[test]
+    #[allow(clippy::useless_asref)]
     fn test_path_as_ref() {
         let path = Path::new("/foo/bar");
         let path_ref: &Path = path.as_ref();

kernel/crates/kernel_vfs/src/path/owned.rs

@@ -242,7 +242,7 @@ mod tests {
     #[test]
     fn test_deref() {
         let owned = OwnedPath::new("/foo/bar");
-        let path: &Path = &*owned;
+        let path: &Path = &owned;
         assert_eq!(&**path, "/foo/bar");
 
         // Should have access to Path methods

kernel/src/driver/virtio/hal.rs

@@ -13,7 +13,7 @@ use x86_64::{PhysAddr, VirtAddr};
 
 use crate::driver::pci::VirtIoCam;
 use crate::mem::address_space::AddressSpace;
-use crate::mem::phys::PhysicalMemory;
+use crate::mem::phys::{OwnedPhysicalMemory, PhysicalMemory};
 use crate::mem::virt::{VirtualMemoryAllocator, VirtualMemoryHigherHalf};
 use crate::{U64Ext, UsizeExt};
 
@@ -34,37 +34,50 @@ pub struct HalImpl;
 
 unsafe impl Hal for HalImpl {
     fn dma_alloc(pages: usize, _: BufferDirection) -> (u64, NonNull<u8>) {
-        let frames = PhysicalMemory::allocate_frames(pages).unwrap();
+        let frames = PhysicalMemory::allocate_frames::<Size4KiB>(pages).unwrap();
+        let start_address = frames.start.start_address().as_u64();
         let segment = VirtualMemoryHigherHalf.reserve(pages).unwrap();
         AddressSpace::kernel()
             .map_range::<Size4KiB>(
                 &*segment,
-                frames,
-                PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
+                frames.leak(), // this has to be deallocated in dma_dealloc
+                PageTableFlags::PRESENT | PageTableFlags::WRITABLE | PageTableFlags::NO_EXECUTE,
             )
             .unwrap();
         let segment = segment.leak();
         let addr = NonNull::new(segment.start.as_mut_ptr::<u8>()).unwrap();
-        (frames.start.start_address().as_u64(), addr)
+        (start_address, addr)
     }
 
     unsafe fn dma_dealloc(paddr: u64, vaddr: NonNull<u8>, pages: usize) -> i32 {
-        let frames = PhysFrameRangeInclusive::<Size4KiB> {
-            start: PhysFrame::containing_address(PhysAddr::new(paddr)),
-            end: PhysFrame::containing_address(PhysAddr::new(
+        // strictly speaking this is not necessary, but since we allocate it through
+        // an [`OwnedPhysicalMemory`] object, we should also deallocate it. That way,
+        // for allocation and deallocation, everything goes through
+        // [`OwnedPhysicalMemory`] which maybe makes debugging easier.
+        //
+        // Alternatively, we could only create the [`PhysFrameRangeInclusive`] and
+        // then deallocate that directly.
+        let frames = {
+            let start = PhysFrame::containing_address(PhysAddr::new(paddr));
+            let end = PhysFrame::containing_address(PhysAddr::new(
                 paddr + (pages * Size4KiB::SIZE.into_usize()).into_u64() - 1,
-            )),
+            ));
+            let range = PhysFrameRangeInclusive::<Size4KiB> { start, end };
+            OwnedPhysicalMemory::from_physical_frame_range(range)
         };
+
         let segment = Segment::new(
             VirtAddr::from_ptr(vaddr.as_ptr()),
             pages.into_u64() * Size4KiB::SIZE,
         );
         unsafe {
             AddressSpace::kernel().unmap_range::<Size4KiB>(&segment, |_| {});
             assert!(VirtualMemoryHigherHalf.release(segment));
-            PhysicalMemory::deallocate_frames(frames);
         }
 
+        // we can not drop this any earlier, especially not until the respective virtual memory is unmapped
+        drop(frames);
+
         0
     }
 

kernel/src/main.rs

@@ -5,7 +5,6 @@ extern crate alloc;
 use alloc::boxed::Box;
 use alloc::sync::Arc;
 use core::error::Error;
-use core::panic::PanicInfo;
 
 use ext2::Ext2Fs;
 use kernel::driver::KernelDeviceId;
@@ -17,9 +16,8 @@ use kernel::mcore;
 use kernel::mcore::mtask::process::Process;
 use kernel_device::block::{BlockBuf, BlockDevice};
 use kernel_vfs::path::{AbsolutePath, ROOT};
-use log::{error, info};
+use log::info;
 use spin::RwLock;
-use x86_64::instructions::hlt;
 
 #[unsafe(export_name = "kernel_main")]
 unsafe extern "C" fn main() -> ! {
@@ -87,15 +85,17 @@ impl<const N: usize> filesystem::BlockDevice for ArcLockedBlockDevice<N> {
 
 #[panic_handler]
 #[cfg(not(test))]
-fn rust_panic(info: &PanicInfo) -> ! {
+fn rust_panic(info: &core::panic::PanicInfo) -> ! {
     handle_panic(info);
     loop {
-        hlt();
+        x86_64::instructions::hlt();
     }
 }
 
 #[cfg(not(test))]
-fn handle_panic(info: &PanicInfo) {
+fn handle_panic(info: &core::panic::PanicInfo) {
+    use log::error;
+
     let location = info.location().unwrap();
     error!(
         "kernel panicked at {}:{}:{}:",

kernel/src/mcore/mtask/process/mem.rs

@@ -4,10 +4,9 @@ use core::slice;
 use kernel_vfs::node::VfsNode;
 use spin::mutex::Mutex;
 use x86_64::VirtAddr;
-use x86_64::structures::paging::PhysFrame;
-use x86_64::structures::paging::frame::PhysFrameRangeInclusive;
 
 use crate::UsizeExt;
+use crate::mem::phys::OwnedPhysicalMemory;
 use crate::mem::virt::OwnedSegment;
 
 pub struct MemoryRegions {
@@ -113,50 +112,32 @@ pub struct LazyMemoryRegion {
     size: usize,
     /// The physical frames that were mapped for this lazy
     /// memory region.
-    _physical_frames: Mutex<Vec<PhysFrame>>,
-}
-
-impl Drop for LazyMemoryRegion {
-    fn drop(&mut self) {
-        todo!("deallocate physical memory")
-    }
+    _physical_frames: Mutex<Vec<OwnedPhysicalMemory>>,
 }
 
 #[derive(Debug)]
 pub struct MappedMemoryRegion {
     segment: OwnedSegment<'static>,
     size: usize,
-    _physical_frames: PhysFrameRangeInclusive,
+    _physical_memory: OwnedPhysicalMemory,
 }
 
 impl MappedMemoryRegion {
     pub fn new(
         segment: OwnedSegment<'static>,
         size: usize,
-        physical_frames: PhysFrameRangeInclusive,
+        physical_memory: OwnedPhysicalMemory,
     ) -> Self {
         Self {
             segment,
             size,
-            _physical_frames: physical_frames,
+            _physical_memory: physical_memory,
         }
     }
 }
 
-impl Drop for MappedMemoryRegion {
-    fn drop(&mut self) {
-        todo!("deallocate physical memory")
-    }
-}
-
 #[derive(Debug)]
 pub struct FileBackedMemoryRegion {
     region: LazyMemoryRegion,
     _node: VfsNode,
 }
-
-impl Drop for FileBackedMemoryRegion {
-    fn drop(&mut self) {
-        todo!("deallocate physical memory")
-    }
-}

kernel/src/mem/phys.rs

@@ -1,6 +1,7 @@
 use alloc::vec::Vec;
 use core::iter::from_fn;
-use core::mem::swap;
+use core::mem::{ManuallyDrop, swap};
+use core::ops::Deref;
 
 use conquer_once::spin::OnceCell;
 use kernel_physical_memory::{PhysicalFrameAllocator, PhysicalMemoryManager};
@@ -21,6 +22,59 @@ fn allocator() -> &'static Mutex<MultiStageAllocator> {
         .expect("physical allocator not initialized")
 }
 
+#[derive(Debug, Eq, PartialEq)]
+pub struct OwnedPhysicalMemory {
+    range: PhysFrameRangeInclusive<Size4KiB>,
+}
+
+impl !Clone for OwnedPhysicalMemory {}
+
+impl OwnedPhysicalMemory {
+    pub fn from_physical_frame<S: PageSize>(frame: PhysFrame<S>) -> Self {
+        let start_address = frame.start_address();
+        let start = unsafe {
+            // Safety: safe because the start address is guaranteed to be valid
+            PhysFrame::<Size4KiB>::from_start_address_unchecked(start_address)
+        };
+        let end = PhysFrame::<Size4KiB>::containing_address(start_address + S::SIZE - 1);
+        Self {
+            range: PhysFrameRangeInclusive { start, end },
+        }
+    }
+
+    pub fn from_physical_frame_range<S: PageSize>(range: PhysFrameRangeInclusive<S>) -> Self {
+        let start_address = range.start.start_address();
+        let start = PhysFrame::<Size4KiB>::containing_address(start_address);
+        let end_address = range.end.start_address() + range.end.size() - 1;
+        let end = PhysFrame::<Size4KiB>::containing_address(end_address);
+        Self {
+            range: PhysFrameRangeInclusive { start, end },
+        }
+    }
+
+    pub fn is_single_frame(&self) -> bool {
+        self.range.start == self.range.end
+    }
+
+    pub fn leak(self) -> PhysFrameRangeInclusive<Size4KiB> {
+        ManuallyDrop::new(self).range
+    }
+}
+
+impl Deref for OwnedPhysicalMemory {
+    type Target = PhysFrameRangeInclusive<Size4KiB>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.range
+    }
+}
+
+impl Drop for OwnedPhysicalMemory {
+    fn drop(&mut self) {
+        PhysicalMemory::deallocate_frames(self.range);
+    }
+}
+
 /// Zero-sized facade to the global physical memory allocator.
 ///
 /// This provides a stateless interface to a two-stage allocator:
@@ -87,11 +141,14 @@ impl PhysicalMemory {
     /// Panics if stage 1 allocator is still active (stage 1 doesn't support contiguous
     /// allocation). Stage 2 is initialized after the heap becomes available.
     #[must_use]
-    pub fn allocate_frames<S: PageSize>(n: usize) -> Option<PhysFrameRangeInclusive<S>>
+    pub fn allocate_frames<S: PageSize>(n: usize) -> Option<OwnedPhysicalMemory>
     where
         PhysicalMemoryManager: PhysicalFrameAllocator<S>,
     {
-        allocator().lock().allocate_frames(n)
+        allocator()
+            .lock()
+            .allocate_frames(n)
+            .map(OwnedPhysicalMemory::from_physical_frame_range)
     }
 
     /// Returns the frame to the free pool for future allocations.