Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mapper #88

Closed
gnzlbg opened this issue Aug 19, 2019 · 7 comments · Fixed by #89
Closed

Mapper #88

gnzlbg opened this issue Aug 19, 2019 · 7 comments · Fixed by #89

Comments

@gnzlbg
Copy link

gnzlbg commented Aug 19, 2019

There are two methods in the Mapper trait that are unsafe. What would it take to make them safe ? Could we, e.g., error or panic if the frame is actually not unused ?

@phil-opp
Copy link
Member

The map_to and identity_map are unsafe because the passed PhysFrame must be unused. There is no way to efficiently check whether a frame is used because we would need to traverse the whole page table hierarchie. While there are ways to make this lookup effective (e.g. an inverted page table), this isn't really something that the library can provide because it's too opinionated.

We could of course add an UnusedPhysFrame wrapper type that is unsafe to construct. Using this type, we could create safe variants of the map_to and identity_map methods. However, this would only shift the unsafety to a different place, so I'm not sure if that is what you want.

@gnzlbg
Copy link
Author

gnzlbg commented Aug 20, 2019

Could we cache whether the frame is used/unused within the frame itself ?

@phil-opp
Copy link
Member

There might not be any memory available for this information in the frame. For example, imagine that we want to map a frame as stack memory: The CPU will potentially use every byte in that frame.

@gnzlbg
Copy link
Author

gnzlbg commented Aug 20, 2019

I meant in the PhysFrame type, as a field.

@phil-opp
Copy link
Member

The problem is that PhysFrame has safe constructor methods, so nothing is preventing the programmer from constructing two PhysFrame instances that represent the same frame.

I think the UnusedPhysFrame type could make sense since the FrameAllocator already uses a similar approach (unsafe to implement, returned frames must be unused). By making the FrameAllocator return UnusedPhysFrames, most of the time no unsafe is needed for calling map_to etc.

@gnzlbg
Copy link
Author

gnzlbg commented Aug 20, 2019

Sounds good!

@phil-opp
Copy link
Member

I opened #89 with these changes.

bors bot added a commit that referenced this issue Dec 5, 2019
84: Make Mapper trait object safe by adding `Self: Sized` bounds on generic functions r=phil-opp a=phil-opp

See #80 for more information

I'm not quite sure whether this is a **breaking change**, but I think it is.

89: Add new UnsafePhysFrame type and use it in Mapper::map_to r=phil-opp a=phil-opp

Fixes #88 

This pull request adds a new `UnsafePhysFrame` type that wraps a `PhysFrame`. The type is unsafe to construct and the caller must guarantee that the frame is not mapped already. This type allows to make the `map_to` and `identity_map` methods of the `Mapper` trait safe. This PR also adjust the `FrameAllocator` to use the new type.

This is a **breaking change**.



Co-authored-by: Philipp Oppermann <dev@phil-opp.com>
bors bot added a commit that referenced this issue Dec 5, 2019
84: Make Mapper trait object safe by adding `Self: Sized` bounds on generic functions r=phil-opp a=phil-opp

See #80 for more information

I'm not quite sure whether this is a **breaking change**, but I think it is.

89: Add new UnsafePhysFrame type and use it in Mapper::map_to r=phil-opp a=phil-opp

Fixes #88 

This pull request adds a new `UnsafePhysFrame` type that wraps a `PhysFrame`. The type is unsafe to construct and the caller must guarantee that the frame is not mapped already. This type allows to make the `map_to` and `identity_map` methods of the `Mapper` trait safe. This PR also adjust the `FrameAllocator` to use the new type.

This is a **breaking change**.



Co-authored-by: Philipp Oppermann <dev@phil-opp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants