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

Add a SelectorErrorCode to segment_not_present in IDT #274

Merged
merged 16 commits into from
Jul 17, 2021
Merged

Add a SelectorErrorCode to segment_not_present in IDT #274

merged 16 commits into from
Jul 17, 2021

Conversation

budde25
Copy link
Contributor

@budde25 budde25 commented Jul 14, 2021

As documented here: https://wiki.osdev.org/Exceptions#Selector_Error_Code
Similar to the PageFaultHandlerFunc this adds a SegmentNotPresentHandlerFunc

The struct does need to be a repr(transparent) u64 otherwise a LLVM error occurs

@josephlr
Copy link
Contributor

@budde25 thanks for contributing to this crate!! The definitions for this type look good, but changing the type of segment_not_present is a breaking change, so we would want to handle that in a separate PR (or discuss it in #262).

Could you change this PR to just declare the SelectorErrorCode type/methods? This would mean removing the segment_not_present change and SegmentNotPresentHandlerFunc type definitions (at least for now).

src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Show resolved Hide resolved
src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Show resolved Hide resolved
@budde25
Copy link
Contributor Author

budde25 commented Jul 14, 2021

@josephlr
Thanks for the code review! I will make the those changes. As for the breaking change would it be enough to just remove the change to segment_not_present? What would the change to SegmentNotPresentHandlerFunc type definitions be? or would it just be removed since it would be unused now. Thanks!

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.

As for the breaking change would it be enough to just remove the change to segment_not_present? What would the change to SegmentNotPresentHandlerFunc type definitions be? or would it just be removed since it would be unused now.

You're exactly correct here, I would want us to remove SegmentNotPresentHandlerFunc so that users don't get confused and try to use it with this crate when it won't (currently) work.

src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Show resolved Hide resolved
src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Outdated Show resolved Hide resolved
src/structures/idt.rs Outdated Show resolved Hide resolved
Comment on lines 637 to 648
/// A segment not present handler function that pushes a selector error code.
///
/// This type alias is only usable with the `abi_x86_interrupt` feature enabled.
#[cfg(feature = "abi_x86_interrupt")]
pub type SegmentNotPresentHandlerFunc =
extern "x86-interrupt" fn(InterruptStackFrame, error_code: SelectorErrorCode);

/// This type is not usable without the `abi_x86_interrupt` feature.
#[cfg(not(feature = "abi_x86_interrupt"))]
#[derive(Copy, Clone, Debug)]
pub struct SegmentNotPresentHandlerFunc(());

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove (for now). Also when we add this back, note that the #NP exception isn't the only exception that uses this error code.

#TS, #NP: SelectorErrorCode
#SS, #GP: SelectorErrorCode or zero (depending on context)
#AC, #DF: Error code always 0

#PF, #CP, #VC, #SX: Custom error code

@josephlr josephlr added the waiting-on-author Waiting for the author to act on review feedback. label Jul 16, 2021
budde25 and others added 10 commits July 16, 2021 14:25
Co-authored-by: Joseph Richey <joerichey94@gmail.com>
Co-authored-by: Joseph Richey <joerichey94@gmail.com>
Co-authored-by: Joseph Richey <joerichey94@gmail.com>
Co-authored-by: Joseph Richey <joerichey94@gmail.com>
Co-authored-by: Joseph Richey <joerichey94@gmail.com>
@budde25
Copy link
Contributor Author

budde25 commented Jul 16, 2021

Alright, I have implemented the requested changes. Sorry for the delay.
I also added a new_truncate to the SelectorErrorCode as convenience method. Similar to the from_bits_truncate in an effort to provide a similar interface.

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.

LGTM, thanks for making the chanes!

I also added a new_truncate to the SelectorErrorCode as convenience method. Similar to the from_bits_truncate in an effort to provide a similar interface.

Nice addition, I like it. It's also similar to how VirtAddr works, so that's also great.

@josephlr josephlr merged commit 1a0b149 into rust-osdev:master Jul 17, 2021
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.

2 participants