The UnusedPhysFrame requirement is too strict for map_to

[phil-opp]

The UnusedPhysFrame type was added in #89 in order to make the map_to method safe. It has an unsafe constructor function that requires a "physical frame that is not used for any mapping".

As reported in phil-opp/blog_os#570 (comment), there are however valid use cases for using the same frame in multiple mappings. I therefore think that we should reduce the (documented) requirements or introduce a new wrapper type.

One potential solution could be the following:

• Add a new UnaliasedPhysFrame wrapper type that represents a PhysFrame where no undefined (mutable) aliasing can occur. Examples are copy-on-write mappings or duplicate mappings where it is guaranteed that one mapping isn't used. It's up to the programmer to ensure this invariant, so it's unsafe to construct the type.
• Add a impl From<UnusedPhysFrame> for UnaliasedPhysFrame implementation since every unused physical frame is also guaranteed to be unaliased.
• Change map_to to expect an Into<UnaliasedPhysFrame> so that it works with both frame wrapper types.

Alternatively, we could rename the UnusedPhysFrame type to UnaliasedPhysFrame without introducing a second type. The main argument for this is that no frame is really unused in kernels that use a mapping of the complete physical memory for accessing page tables. To avoid breakage, we could add a deprecated reexport of the old type name for some time.

[phil-opp]

@foxcob What do you think about these potential solutions?

[foxcob]

Interesting solutions. I'm thinking through the different usage scenarios. Your idea of the UnaliasedPhysFrame could be extended to explicitly allow or disallow multiple mappings. For instance a SharedPhysFrame could allow cloning and be added multiple times or to multiple page tables. An additional property is_mutable would be different from the page table writable permission. It would tell you that it points to mutable data and whether it was legal to map with a writable flag. Meaning a writable alias may exist or maybe it's memory mapped I/O. Not sure if it'd be better to have different PhysFrame types to support this or a generic PhysFrame type type that had a get_type or get_permissions method. Either way, it would be nice to have an assurance that an UnaliasedPhysFrame (or PhysFrame with no aliasable permission) wouldn't be aliased and vice-versa. map_to could prevent setting the writable flag with a frame that didn't have the is_mutable.

I do think that with an UnaliasedPhysFrame type (or PhysFrame with no aliasable permission) the need for an UnusedPhysFrame becomes redundant.

The case for mapping all of physical memory for page table access feels special somehow. Either it needs to be mapped in a way that is "outside" of the frame management code, or blanket mapping is disallowed.

I feel like I rambled on a little bit here, but I'm trying to figure out a structured way to support these concepts without building it around a specific usage implementation. What do you think?

[phil-opp]

The advantage of a separate UnaliasedPhysFrame type is that we can encode this requirement at type system level in the function signature. In contrast, we would need to do a runtime check and return a Result when checking permissions of a PhysFrame type.

Supporting all use cases of frames in this crate does not seem reasonable, so I think the best solution is to just rename UnusedPhysFrame to UnaliasedPhysFrame, at least for now. This way, we can avoid the confusion and properly document the requirements on the type.

Types like SharedPhysFrame can be still created on the kernel site with an Into<SharedPhysFrame> implementation, if one wants to build additional abstractions.

[phil-opp]

Thinking more about this, I would like to propose a different solution: Make map_to unsafe again and remove the UnusedPhysFrame wrapper completely. See #134 for more information.

[foxcob]

I think that is probably the simplest solution. It also does not exclude people from building some of those other ideas on top of it. This removes policy from the implementation which I think is probably more appropriate for something like this.

[phil-opp]

Ok, then let's do this! Closing this issue in favor of #134.