search

rust-osdev / x86_64

Library to program x86_64 hardware.
https://docs.rs/x86_64
Apache License 2.0
797 stars 132 forks source link

implement `core::iter::Step` for `VirtAddr` and `Page` #342

Closed Freax13 closed 2 years ago

Freax13 commented 2 years ago

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 https://github.com/rust-osdev/x86_64/issues/293#issuecomment-903260820. 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 commented 2 years ago

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 commented 2 years ago

[...] 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 commented 2 years ago

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

phil-opp commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.