Skip to content

Commit

Permalink
Revert "Add new UnsafePhysFrame type and use it in Mapper::map_to"
Browse files Browse the repository at this point in the history
This reverts commit 0c8756f.
  • Loading branch information
phil-opp committed Apr 11, 2020
1 parent 6bd9bbe commit 8a257cc
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 77 deletions.
42 changes: 3 additions & 39 deletions src/structures/paging/frame_alloc.rs
Original file line number Diff line number Diff line change
@@ -1,54 +1,18 @@
//! Traits for abstracting away frame allocation and deallocation.
use crate::structures::paging::{PageSize, PhysFrame, Size4KiB};
use core::ops::{Deref, DerefMut};
use crate::structures::paging::{PageSize, PhysFrame};

/// A trait for types that can allocate a frame of memory.
///
/// This trait is unsafe to implement because the implementer must guarantee that
/// the `allocate_frame` method returns only unique unused frames.
pub unsafe trait FrameAllocator<S: PageSize> {
/// Allocate a frame of the appropriate size and return it if possible.
fn allocate_frame(&mut self) -> Option<UnusedPhysFrame<S>>;
fn allocate_frame(&mut self) -> Option<PhysFrame<S>>;
}

/// A trait for types that can deallocate a frame of memory.
pub trait FrameDeallocator<S: PageSize> {
/// Deallocate the given frame of memory.
fn deallocate_frame(&mut self, frame: UnusedPhysFrame<S>);
}

/// Represents a physical frame that is not used for any mapping.
#[derive(Debug)]
pub struct UnusedPhysFrame<S: PageSize = Size4KiB>(PhysFrame<S>);

impl<S: PageSize> UnusedPhysFrame<S> {
/// Creates a new UnusedPhysFrame from the given frame.
///
/// ## Safety
///
/// This method is unsafe because the caller must guarantee
/// that the given frame is unused.
pub unsafe fn new(frame: PhysFrame<S>) -> Self {
Self(frame)
}

/// Returns the physical frame as `PhysFrame` type.
pub fn frame(self) -> PhysFrame<S> {
self.0
}
}

impl<S: PageSize> Deref for UnusedPhysFrame<S> {
type Target = PhysFrame<S>;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl<S: PageSize> DerefMut for UnusedPhysFrame<S> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
fn deallocate_frame(&mut self, frame: PhysFrame<S>);

This comment has been minimized.

Copy link
@m-ou-se

m-ou-se Apr 11, 2020

Contributor

Shouldn't FrameDeallocator::deallocate_frame be unsafe now, now that it's no longer protected by UnusedPhysFrame being unsafe to make?

This comment has been minimized.

Copy link
@phil-opp

phil-opp Apr 11, 2020

Author Member

Good catch! It seems like this method was incorrectly safe since it was introduced in #30, so the revert introduced that bug again.

Do you want to create a PR for this or should I?

This comment has been minimized.

Copy link
@m-ou-se

m-ou-se Apr 11, 2020

Contributor

Do you want to create a PR for this or should I?

Go ahead. I'm working on another PR now ^^'

This comment has been minimized.

Copy link
@phil-opp

phil-opp Apr 11, 2020

Author Member

Ok, thanks again for reporting!

This comment has been minimized.

Copy link
@phil-opp

phil-opp Apr 11, 2020

Author Member

I filed #146 for this.

}
25 changes: 11 additions & 14 deletions src/structures/paging/mapper/mapped_page_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
fn map_to_1gib<A>(
&mut self,
page: Page<Size1GiB>,
frame: UnusedPhysFrame<Size1GiB>,
frame: PhysFrame<Size1GiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size1GiB>, MapToError<Size1GiB>>
Expand All @@ -66,7 +66,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
fn map_to_2mib<A>(
&mut self,
page: Page<Size2MiB>,
frame: UnusedPhysFrame<Size2MiB>,
frame: PhysFrame<Size2MiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size2MiB>, MapToError<Size2MiB>>
Expand Down Expand Up @@ -94,7 +94,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
fn map_to_4kib<A>(
&mut self,
page: Page<Size4KiB>,
frame: UnusedPhysFrame<Size4KiB>,
frame: PhysFrame<Size4KiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size4KiB>, MapToError<Size4KiB>>
Expand All @@ -115,17 +115,17 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
if !p1[page.p1_index()].is_unused() {
return Err(MapToError::PageAlreadyMapped(frame));
}
p1[page.p1_index()].set_frame(frame.frame(), flags);
p1[page.p1_index()].set_frame(frame, flags);

Ok(MapperFlush::new(page))
}
}

impl<'a, P: PhysToVirt> Mapper<Size1GiB> for MappedPageTable<'a, P> {
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size1GiB>,
frame: UnusedPhysFrame<Size1GiB>,
frame: PhysFrame<Size1GiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size1GiB>, MapToError<Size1GiB>>
Expand Down Expand Up @@ -195,10 +195,10 @@ impl<'a, P: PhysToVirt> Mapper<Size1GiB> for MappedPageTable<'a, P> {
}

impl<'a, P: PhysToVirt> Mapper<Size2MiB> for MappedPageTable<'a, P> {
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size2MiB>,
frame: UnusedPhysFrame<Size2MiB>,
frame: PhysFrame<Size2MiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size2MiB>, MapToError<Size2MiB>>
Expand Down Expand Up @@ -276,10 +276,10 @@ impl<'a, P: PhysToVirt> Mapper<Size2MiB> for MappedPageTable<'a, P> {
}

impl<'a, P: PhysToVirt> Mapper<Size4KiB> for MappedPageTable<'a, P> {
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size4KiB>,
frame: UnusedPhysFrame<Size4KiB>,
frame: PhysFrame<Size4KiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size4KiB>, MapToError<Size4KiB>>
Expand Down Expand Up @@ -465,10 +465,7 @@ impl<P: PhysToVirt> PageTableWalker<P> {

if entry.is_unused() {
if let Some(frame) = allocator.allocate_frame() {
entry.set_frame(
frame.frame(),
PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
);
entry.set_frame(frame, PageTableFlags::PRESENT | PageTableFlags::WRITABLE);
created = true;
} else {
return Err(PageTableCreateError::FrameAllocationFailed);
Expand Down
17 changes: 10 additions & 7 deletions src/structures/paging/mapper/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@ pub use self::mapped_page_table::{MappedPageTable, PhysToVirt};
pub use self::{offset_page_table::OffsetPageTable, recursive_page_table::RecursivePageTable};

use crate::structures::paging::{
frame_alloc::{FrameAllocator, UnusedPhysFrame},
page_table::PageTableFlags,
Page, PageSize, PhysFrame, Size1GiB, Size2MiB, Size4KiB,
frame_alloc::FrameAllocator, page_table::PageTableFlags, Page, PageSize, PhysFrame, Size1GiB,
Size2MiB, Size4KiB,
};
use crate::{PhysAddr, VirtAddr};

Expand Down Expand Up @@ -83,10 +82,13 @@ pub trait Mapper<S: PageSize> {
///
/// This function might need additional physical frames to create new page tables. These
/// frames are allocated from the `allocator` argument. At most three frames are required.
fn map_to<A>(
///
/// This function is unsafe because the caller must guarantee that passed `frame` is
/// unused, i.e. not used for any other mappings.
unsafe fn map_to<A>(
&mut self,
page: Page<S>,
frame: UnusedPhysFrame<S>,
frame: PhysFrame<S>,
flags: PageTableFlags,
frame_allocator: &mut A,
) -> Result<MapperFlush<S>, MapToError<S>>
Expand Down Expand Up @@ -116,11 +118,12 @@ pub trait Mapper<S: PageSize> {
///
/// ## Safety
///
/// TODO: Should this function be safe?
/// This function is unsafe because the caller must guarantee that the passed `frame` is
/// unused, i.e. not used for any other mappings.
#[inline]
unsafe fn identity_map<A>(
&mut self,
frame: UnusedPhysFrame<S>,
frame: PhysFrame<S>,
flags: PageTableFlags,
frame_allocator: &mut A,
) -> Result<MapperFlush<S>, MapToError<S>>
Expand Down
12 changes: 6 additions & 6 deletions src/structures/paging/mapper/offset_page_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ impl PhysToVirt for PhysOffset {
// delegate all trait implementations to inner

impl<'a> Mapper<Size1GiB> for OffsetPageTable<'a> {
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size1GiB>,
frame: UnusedPhysFrame<Size1GiB>,
frame: PhysFrame<Size1GiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size1GiB>, MapToError<Size1GiB>>
Expand Down Expand Up @@ -91,10 +91,10 @@ impl<'a> Mapper<Size1GiB> for OffsetPageTable<'a> {

impl<'a> Mapper<Size2MiB> for OffsetPageTable<'a> {
#[inline]
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size2MiB>,
frame: UnusedPhysFrame<Size2MiB>,
frame: PhysFrame<Size2MiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size2MiB>, MapToError<Size2MiB>>
Expand Down Expand Up @@ -129,10 +129,10 @@ impl<'a> Mapper<Size2MiB> for OffsetPageTable<'a> {

impl<'a> Mapper<Size4KiB> for OffsetPageTable<'a> {
#[inline]
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size4KiB>,
frame: UnusedPhysFrame<Size4KiB>,
frame: PhysFrame<Size4KiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size4KiB>, MapToError<Size4KiB>>
Expand Down
22 changes: 11 additions & 11 deletions src/structures/paging/mapper/recursive_page_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl<'a> RecursivePageTable<'a> {

if entry.is_unused() {
if let Some(frame) = allocator.allocate_frame() {
entry.set_frame(frame.frame(), Flags::PRESENT | Flags::WRITABLE);
entry.set_frame(frame, Flags::PRESENT | Flags::WRITABLE);
created = true;
} else {
return Err(MapToError::FrameAllocationFailed);
Expand Down Expand Up @@ -141,7 +141,7 @@ impl<'a> RecursivePageTable<'a> {
fn map_to_1gib<A>(
&mut self,
page: Page<Size1GiB>,
frame: UnusedPhysFrame<Size1GiB>,
frame: PhysFrame<Size1GiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size1GiB>, MapToError<Size1GiB>>
Expand All @@ -167,7 +167,7 @@ impl<'a> RecursivePageTable<'a> {
fn map_to_2mib<A>(
&mut self,
page: Page<Size2MiB>,
frame: UnusedPhysFrame<Size2MiB>,
frame: PhysFrame<Size2MiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size2MiB>, MapToError<Size2MiB>>
Expand Down Expand Up @@ -196,7 +196,7 @@ impl<'a> RecursivePageTable<'a> {
fn map_to_4kib<A>(
&mut self,
page: Page<Size4KiB>,
frame: UnusedPhysFrame<Size4KiB>,
frame: PhysFrame<Size4KiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size4KiB>, MapToError<Size4KiB>>
Expand All @@ -217,17 +217,17 @@ impl<'a> RecursivePageTable<'a> {
if !p1[page.p1_index()].is_unused() {
return Err(MapToError::PageAlreadyMapped(frame));
}
p1[page.p1_index()].set_frame(frame.frame(), flags);
p1[page.p1_index()].set_frame(frame, flags);

Ok(MapperFlush::new(page))
}
}

impl<'a> Mapper<Size1GiB> for RecursivePageTable<'a> {
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size1GiB>,
frame: UnusedPhysFrame<Size1GiB>,
frame: PhysFrame<Size1GiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size1GiB>, MapToError<Size1GiB>>
Expand Down Expand Up @@ -310,10 +310,10 @@ impl<'a> Mapper<Size1GiB> for RecursivePageTable<'a> {

impl<'a> Mapper<Size2MiB> for RecursivePageTable<'a> {
#[inline]
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size2MiB>,
frame: UnusedPhysFrame<Size2MiB>,
frame: PhysFrame<Size2MiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size2MiB>, MapToError<Size2MiB>>
Expand Down Expand Up @@ -415,10 +415,10 @@ impl<'a> Mapper<Size2MiB> for RecursivePageTable<'a> {
}

impl<'a> Mapper<Size4KiB> for RecursivePageTable<'a> {
fn map_to<A>(
unsafe fn map_to<A>(
&mut self,
page: Page<Size4KiB>,
frame: UnusedPhysFrame<Size4KiB>,
frame: PhysFrame<Size4KiB>,
flags: PageTableFlags,
allocator: &mut A,
) -> Result<MapperFlush<Size4KiB>, MapToError<Size4KiB>>
Expand Down

0 comments on commit 8a257cc

Please sign in to comment.