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 trait for for PhysFrame #292

Conversation

ncatelli
Copy link

General

This is an initial naive first pass at implementing core::iter::Step for PhysFrame. I initially had it align off the start address of a frame and look for overflows of the u64 space from the start. But I can imagine it would be worth checking chat the end overflows as well and figured I'd post to get your feedback before deciding.

Related Issues

#212

@ncatelli
Copy link
Author

blocked by fix linked in #294

@josephlr
Copy link
Contributor

josephlr commented Aug 19, 2021

@ncatelli can you implement this on PhysAddr as well, and then just forward the PhysFrame implementation to the PhysAddr impl? That way we will have a single place that we check for overflow of the physical address space.

EDIT: we've also fixed the CI issue, so you should be able to rebase and have things work

@ncatelli ncatelli force-pushed the feature/212/implement-step-trait-for-PhysFrame branch from 5e29388 to 9bdd526 Compare August 19, 2021 15:25
@ncatelli ncatelli force-pushed the feature/212/implement-step-trait-for-PhysFrame branch from 9bdd526 to 644cc68 Compare August 19, 2021 15:26
@ncatelli
Copy link
Author

@ncatelli can you implement this on PhysAddr as well, and then just forward the PhysFrame implementation to the PhysAddr impl? That way we will have a single place that we check for overflow of the physical address space.

EDIT: we've also fixed the CI issue, so you should be able to rebase and have things work

These changes have been applied per your request.

@ncatelli
Copy link
Author

For now I've used an implementation of checked_add/checked_sub on the u64 and usize if #293 is completed prior to this merging i will migrate to use that. I'll leave that up to you if that should be blocking on this but I'll take the ownership of keeping this up to date and, if you prefer, integrating that change.

Copy link
Contributor

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

I think we should implement these in terms of the checked_add/checked_sub implementations. This will result in smaller and more consistent code. Feel free to just have this PR depend on #298

Comment on lines +135 to +151
#[cfg(feature = "step_trait")]
impl<S: PageSize> core::iter::Step for PhysFrame<S> {
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
PhysAddr::steps_between(&start.start_address, &end.start_address)
}

fn forward_checked(start: Self, count: usize) -> Option<Self> {
PhysAddr::forward_checked(start.start_address, count)
.and_then(|start_addr| Self::from_start_address(start_addr).ok())
}

fn backward_checked(start: Self, count: usize) -> Option<Self> {
PhysAddr::backward_checked(start.start_address, count)
.and_then(|start_addr| Self::from_start_address(start_addr).ok())
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

These implementations are incorrect. We need to be taking the page size into account (so that we are consistent with the PhysFrame<S>: Add<u64> implementation).

Specifically, steps_between should be:

end.checked_sub(start)?.try_into() 

and forward_checked should be:

start.checked_add(count.try_into()?)

(similar for backward_checked)

@ncatelli
Copy link
Author

Oof, that was my mistake, I tried to move it quick to get feedback and didn't think it through. Sorry for the inconvenience. I agree with your above sentiment that it's it should just wait on #293 I'm going to close this out for now and will revisit this after we settle on the implementations for at least the addressing types.

@ncatelli ncatelli closed this Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants