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

Use core::ops::Range and core::ops::RangeInclusive instead of custom range structs #517

Closed
wants to merge 46 commits into from

Conversation

ChocolateLoverRaj
Copy link

@ChocolateLoverRaj ChocolateLoverRaj commented Nov 28, 2024

(Breaking change)

I made this PR based on #513 since I'm using the latest nightly and I didn't want a ton of errors. Closes #515.

I deleted Page::range and similar functions because the .. and ..= syntax can be used instead.

Freax13 and others added 30 commits May 24, 2024 12:16
Ensure that Page actually implements Hash
Starting with the latest nightly, this results in an error:

error: avoid using labels containing only the digits `0` and `1` in inline assembly
  --> src/instructions/segmentation.rs:81:18
   |
81 |                 "1:",
   |                  ^ use a different label that doesn't start with `0` or `1`
   |
   = note: an LLVM bug makes these labels ambiguous with a binary literal number
   = note: `#[deny(binary_asm_labels)]` on by default
Add size and len for PageRange, PhysFrameRange, PageRangeInclusive and PhysFrameRangeInclusive
This feature will be stable as of Rust 1.80 and so enabling it explicitly
throws a warning.
Make `GDT::append` and `GDT::push` `const` by default. The `const_fn` feature is now a no-op.
…fs-feature

Remove stabilized `const_mut_refs` feature
rustc is phasing out allowing the "x86-interrupt" ABI on non-x86
targets. Using "x86-interrupt" on a non-x86 target currently causes a
warning, but this will become a hard error in a future version.

Previously, if `abi_x86_interrupt` was enabled (it's enabled by default
), we used it in a pointer type for the declaration of the `HandlerFunc`-
family of types and used a private empty tuple if `abi_x86_interrupt`
wasn't enabled. This patch changes the cfg gate to only use the
"x86-interrupt" abi on x86 targets. This is technically a breaking
change, but I'd like to argue that we shouldn't treat it as such:
The danger with treating this as a breaking change is that we can't
release this fix as a patch update and so once rustc eventually treats
this as an error, we might not yet have released the next breaking
version leaving our users with not published fix.
My hope is that there is no one using `HandlerFunc` on a non-x86 target.
Even today, declaring a function (not just a function pointer) with the
"x86-interrupt" abi on a non-x86 target causes an error, so it's
unlikely that this will affect real code. It's technically possible to
create a `HandlerFunc` on a non-x86 target by using transmute, but,
again my hope is that no one is actually doing that. I'd also like to
point out that the only use of a `HandlerFunc` on a non-x86 target
would be to call set_handler_fn and any such calls could simply be
replaced by calls to set_handler_addr.
Both Intel's and AMD's manuals describe the INVPCID descriptor as a
128-bit value with the linear address stored in the upper half and the
PCID stored in the lower half. x86 uses little-endian byte ordering and
so the lower half (pcid) should be stored before the upper half
(address). Previously, our `InvpcidDescriptor` type stored the halves
in the opposite order with the address before the pcid. This patch
fixes the order, so that the pcid is stored before the address. This
new order is also the order used by Linux and OpenBSD:
https://github.com/torvalds/linux/blob/3e5e6c9900c3d71895e8bdeacfb579462e98eba1/arch/x86/include/asm/invpcid.h#L8
https://github.com/openbsd/src/blob/4e368faebf86e9a349446b5839c333bc17bd3f9a/sys/arch/amd64/include/cpufunc.h#L173
It's beyond me how this wasn't noticed earlier. The previous incorrect
layout could lead to TLB entries not being flushed and #GP faults.
fix field order for INVPCID descriptor
It turns out that dtolnay/rust-toolchain sets the installed toolchain
as the global default, but rustup prioritizes our rust-toolchain.toml.
Instead, set an environment variable. RUSTUP_TOOLCHAIN has a higher
priority than rust-toolchain.toml [^1].

[^1]: https://rust-lang.github.io/rustup/overrides.html#overrides
gate HandlerFunc behind target_arch = "x86{_64}"
Rename the enum and add a deprecated type alias for the old name.
mrjbom and others added 15 commits November 16, 2024 12:46
…ctor

TryFrom implementation for ExceptionVector
Typo fix in TaskStateSegment comment
Minor clarification DescriptorTablePointer::limit comment
A recent nightly changed the signature of Step::steps_between to match
the signature of Iterator::size_hint. This patch changes our
implementations to match the new signature.
Without the as_slice call, the value passed to from_ptr was &[i32; 5]
which is *not* a slice.
Previously, we scaled count as a usize, not a u64. This is bad because
stepping forward by 0x10_0000 is perfectly fine, yet we were always
rejecting this because 0x10_0000 * 0x1000 doesn't fit in usize on
32-bit platforms. Similarly, we would fail to return a step count above
or equal to 0x10_0000 because the call to
<VirtAddr as Step>::steps_between would fail.
Never scale usize values, instead, always scale u64 values. To make
this easier to work with, this patch adds variants of the Step
functions to VirtAddr that take and return u64 instead of usize.
This patch also adds some regression tests.
@Freax13
Copy link
Member

Freax13 commented Nov 28, 2024

Thank you for your contribution!

I deleted Page::range and similar functions because the .. and ..= syntax can be used instead.

This won't work on a stable Rust channel. std::ops::Range<T> only implements Iterator when T implements Step, but Step can currently only be implemented on nightly.

(Breaking change)

Breaking changes should target the next branch.

@ChocolateLoverRaj ChocolateLoverRaj changed the base branch from master to next November 28, 2024 06:40
@Freax13
Copy link
Member

Freax13 commented Nov 30, 2024

I deleted Page::range and similar functions because the .. and ..= syntax can be used instead.

This won't work on a stable Rust channel. std::ops::Range<T> only implements Iterator when T implements Step, but Step can currently only be implemented on nightly.

Do you think there's a way to solve this?

@ChocolateLoverRaj
Copy link
Author

Breaking changes should target the next branch.

How should I rebase to the next branch? The next branch is very behind in commits.

@ChocolateLoverRaj
Copy link
Author

Do you think there's a way to solve this?

I think (you probably have a better idea than me) most people use this crate with nightly since it's much harder to make an OS without nightly. For the people who use this crate without nightly, I added a iter_pages function so that they can still get an iterator from a Range<Page>, it just won't be implicit. Since this contains other breaking changes anyways, I think it's ok if people have to change their

for page in page_range {
  // ...
}

to

for page in page_range.iter_pages() {
  // ...
}

@Freax13
Copy link
Member

Freax13 commented Nov 30, 2024

Do you think there's a way to solve this?

I think (you probably have a better idea than me) most people use this crate with nightly since it's much harder to make an OS without nightly.

That's a reasonable assumption, but support for stable is still important to us. There are users of this crate (including me) that don't use any nightly features.

For the people who use this crate without nightly, I added a iter_pages function so that they can still get an iterator from a Range<Page>, it just won't be implicit. Since this contains other breaking changes anyways, I think it's ok if people have to change their

for page in page_range {
  // ...
}

to

for page in page_range.iter_pages() {
  // ...
}

Hmm, I'm not sure if this is really an improvement, IMO it just changes the API. Nightly users can already use for page in page_range { ... } today if page_range is either a core::ops::Range or PageRange. Stable users can currently use for page in page_range { ... } if page_range is a PageRange, but this PR removes this in favor of for page in page_range.iter_pages() { ... } where page_range is a core::ops::Range. This new API also requires users to import an extension trait which IMO is not preferable to using inherent functions on a struct. Can you explain why you prefer the new API over the existing one?

Another implicit change in this PR is that some of our functions that take PageRange(Inclusive) now take core::ops::Range(Inclusive). AFAICT this only affects CleanUp::clean_up_addr_range and InvlpgbFlushBuilder::pages. I don't feel very strongly either way about keeping or changing this. I'd like to point out that this change can be done independently of removing PageRange.

Breaking changes should target the next branch.

How should I rebase to the next branch? The next branch is very behind in commits.

Usually, you could use something like git rebase origin/master --onto origin/next, but in this case, as you noted, next is quite old. I created #521 to sync next. You can try rebasing against the source branch of that PR.

@ChocolateLoverRaj
Copy link
Author

I get that this change doesn't provide much of a benefit. I'll leave this PR open and if it's wanted then it can be merged. It's okay if it's closed.

Once (if) Step is in stable Rust I will reopen this if it's closed.

@Freax13
Copy link
Member

Freax13 commented Dec 7, 2024

Let's close this for now and re-evaluate when Step is stabilized.

@Freax13 Freax13 closed this Dec 7, 2024
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.

Make PageRange a trait for core::ops::Range<Page>
5 participants