Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Mar 1, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ volatile = "0.4.4"
[features]
default = [ "nightly", "instructions" ]
instructions = []
nightly = [ "const_fn", "abi_x86_interrupt", "doc_cfg" ]
nightly = [ "const_fn", "step_trait", "abi_x86_interrupt", "doc_cfg" ]
abi_x86_interrupt = []
const_fn = []
step_trait = []
doc_cfg = []

# These features are no longer used and only there for backwards compatibility.
Expand Down
112 changes: 112 additions & 0 deletions src/addr.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
//! Physical and virtual addresses manipulation

#[cfg(feature = "step_trait")]
use core::convert::TryFrom;
use core::fmt;
#[cfg(feature = "step_trait")]
use core::iter::Step;
use core::ops::{Add, AddAssign, Sub, SubAssign};

use crate::structures::paging::page_table::PageTableLevel;
Expand Down Expand Up @@ -322,6 +326,44 @@ impl Sub<VirtAddr> for VirtAddr {
}
}

#[cfg(feature = "step_trait")]
impl Step for VirtAddr {
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
let mut steps = end.0.checked_sub(start.0)?;

// Check if we jumped the gap.
if end.0.get_bit(47) && !start.0.get_bit(47) {
steps -= 0xffff_0000_0000_0000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

usize::try_from(steps).ok()
}

fn forward_checked(start: Self, count: usize) -> Option<Self> {
let offset = u64::try_from(count).ok()?;
let mut addr = start.0.checked_add(offset)?;

// Jump the gap by sign extending the 47th bit.
if addr.get_bits(47..) == 0x1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

addr.set_bits(47.., 0x1ffff);
}

Some(Self(addr))
}

fn backward_checked(start: Self, count: usize) -> Option<Self> {
let offset = u64::try_from(count).ok()?;
let mut addr = start.0.checked_sub(offset)?;

// Jump the gap by sign extending the 47th bit.
if addr.get_bits(47..) == 0x1fffe {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

addr.set_bits(47.., 0);
}

Some(Self(addr))
}
}

/// A passed `u64` was not a valid physical address.
///
/// This means that bits 52 to 64 were not all null.
Expand Down Expand Up @@ -577,6 +619,76 @@ mod tests {
assert_eq!(VirtAddr::new_truncate(123 << 47), VirtAddr(0xfffff << 47));
}

#[test]
#[cfg(feature = "step_trait")]
fn virtaddr_step() {
assert_eq!(Step::forward(VirtAddr(0), 0), VirtAddr(0));
assert_eq!(Step::forward(VirtAddr(0), 1), VirtAddr(1));
assert_eq!(
Step::forward(VirtAddr(0x7fff_ffff_ffff), 1),
VirtAddr(0xffff_8000_0000_0000)
);
assert_eq!(
Step::forward(VirtAddr(0xffff_8000_0000_0000), 1),
VirtAddr(0xffff_8000_0000_0001)
);
assert_eq!(
Step::forward_checked(VirtAddr(0xffff_ffff_ffff_ffff), 1),
None
);

assert_eq!(Step::backward(VirtAddr(0), 0), VirtAddr(0));
assert_eq!(Step::backward_checked(VirtAddr(0), 1), None);
assert_eq!(Step::backward(VirtAddr(1), 1), VirtAddr(0));
assert_eq!(
Step::backward(VirtAddr(0xffff_8000_0000_0000), 1),
VirtAddr(0x7fff_ffff_ffff)
);
assert_eq!(
Step::backward(VirtAddr(0xffff_8000_0000_0001), 1),
VirtAddr(0xffff_8000_0000_0000)
);

assert_eq!(Step::steps_between(&VirtAddr(0), &VirtAddr(0)), Some(0));
assert_eq!(Step::steps_between(&VirtAddr(0), &VirtAddr(1)), Some(1));
assert_eq!(Step::steps_between(&VirtAddr(1), &VirtAddr(0)), None);
assert_eq!(
Step::steps_between(
&VirtAddr(0x7fff_ffff_ffff),
&VirtAddr(0xffff_8000_0000_0000)
),
Some(1)
);
assert_eq!(
Step::steps_between(
&VirtAddr(0xffff_8000_0000_0000),
&VirtAddr(0x7fff_ffff_ffff)
),
None
);
assert_eq!(
Step::steps_between(
&VirtAddr(0xffff_8000_0000_0000),
&VirtAddr(0xffff_8000_0000_0000)
),
Some(0)
);
assert_eq!(
Step::steps_between(
&VirtAddr(0xffff_8000_0000_0000),
&VirtAddr(0xffff_8000_0000_0001)
),
Some(1)
);
assert_eq!(
Step::steps_between(
&VirtAddr(0xffff_8000_0000_0001),
&VirtAddr(0xffff_8000_0000_0000)
),
None
);
}

#[test]
pub fn test_align_up() {
// align 1
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#![cfg_attr(feature = "const_fn", feature(const_fn_fn_ptr_basics))] // IDT new()
#![cfg_attr(feature = "const_fn", feature(const_fn_trait_bound))] // PageSize marker trait
#![cfg_attr(feature = "abi_x86_interrupt", feature(abi_x86_interrupt))]
#![cfg_attr(feature = "step_trait", feature(step_trait))]
#![cfg_attr(feature = "doc_cfg", feature(doc_cfg))]
#![warn(missing_docs)]
#![deny(missing_debug_implementations)]
Expand Down
28 changes: 28 additions & 0 deletions src/structures/paging/page.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use crate::structures::paging::page_table::PageTableLevel;
use crate::structures::paging::PageTableIndex;
use crate::VirtAddr;
use core::fmt;
#[cfg(feature = "step_trait")]
use core::iter::Step;
use core::marker::PhantomData;
use core::ops::{Add, AddAssign, Sub, SubAssign};

Expand Down Expand Up @@ -274,6 +276,32 @@ impl<S: PageSize> Sub<Self> for Page<S> {
}
}

#[cfg(feature = "step_trait")]
impl<S: PageSize> Step for Page<S> {
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Step::steps_between(&start.start_address, &end.start_address)
.map(|steps| steps / S::SIZE as usize)
}

fn forward_checked(start: Self, count: usize) -> Option<Self> {
let count = count.checked_mul(S::SIZE as usize)?;
let start_address = Step::forward_checked(start.start_address, count)?;
Some(Self {
start_address,
size: PhantomData,
})
}

fn backward_checked(start: Self, count: usize) -> Option<Self> {
let count = count.checked_mul(S::SIZE as usize)?;
let start_address = Step::backward_checked(start.start_address, count)?;
Some(Self {
start_address,
size: PhantomData,
})
}
}

/// A range of pages with exclusive upper bound.
#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[repr(C)]
Expand Down