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

Start populating the IDT and handle double faults. #2893

Merged
merged 1 commit into from
May 25, 2022

Conversation

andrisaar
Copy link
Collaborator

No description provided.

@andrisaar andrisaar marked this pull request as ready for review May 24, 2022 20:40
@andrisaar andrisaar requested a review from a team as a code owner May 24, 2022 20:40
@andrisaar andrisaar requested review from rbehjati and conradgrobler and removed request for a team May 24, 2022 20:40
};
}

extern "x86-interrupt" fn double_fault_handler(
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "x86-interrupt"? Is this a type of linker that the Rust compiler already knows about or is it defined somewhere? Could you add a clarifying comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's not about linker -- it changes the calling convention of the function and what code the compiler emits. The particular calling convention is required by the x86-64 ABI.

You can read more about the x86-interrupt in particular here: rust-lang/rust#40180

In short, you don't really expect to see that used anywhere else except for interrupt handlers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comment with a link to that issue would help us understand what is going on later when we look at the code.

// We're not calling `panic!` here because (a) the panic location would be all wrong (the error
// is not in the fault handler itself as the location would suggest), (b) in theory the
// panic handler itself could cause a double fault, and (c) we want to be sure that we shut
// down the machine after us.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is "shut down the machine after us" an idiom that I don't know of? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it just means we want to be sure that the machine is shut down after we have a double fault.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I think describing it in those words would be clearer :)

@andrisaar andrisaar merged commit a206ac3 into project-oak:main May 25, 2022
@andrisaar andrisaar deleted the faults branch May 25, 2022 14:47
@github-actions
Copy link

Reproducibility Index:

8b64a4e360af2bac9c2014a5f0d0d6237b3ba588022c027b6ee43cd980ed5d2a  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
f2352d3a83f3e8ba1dd4a8d9b091c35a144b7c49048dbc88e1c356471eba3e9c  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe

Reproducibility Index diff:

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.

3 participants