Remove impl Allocator for memory mapper; not confident is it correct

Author: jdreaver

README.md

@@ -94,22 +94,13 @@ make test
 - VirtIO improvements:
   - Abstract and improve request/response queue from rng
   - Ensure we zero out descriptors after using them so we don't accidentally reuse the buffer
-  - Allocate buffers on the fly instead of using static buffers, or have client provide buffer
-    - Reuse them if we are reusing a descriptor, or free them.
-    - If the VirtIO device always owns the memory it uses, it is easier to reason about alloc/free, ensure buffers are the right size, and ensure provided buffers are physically contiguous
-  - Allocation and pointers: avoid manually calling `memory` alloc functions and passing around pointers
-    - Have virtqueues "own" their buffers and handle alloc/dealloc. Devices that need to alloc for descriptors, like virtio-blk, can do the same.
-    - We can use `Box<T, KERNEL_PHYSICAL_ALLOC>` to ensure we are using physical memory.
+  - Improve memory "ownership" ergonomics, ensuring buffers are dropped after use. Maybe abstract into a common layer somehow?
+  - Create a physically contiguous heap, or slab allocator, or something for virtio buffer requests so we don't waste an entire page per tiny allocation.
   - Locking: we need to lock writes (I think?), but we should be able to read from the queue without locking. This should be ergonomic. I don't necessarily want to bury a mutex deep in the code.
     - Investigate how Linux or other OS virtio drivers do locking
   - Ensure we don't accidentally reuse descriptors while we are waiting for a response from the device. Don't automatically just wrap around! This is what might require a mutex rather than just atomic integers?
   - I think there is a race condition with the interrupts with the current non-locking mechanism. Ensure that if there are concurrent writes while an interrupt, then an interrupt won't miss a read (e.g. there will at least be a followup interrupt)
   - Remember features we negotiate, and ensure we are accounting for the different features in the logic (especially around notifications)
-- Physical memory allocation
-  - Ensure `PhysicalBuffer` allocation and deallocation is safe, and that page and size math is correct (particularly in `drop()`). Make it foolproof.
-    - Be explicit about it allocating pages. Don't pretend it is just addresses. Perhaps put page <-> address translation layer in a single spot.
-    - In fact, maybe `LockedPhysicalMemoryAllocator` should _not_ implement `Allocator`. If we want a malloc-like behavior on top of contiguous physical memory, we should explicitly use e.g. `LockedHeap` on top of some allocated physical pages (basically an arena).
-- Does `PhysicalBuffer::allocate` need an alignment argument? Does anything ever need more than page alignment?
 - Create `sync` module that has synchronization primitives
   - Wrapper around `spin::Mutex` that also disables interrupts, similar to Linux's `spin_lock_irqsave` (`x86_64::interrupts::without_interrupts` is handy here). Might need our own custom `MutexGuard` wrapper that handles re-enabling interrupts on `drop()`
   - In the future we should disable preemption when spin locks are taken

kernel/src/memory.rs

@@ -1,6 +1,5 @@
-use core::alloc::{AllocError, Allocator, Layout};
+use core::alloc::AllocError;
 use core::ptr;
-use core::ptr::NonNull;
 
 use spin::Mutex;
 use x86_64::structures::paging::mapper::{MapToError, Translate};
@@ -16,7 +15,7 @@ use bitmap_alloc::{bootstrap_allocator, BitmapAllocator, MemoryRegion};
 static KERNEL_MAPPER: KernelMapper = KernelMapper::new();
 
 /// Physical memory frame allocator used by all kernel contexts.
-pub(crate) static KERNEL_PHYSICAL_ALLOCATOR: LockedPhysicalMemoryAllocator =
+static KERNEL_PHYSICAL_ALLOCATOR: LockedPhysicalMemoryAllocator =
     LockedPhysicalMemoryAllocator::new();
 
 /// Initialize the `KERNEL_MAPPER` with the passed `physical_memory_offset`.
@@ -195,52 +194,61 @@ impl LockedPhysicalMemoryAllocator<'_> {
     }
 }
 
-/// We implement the `Allocator` trait for `PhysicalMemoryAllocator`
-/// so that we can use it for custom allocations for physically contiguous
-/// memory.
-unsafe impl Allocator for LockedPhysicalMemoryAllocator<'_> {
-    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
-        let size = layout.size();
-        let num_pages = size.div_ceil(PhysicalMemoryAllocator::PAGE_SIZE);
-
-        let alignment = layout.align() as u64;
-        assert!(
-            alignment <= PhysicalMemoryAllocator::PAGE_SIZE as u64,
-            "alignment {alignment} must be <= page size {}. What the hell are we aligning???",
-            PhysicalMemoryAllocator::PAGE_SIZE,
-        );
-        let start_page = self.with_lock(|allocator| {
-            allocator
-                .allocator
-                .allocate_contiguous(num_pages)
-                .ok_or(AllocError)
-        })?;
-        let start_address = start_page * PhysicalMemoryAllocator::PAGE_SIZE;
-        let actual_size = num_pages * PhysicalMemoryAllocator::PAGE_SIZE;
-        let ptr = unsafe { nonnull_ptr_slice_from_addr_len(start_address, actual_size) };
-
-        Ok(ptr)
-    }
-
-    unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
-        let size = layout.size();
-        let num_pages = size.div_ceil(PhysicalMemoryAllocator::PAGE_SIZE);
-        let start_addr = ptr.as_ptr() as usize;
-        assert!(
-            start_addr % PhysicalMemoryAllocator::PAGE_SIZE == 0,
-            "somehow start address of {start_addr} is not page aligned"
-        );
-        let start_page = start_addr / PhysicalMemoryAllocator::PAGE_SIZE;
-        self.with_lock(|allocator| {
-            allocator.allocator.free_contiguous(start_page, num_pages);
-        });
-    }
-}
-
-unsafe fn nonnull_ptr_slice_from_addr_len(addr: usize, len_bytes: usize) -> NonNull<[u8]> {
-    let ptr = addr as *mut u8;
-    NonNull::new_unchecked(core::slice::from_raw_parts_mut(ptr, len_bytes))
-}
+// I'm not sure this is 100% correct, so I'm not doing it. In particular, I
+// worry that deallocate is incorrect because I'm not sure what the
+// characteristics of Layout are. It is better to be explicit that the physical
+// memory allocator deals with pages. If we want contiguous heap-like
+// allocation, we should implement a heap on top of physically contiguous
+// memory, or do something like slab allocation on top of physically contiguous
+// memory.
+//
+//
+// /// We implement the `Allocator` trait for `PhysicalMemoryAllocator`
+// /// so that we can use it for custom allocations for physically contiguous
+// /// memory.
+// unsafe impl Allocator for LockedPhysicalMemoryAllocator<'_> {
+//     fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+//         let size = layout.size();
+//         let num_pages = size.div_ceil(PhysicalMemoryAllocator::PAGE_SIZE);
+
+//         let alignment = layout.align() as u64;
+//         assert!(
+//             alignment <= PhysicalMemoryAllocator::PAGE_SIZE as u64,
+//             "alignment {alignment} must be <= page size {}. What the hell are we aligning???",
+//             PhysicalMemoryAllocator::PAGE_SIZE,
+//         );
+//         let start_page = self.with_lock(|allocator| {
+//             allocator
+//                 .allocator
+//                 .allocate_contiguous(num_pages)
+//                 .ok_or(AllocError)
+//         })?;
+//         let start_address = start_page * PhysicalMemoryAllocator::PAGE_SIZE;
+//         let actual_size = num_pages * PhysicalMemoryAllocator::PAGE_SIZE;
+//         let ptr = unsafe { nonnull_ptr_slice_from_addr_len(start_address, actual_size) };
+
+//         Ok(ptr)
+//     }
+
+//     unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
+//         let size = layout.size();
+//         let num_pages = size.div_ceil(PhysicalMemoryAllocator::PAGE_SIZE);
+//         let start_addr = ptr.as_ptr() as usize;
+//         assert!(
+//             start_addr % PhysicalMemoryAllocator::PAGE_SIZE == 0,
+//             "somehow start address of {start_addr} is not page aligned"
+//         );
+//         let start_page = start_addr / PhysicalMemoryAllocator::PAGE_SIZE;
+//         self.with_lock(|allocator| {
+//             allocator.allocator.free_contiguous(start_page, num_pages);
+//         });
+//     }
+// }
+
+// unsafe fn nonnull_ptr_slice_from_addr_len(addr: usize, len_bytes: usize) -> NonNull<[u8]> {
+//     let ptr = addr as *mut u8;
+//     NonNull::new_unchecked(core::slice::from_raw_parts_mut(ptr, len_bytes))
+// }
 
 unsafe impl<S: PageSize> FrameAllocator<S> for LockedPhysicalMemoryAllocator<'_> {
     fn allocate_frame(&mut self) -> Option<PhysFrame<S>> {

kernel/src/tests.rs

@@ -91,14 +91,6 @@ pub(crate) fn run_misc_tests() {
     serial_println!("vec at {:p}: {vec:?}", vec.as_slice());
     assert_eq!(vec.into_iter().sum::<u32>(), 45);
 
-    // Create a Box value with the `Allocator` API
-    let my_box = Box::new_in(42, &memory::KERNEL_PHYSICAL_ALLOCATOR);
-    serial_println!("Allocator alloc'ed my_box {my_box:?} at {my_box:p}");
-
-    drop(my_box);
-    let other_box = Box::new_in(42, &memory::KERNEL_PHYSICAL_ALLOCATOR);
-    serial_println!("Allocator alloc'ed other_box at {other_box:?} at {other_box:p}");
-
     // Trigger a page fault, which should trigger a double fault if we don't
     // have a page fault handler.
     // unsafe {