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

feat(idt): add unsafe const set_handler_{fn|addr}_unchecked methods #225

Closed
wants to merge 3 commits into from
Closed

feat(idt): add unsafe const set_handler_{fn|addr}_unchecked methods #225

wants to merge 3 commits into from

Conversation

mental32
Copy link
Member

@mental32 mental32 commented Jan 2, 2021

Allows users of structures::idt::InterruptDescriptorTable to set their handler functions in const, if you wanted to do something like this before you would've had to resort to constructing the table yourself and transmuting it into the crate provided IDT struct (which is error prone.)

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! I like the idea of adding a way to construct an IDT at compile time.

I left a few comments inline, but overall this looks good to me.

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
@josephlr
Copy link
Contributor

josephlr commented Jan 4, 2021

While I really like this idea in principle, I don't think it actually works. For example, I would imagine this function would be used like:

extern "x86-interrupt" fn handle_debug(_: &mut InterruptStackFrame) {}
extern "x86-interrupt" fn handle_overflow(_: &mut InterruptStackFrame) {} 

const CS: SegmentSelector = SegmentSelector::new(1, PrivilegeLevel::Ring0);
const DEBUG: HandlerFunc = handle_debug;
const OVERFLOW: HandlerFunc = handle_overflow;

const fn make_idt() -> InterruptDescriptorTable {
    let mut idt = InterruptDescriptorTable::new();
    unsafe { idt.debug.set_handler_fn_with_segment(DEBUG, CS) };
    unsafe { idt.overflow.set_handler_fn_with_segment(OVERFLOW, CS) };
    idt
}

static IDT: InterruptDescriptorTable = make_idt();

However, this results in a "pointer-to-integer cast" needs an rfc before being allowed inside constants at the self.set_handler_addr_with_segment(handler as u64, cs_segment) line.

Fundamentally, the const_raw_ptr_to_usize_cast feature doesn't really work, and doesn't have a path to stabilization.

@mental32, am I missing something here? Is there another way you were imagining set_handler_fn_with_segment to be used?

@mental32
Copy link
Member Author

mental32 commented Jan 4, 2021

@josephlr you're not missing anything, that's exactly what I was hoping to write.

I'm not actually sure how this could be done in const without ptr-to-integer casts which is a shame 😦

Which seems possible given this code in MIRI exists but if I'm understanding the situation correctly its more an issue of "this language feature just isn't getting attention/stabilized"

@mental32
Copy link
Member Author

mental32 commented Jan 4, 2021

@phil-opp since the const-part of this PR doesn't look like it's happening I can remove the related code.

There is however, potential salvage, the addition of the *_with_segment methods for the IDT (which allows specifying custom code segments for entries) but I'm not sure how or if this is a useful thing to have.

Should I close this PR or attempt to salvage it?

@josephlr
Copy link
Contributor

josephlr commented Jan 5, 2021

There is however, potential salvage, the addition of the *_with_segment methods for the IDT (which allows specifying custom code segments for entries) but I'm not sure how or if this is a useful thing to have.

I was talking to some OS folks at work and they thought of case where this might be useful: direct-to-user interrupts. In that case, the interrupt CS has to point to the user CS instead of the standard system CS. It's fairly specialized, but I think it would be nice to have.

However, if we do that, I think setting the system selector should just be part of the EntryOptions structure. Basically, the following structs would be redefined:

#[repr(C)]
pub struct EntryOptions {
    selector: SegmentSelector,
    bits: u16,
};

#[repr(C)]
pub struct Entry<F> {
    pointer_low: u16,
    options: EntryOptions,
    pointer_middle: u16,
    pointer_high: u32,
    reserved: u32,
    phantom: PhantomData<F>,
}

and a set_code_segment(SegmentSelector) method added to EntryOptions.

That would allow users to do:

/// Setup debug for system
idt.debug.set_handler_fn(system_debug);
/// Setup debug for usermode 
idt.debug
    .set_handler_fn(usermode_debug)
    .set_code_segment(USER_CS)
    .set_set_privilege_level(PrivilegeLevel::Ring3);

@josephlr
Copy link
Contributor

josephlr commented Jan 5, 2021

Which seems possible given this code in MIRI exists but if I'm understanding the situation correctly its more an issue of "this language feature just isn't getting attention/stabilized"

Unfortunately, the situation is about 100 times worse than that. A compile-time IDT is essentially impossible on x86.

At compile time (in C, C++, Rust, etc...) a pointer (including function pointers) can either be:

  • A "fake" pointer, i.e. a raw numerical value: 0xdeadbeef as *const u8
  • A "real" pointer to some data or code: my_func as *const u8

The "fake" pointers can be cast to integers at compile-time (as they started as integers), this is the use-case of the "ptr-to-integer cast" feature. However, real pointers don't even have a final value at compile-time, as the final virtual address of the code/data is not yet known to rust. All rustc can emit is essentially a (symbol, offset) pair, at link-time these symbols are then resolved into their final values.

This means you can add/subtract integers from real pointers (this is essentially what array accesses do) at compile-time, but no other arithmetic is possible.

Now if we look at the structure of an IDT entry:

pub struct Entry {
    pointer_low: u16,
    options: EntryOptions,
    pointer_middle: u16,
    pointer_high: u32,
    reserved: u32,
}

we see that to initialize the entry we must do:

let addr = func as usize;
self.pointer_high = (addr >> 32) as u32

Grabbing the high 32 bits of a real pointer is not something that can be done at compile-time in any language. Note that if the IDT entry instead looked like:

pub struct Entry {
    pointer: u64,
    options: EntryOptions,
    reserved: u32,
}

we wouldn't have any of the above problems, as we wouldn't have to do any bit math to fill out pointer.

This is why all IDTs are dynamically initialized. For example, Linux initializes its IDT from a separate static table. The separate table has entries that do not have the above problem (compare the layout of idt_data to gate_struct).

EDIT: Coreboot's IDT bringup code also explains this sad state of affairs, also see the exception_init function.

@josephlr
Copy link
Contributor

#226 implements a subset of this PR as part of additional IDT cleanup, PTAL

@mental32
Copy link
Member Author

Sorry I AWOL'd on this a while ago.

@josephlr I just checked out that PR and the subset you mention seems to be the ability to set the CS selector onto the EntryOptions struct itself via EntryOptions::set_cs_selector ?

That seems to be what was left to address in this PR so should I close here in favor of #226?

@josephlr
Copy link
Contributor

That seems to be what was left to address in this PR so should I close here in favor of #226?

Sounds reasonable, closing in favor of #226. I do think we should look into other solutions for fully static IDTs. One idea might be to use an jump table, but it might not be worth the effort (however jump tables could be used as an alternative to #168).

@josephlr josephlr closed this Jan 12, 2021
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