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 InterruptDescriptorTable::load_unsafe #137

Merged
merged 3 commits into from
Apr 18, 2020

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Mar 8, 2020

Without this kind of method, there is no way right now to have a dynamically-allocated IDT, and thus no real way to have a separate IDT per CPU.

@phil-opp
Copy link
Member

phil-opp commented Mar 8, 2020

Thanks for the pull request! Could you clarify why the current API is insufficient? Creating a &'static reference from a Box should be possible through the Box::leak method.

@phil-opp
Copy link
Member

@tomaka Friendly ping :).

@josephlr
Copy link
Contributor

So there are three use cases that this might be targeting:

  1. The IDTs are dynamically created on the heap, but after creation are valid for the remaining lifetime of the program.
  2. The IDTs live in static memory, but need to be initialized w/ runtime parameters before being loaded.
    • You can use a OnceCell to handle this:
      • For example:
        static IDTS: [OnceCell<InterruptDescriptorTable>; N]> = [OnceCell::new(); N];
        fn init_cpu(n: usize) {
            ...
            // Make the IDT and put it in static memory
            let idt = InterruptDescriptorTable { ... };
            IDTS[n].set(idt);
            ...
        }
        fn start_cpu(n: usize) {
            ...
            // Load the IDT
            IDTS[n].get().unwrap().load();
            ...
        }
      • For no_std multithreading support in once_cell, see my comment on the PR to add no_std support.
    • Alternatively, this could just be done w/ static mut. You borrow mutably to initialize, and then borrow immutably when passing it to load. This requires unsafe, but is fairly simple.
  3. The IDT doesn't actually live for the entire duration of the program and is replaced by a subsequent call to InterruptDescriptorTable::load and then is later destroyed.
    • I don't think this is what @tomaka was originally talking about
    • This requires complex, custom lifetime management by the user of x86_64, so wouldn't be a good fit for this crate.
    • Provided such management code was correct, they could use unsafe code with the existing API:
      // We are sure this IDT will live long enough 
      let idt: &InterruptDescriptorTable;
      unsafe { *(idt as *const _) }.load()

@tomaka have I missed anything?

As all of these use cases can be solved though other mechanisms, I think this can be closed.

@tomaka
Copy link
Contributor Author

tomaka commented Apr 18, 2020

I don't think this is what @tomaka was originally talking about

This is indeed what I was talking about.

While any use case can be achieved, I also fail to understand why adding this unsafe method is undesirable?

@phil-opp
Copy link
Member

@josephlr Thanks a lot for the examples! I agree that there are better options for the first two use cases: For use case 1 you can use the safe Box::leak method and for use case 2 you can get the required &'static reference directly from the static.

However, I think that an unsafe_load method would make use case 3 much more expressive, so I'm fine with adding this method to the API.

@tomaka

While any use case can be achieved, I also fail to understand why adding this unsafe method is undesirable?

I have no problem with unsafe methods in general, as long as there is a reasonable use case for them. I didn't think of temporary IDTs, so I didn't understand the advantage of such a method in comparison to the safe approaches that already exist. Now that I know where this method would be useful, I'm fine with merging this PR. Thanks again for opening it and sorry for the delay in merging!

@phil-opp phil-opp merged commit d73f3e0 into rust-osdev:master Apr 18, 2020
phil-opp added a commit that referenced this pull request Apr 18, 2020
@phil-opp
Copy link
Member

Published as version 0.10.1

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