x86_64 icon indicating copy to clipboard operation
x86_64 copied to clipboard

Support mapping ranges

Open Freax13 opened this issue 4 years ago • 9 comments

This pr adds various methods for modifying a range of pages:

  • Mapper::map_to_range
  • Mapper::map_to_range_with_table_flags
  • Mapper::map_range
  • Mapper::map_range_with_table_flags
  • Mapper::unmap_range
  • Mapper::update_flags_range
  • Mapper::identity_map_range

All of those functions have a default implementation that just calls the non-ranged version for each page. MappedPageTable, OffsetPageTable and RecursivePageTable have specialized implementations that avoid looking up the P1-P3 tables multiple times which should improve performance (I haven't run any benchmarks to verify this, the kernel I used to test the pr doesn't work with timers yet lol).

Closes #192

Sort of depends on #136: The default implementation for Mapper::map_range_with_table_flags should use Mapper::map_with_table_flags instead of Mapper::map_to_with_table_flags once #136 is merged.

Freax13 avatar Jun 07 '21 18:06 Freax13

My proposal is the following: We merge this PR, but hide all the new methods behind an optional cargo feature named experimental. We then document clearly that the methods are only sparsely tested and might still contain bugs. We should also link to a tracking issue where users can report bugs, but also positive feedback (if everything works as intended). What do you think about this approach?

Sounds good to me.

Freax13 avatar Sep 04 '21 15:09 Freax13

This would be a breaking change, so I should set next as the base branch, correct? Should I still merge master into my branch?

Freax13 avatar Sep 05 '21 11:09 Freax13

Why do you think that it would be a breaking change?

phil-opp avatar Sep 13 '21 07:09 phil-opp

Why do you think that it would be a breaking change?

It adds methods to a trait, but they all have default implementations, so I think this is a minor non-breaking change. Unless I'm missing something.

Reference: https://doc.rust-lang.org/cargo/reference/semver.html#possibly-breaking-adding-a-defaulted-trait-item

josephlr avatar Sep 13 '21 07:09 josephlr

Oops my bad, this isn't a breaking change.

Freax13 avatar Sep 13 '21 08:09 Freax13

What's the status of this? In the PR description you mention that this depends on #136. Is this still true?

Also, should this go into 0.15 or is this a non-breaking change that can be merged directly into master (the recent comments seem to indicate the latter).

phil-opp avatar Oct 17 '21 17:10 phil-opp

What's the status of this? In the PR description you mention that this depends on #136. Is this still true?

Also, should this go into 0.15 or is this a non-breaking change that can be merged directly into master (the recent comments seem to indicate the latter).

Thanks for reminding me!

The pr sort of depends on #136 because of Mapper::map_range_with_table_flags: Usually the default implementations for the *_range methods call the non-range versions, but Mapper::map_range_with_table_flags can't do that until #136 adds Mapper::map_with_table_flags. Currently Mapper::map_range_with_table_flags just allocates a frame and calls Mapper::map_to_with_table_flags. Other than that the pr should be ready to merge.

This pr should not be a breaking change regardless of the experimental flag. Adding methods with a default implementation to a trait is not a breaking change.

I just rebased the branch onto master, I'd appreciate it if you could double check whether I did this correctly (I already fixed a few mistakes).

Freax13 avatar Oct 17 '21 17:10 Freax13

I just rebased this with the current master branch and got rid of some commits that were accidentally included from the next branch.

Freax13 avatar Dec 26 '21 12:12 Freax13

My proposal is the following: We merge this PR, but hide all the new methods behind an optional cargo feature named experimental. We then document clearly that the methods are only sparsely tested and might still contain bugs. We should also link to a tracking issue where users can report bugs, but also positive feedback (if everything works as intended). What do you think about this approach?

We could also use a cfg flag instead of a feature. This is done in other libraries such as tokio and tracing.

Freax13 avatar Apr 17 '22 12:04 Freax13