Non-zero but non-present page table entries are handled poorly.

[pjht]

unmap and map_to have different semantics for determining when a page is mapped, which causes problems handling entries that are non-zero but do not have the present flag set. translate mixes both semantics.

unmap checks the entry's present flag, while map_to checks if the entry is non-zero.
translate has mixed behavior. Non-present 4KiB page entries with bits set are returned as mapped, while non-present huge page entries with the huge page flag and potentially other bits set but not the present flag are returned as unmapped. (huge page behavior inferred from reading the code, do not use huge pages in my OS and have no easy way of confirming that).

This causes issues trying to store info about unmapped but mappable on-demand pages in the page tables. I ran into this trying to add memory overcommit to my kernel, wanting to set an OS-reserved bit in the entry to mark it as being allocated but not backed by physical memory yet. Making the entry worked, but in the code to back the allocated page on initial access and thus page fault, map errored saying the page was already mapped while unmap errored saying the page wasn't mapped. I made it work by calling update_flags to set the present flag on the entry than calling unmap followed by map but it's not an ideal solution. I would expect to be able to just call map on the entry since the entry's not marked present.

translate should definitely be made consistent between huge pages and regular pages, but I'm not sure whether non-zero non-present entries should be considered mapped. Only having to call map is somewhat more elegant but runs the risk of accidentally overwriting stored info if you mix up a page. Having to call unmap before that fixes things but might(?) add overhead and an unnecessary TLB flush. Maybe add a ```(re?)map_nonzero_nonpresent`` method? (obviously that wouldn't be the actual name)

Also, if translate gets changed to return non-zero non-present page entries as unmapped, the flags of the unmapped entry should be added to TranslateResult, as well as the level the translation stopped at (to determine page size), as otherwise there would be no easy way to retrieve the stored info on a page fault with available APIs outside of writing your own table walker.

[Freax13]

Thanks for the detailed write-up!

I think we should require the PRESENT flag in translate. The documentation for translate says:

Return the frame that the given virtual address is mapped to and the offset within that frame.

To me, that heavily implies that the page can actually be accessed, so the PRESENT flag must be set.

If someone sets bits in the entry without setting the PRESENT bit, chances are they're encoding their own information and don't necessarily even follow the same format as regular page table entries. For example, I could imagine that one could implement demand paging by encoding a pointer to a VMA object in the page table bits. If we treat page table entries without the PRESENT bit as opaque values, it follows that we should never try to interpret their bits as a PhysFrame and so translate shouldn't return one. For entries without the PRESENT bit, we might want to just return the PageTableEntry.

I think we should change map_to to ignore and replace any entries where PRESENT is not already set. Currently, if there's a non-zero entry, we return MapToError::PageAlreadyMapped(PhysFrame<S>). Again, to me, this implies that the page is mapped with the PRESENT bit set and that accessing the page will access the physical frame.

I'm a bit wary about changing this in a non-breaking release. I'm concerned that it might be possible that users depend on one or more of these implementation quirks and that fixing these might break their code. I think if we want to address this, we should do so in a breaking release and document some of the more subtle ways we change these methods.

I think we're due for a breaking release anyway, cleaning up some of the page table code would be a nice addition for that.