implement core::iter::Step for VirtAddr and Page

[Freax13]

This pr implements core::iter::Step for VirtAddr and Page and adds corresponding unit tests.
This implementation jumps the gap between 0x7fff_ffff_ffff and 0xffff_8000_0000_0000 as discussed in #293 (comment).
The Step trait is still unstable, so the implementation is put behind a new feature flag step_trait. This flag is enabled by the nightly feature flag.

Part of #212

[phil-opp]

Sorry for not reviewing this earlier. The implementation looks good to me, but I'm still a bit unsure about jumping the gap because of Intel's 5-level paging. It would result in a smaller gap, but we should not change the behavior of stepping/ranges e.g. based on the CPU.

I guess that separate Page and VirtualAddress types for 5-level paging are probably the best solution. It's probably a good idea to be explicit about this anyway since it requires set up, e.g. a different page table format.

What do you think is the best approach?

[Freax13]

[...] we should not change the behavior of stepping/ranges e.g. based on the CPU.

I agree that we shouldn't do that based on the CPU's configuration especially because VirtAddr and Page are also available on non x86 platforms where we have no way to automatically infer whether 4-level paging or 5-level paging should be assumed.

I guess that separate Page and VirtualAddress types for 5-level paging are probably the best solution. It's probably a good idea to be explicit about this anyway since it requires set up, e.g. a different page table format.

Will there just be different types for Page and VirtAddr or will we need to add new types for other things e.g. Translate or Mapper and new methods e.g. flush?

What do you think is the best approach?

I wonder if it's possible to decide between 4-level paging and 5-level paging with a new cargo feature. Probably not because that would limit a kernel (or any other program using the crate) to only one of the two. We could also run into problems because cargo features are meant to be purely additive.

Maybe we could a new const generic parameter to all affected types to switch between 4-level paging and 5-level paging. This would probably allow reusing most types/functions/code for both modes.

Either way, adding support for 5-level paging will very likely be a breaking change anyways, so I'm not sure if we should worry a lot about being forward compatible just yet.

[Freax13]

I just rebased onto the new master to resolve some merge conflicts.

[phil-opp]

Will there just be different types for Page and VirtAddr or will we need to add new types for other things e.g. Translate or Mapper and new methods e.g. flush?

We might be able to make some methods generic, but e.g. Mapper::map_to would need to be duplicated as well, e.g. through a new trait, since not all page tables can map pages that use 5-level paging.

Changing behavior through cargo features is not a good idea because cargo unifies dependencies across the whole project. So adding a dependency to a crate that internally uses x86_64 with the 5-level paging feature enabled would also activate that feature for other x86_64 dependencies across the whole project.

Maybe we could a new const generic parameter to all affected types to switch between 4-level paging and 5-level paging. This would probably allow reusing most types/functions/code for both modes.

Yeah, we could do that too if it doesn't make the API too complicated.

Either way, adding support for 5-level paging will very likely be a breaking change anyways, so I'm not sure if we should worry a lot about being forward compatible just yet.

I don't think that this has to be a breaking change. Duplicating types would be fully backwards compatible. However, the feature is big enough that a new minor release would not be a problem either. But I still want to avoid silent behavior changes in minor releases, i.e. breaking changes that don't lead to any compile errors.

[phil-opp]

I think we can use the design as is. The gap jumping behavior seems logical and useful to me and we will probably need to add some separate types (or new generic parameters) for 5-level paging anyway, so this change doesn't make it more difficult to add support for 5-level paging later.

I left a few minor comments, afterwards this is ready to be merged from my side. Thanks again!

[phil-opp]

Perhaps add an assertion here that steps is larger than 0xffff_0000_0000_0000?

[Freax13]

Assuming that the address is canonical steps is always equal to or larger than 0xffff_0000_0000_0000, is it not? I would be fine with adding a debug assertion just to make sure.

[phil-opp]

What happens if count is so large that this get_bits returns 0x3 or more? We should probably return None, right?

[Freax13]

What happens if count is so large that this get_bits returns 0x3 or more? We should probably return None, right?

Good point. Note that we can also get a larger value (0x1ffff) if start is in the upper half. We can check if offset is bigger than the total address space and return None if that's the case.

[phil-opp]

Same concern as above: If count is too large, this get_bits might return something smaller than 0x1fffe.

[phil-opp]

I don't think that the check that you added is enough. I pushed some more test cases to show examples that are still broken. To be safe, I also pushed a commit to re-check that addresses are canoncial after stepping, and I changed the steps_between to use checked_sub. These additional checks should not be needed for a correct implementation, so I'm happy to downgrade them to debug assertions at some point, but for now I think that it's better to be safe than fast here.

[Freax13]

I don't think that the check that you added is enough. I pushed some more test cases to show examples that are still broken.

The remaining issues should be fixed now.

To be safe, I also pushed a commit to re-check that addresses are canoncial after stepping, and I changed the steps_between to use checked_sub. These additional checks should not be needed for a correct implementation, so I'm happy to downgrade them to debug assertions at some point, but for now I think that it's better to be safe than fast here.

Fair enough.