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 conversion traits for PhysAddr and VirtAddr #394

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mikeleany
Copy link

Implements the following conversions:

  • TryFrom<u64> for VirtAddr and PhysAddr
  • TryFrom<usize> for VirtAddr and PhysAddr
  • From<u32> for VirtAddr and PhysAddr
  • TryFrom<*const T> for VirtAddr
  • TryFrom<*mut T> for VirtAddr
  • From<&T> for VirtAddr
  • From<&mut T> for VirtAddr
  • From<VirtAddr> for u64, *const T and *mut T
  • From<PhysAddr> for u64

I restricted pointer and usize conversions with #[cfg(any(target_pointer_width = "32", target_pointer_width = "64"))]. This restriction isn't strictly necessary for TryFrom, but without it we would need a different error type than what try_new uses.

Closes #268.
See also #293 (comment) by @josephlr.

@mikeleany mikeleany marked this pull request as ready for review October 23, 2022 02:39
@phil-opp
Copy link
Member

Thanks for the PR! Looks all reasonable to me.

cc @josephlr @Freax13 What do you think about these new impls?

src/addr.rs Outdated
Comment on lines 457 to 471
#[cfg(any(target_pointer_width = "32", target_pointer_width = "64"))]
impl<T> From<&T> for VirtAddr {
#[inline]
fn from(addr: &T) -> Self {
Self::new(addr as *const _ as u64)
}
}

#[cfg(any(target_pointer_width = "32", target_pointer_width = "64"))]
impl<T> From<&mut T> for VirtAddr {
#[inline]
fn from(addr: &mut T) -> Self {
Self::new(addr as *mut _ as u64)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

These could fail on different architectures. Do we want to care about that given that most users will probably run on x86?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, good point! I guess we could a target_arch cfg-gate to avoid this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that'd work

Copy link
Author

Choose a reason for hiding this comment

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

Changes these to target_arch. The target_pointer_width ought to be reasonable if the target_arch is x86 or x86_64, so I just removed those.

Copy link
Author

Choose a reason for hiding this comment

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

I was also wondering if we ought to use target_arch for the raw pointer conversions. I left them for now, since they implement TryFrom instead of From, but those really don't make a whole lot of sense on non-x86 based architectures.

src/addr.rs Outdated
}
}

#[cfg(any(target_pointer_width = "32", target_pointer_width = "64"))]
Copy link
Member

Choose a reason for hiding this comment

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

There's also target_pointer_width = "16".

Copy link
Author

Choose a reason for hiding this comment

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

Added target_pointer_width = "16" where applicable.

@Freax13
Copy link
Member

Freax13 commented Oct 24, 2022

Do we want to deprecate the old conversion methods/constructors?

@josephlr
Copy link
Contributor

Do we want to deprecate the old conversion methods/constructors?

I think the only issue here would be that some of those are const, while the trait implementations might not be (at least for now).

Otherwise the idea seems reasonable to me.

@Freax13
Copy link
Member

Freax13 commented Nov 3, 2022

We should unify the cfg gates for pointer conversions and similar operations, we use four different variations:

  • cfg(target_pointer_width = "64")
    • VirtAddr::as_ptr, VirtAddr::as_mut_ptr
    • <VirtAddr as Add<usize>>::add, <VirtAddr as AddAssign<usize>>::add_assign, <VirtAddr as Sub<usize>>::sub, <VirtAddr as SubAssign<usize>>::sub_assign, <PhysAddr as Add<usize>>::add, <PhysAddr as AddAssign<usize>>::add_assign, <PhysAddr as Sub<usize>>::sub, <PhysAddr as SubAssign<usize>>::sub_assign
      • These implementations are removed on the next branch because we implement these traits for u64.
    • <*const T as From<VirtAddr>>::from, <*mut T as From<VirtAddr>>::from
      • introduced by this pr
  • cfg(any(target_pointer_width = "32", target_pointer_width = "64"))
    • VirtAddr::from_ptr
      • There's a comment stating that we do this for backward compatibily reasons, this has already been changed to cfg(target_pointer_width = "64") on the next branch.
  • cfg(any(target_pointer_width = "16", target_pointer_width = "32", target_pointer_width = "64"))
    • <VirtAddr as TryFrom<usize>>::try_from, <VirtAddr as TryFrom<*const T>>::try_from and <VirtAddr as TryFrom<*mut T>>::try_from
      • introduced by this pr
  • cfg(any(target_arch = "x86", target_arch = "x86_64"))
    • <VirtAddr as From<&T>>::from and <VirtAddr as From<&mut T>>::from
      • introduced by this pr

It's not entirely clear to me which variation we should use.

Comment on lines +469 to +483
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
impl<T> From<&T> for VirtAddr {
#[inline]
fn from(addr: &T) -> Self {
Self::new(addr as *const _ as u64)
}
}

#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
impl<T> From<&mut T> for VirtAddr {
#[inline]
fn from(addr: &mut T) -> Self {
Self::new(addr as *mut _ as u64)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

These could technically fail if Intel's 5 level paging is active.

Copy link
Author

Choose a reason for hiding this comment

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

This possibility was mentioned in issue #293, here and here.

The last comment on the subject, by @josephlr was:

Personally, I think the panic is fine (for now). Before we (potentially) support 5-level paging, we can just have a disclaimer that is like: "if you use 5-level paging, some methods might panic, but safety will not be violated".

No disclaimer has been added yet, but I can add one if that's the course we want to take.

Copy link
Member

@Freax13 Freax13 Nov 7, 2022

Choose a reason for hiding this comment

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

Personally, I think the panic is fine (for now). Before we (potentially) support 5-level paging, we can just have a disclaimer that is like: "if you use 5-level paging, some methods might panic, but safety will not be violated".

That seems like a good pragmatic option to me. Intel's LAM (Linear Address Masking) and AMD's UAI (Upper Address Ignore) can also cause problems because they relax the requirements on pointers to be canonical.

No disclaimer has been added yet, but I can add one if that's the course we want to take.

That'd be great, thanks!

@mikeleany
Copy link
Author

@Freax13

We should unify the cfg gates for pointer conversions and similar operations, we use four different variations:

It's not entirely clear to me which variation we should use.

For conversions from pointers (and references), I think cfg(any(target_arch = "x86", target_arch = "x86_64")) makes the most sense to me because

  • there's a legitimate use case to convert x86 pointers to a VirtAddr when running 16-bit or 32-bit system initialization code prior to entering long mode,
  • but a pointer on any other architecture is not likely to correspond in any way with a virtual address on x86_64, even if the pointers happen to be the same size.

For conversions to pointers target_arch = "x86" cannot be allowed for infallible conversions. Perhaps we could implement From when on x86_64 and TryFrom on x86 if people want that.

usize is a little different since it's an integer rather than a pointer. However, as I think about it, I think it makes more sense to require conversion from a u64 rather than usize on non-x86 based architectures, even if usize happens to be 64 bits. Doing otherwise would encourage non-portable code.


So I think the following cfg gates might be the most reasonable in my view:

cfg(any(target_arch = "x86", target_arch = "x86_64")) whenever converting from pointers or usize to VirtAddr

cfg(target_arch = "x86_64) whenever performing infallible conversions from VirtAddr to pointers or usize

However, if I modified the cfg gates on the existing inherent methods, that would technically be a breaking change, though I'm not sure if that would actually affect anybody.

@Freax13
Copy link
Member

Freax13 commented Nov 7, 2022

For conversions from pointers (and references), I think cfg(any(target_arch = "x86", target_arch = "x86_64")) makes the most sense to me because

  • there's a legitimate use case to convert x86 pointers to a VirtAddr when running 16-bit or 32-bit system initialization code prior to entering long mode,

  • but a pointer on any other architecture is not likely to correspond in any way with a virtual address on x86_64, even if the pointers happen to be the same size.

For conversions to pointers target_arch = "x86" cannot be allowed for infallible conversions. Perhaps we could implement From when on x86_64 and TryFrom on x86 if people want that.

👍

usize is a little different since it's an integer rather than a pointer. However, as I think about it, I think it makes more sense to require conversion from a u64 rather than usize on non-x86 based architectures, even if usize happens to be 64 bits. Doing otherwise would encourage non-portable code.

We could also just drop the usize impls. There's already some precedence for preferring u64 impls over usize impls: #364.

So I think the following cfg gates might be the most reasonable in my view:

cfg(any(target_arch = "x86", target_arch = "x86_64")) whenever converting from pointers or usize to VirtAddr

cfg(target_arch = "x86_64) whenever performing infallible conversions from VirtAddr to pointers or usize

👍

However, if I modified the cfg gates on the existing inherent methods, that would technically be a breaking change, though I'm not sure if that would actually affect anybody.

Yes, breaking changes should only be merged into the next branch. This could be done in a follow up pr.

@Freax13 Freax13 added the waiting-on-author Waiting for the author to act on review feedback. label Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting for the author to act on review feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impl TryFrom, From, and Into for VirtAddr
4 participants