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 `slice` and `slice_mut` methods to IDT #95

Merged
merged 8 commits into from Nov 26, 2019

Conversation

@foxcob
Copy link
Contributor

foxcob commented Nov 18, 2019

The current IDT structure doesn't provide much in the way of dynamic management. I would like to be able to manage hardware interrupts externally. By making idt.interrupts public adding slice and slice_mut methods, I can return a slice of interrupt vectors to a hardware interrupt controller class so that that class can assign it's own handlers in the IDT. Without this, the external class needs direct access to the IDT.

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Nov 20, 2019

Thanks for the pull request! We used to have this field public, but it caused lots of confusion because it starts at index 32 of the IDT. So if you wanted to set a handler for the 34th entry in the IDT, you would need to access idt.interrupts[34 - 32]. For this reason, we made the field private in 7e80300. I'm not comfortable with reverting this decision because of the potential for confusion.

How about a different solution: The InterruptDescriptorTable currently implements Index<usize> and IndexMut<usize> to get access to individual entries. We could additionally implement the Index and IndexMut traits for RangeBound<usize>. This way, the InterruptDescriptorTable could be used like a normal slice. For example idt[32..] would return a slice of all interrupt entries and idt[32..34] would return a two-element slice of IDT entries 32 and 33. Would this work for you?

The implementation would be similar to the existing Index/IndexMut implementations, with the difference that a slice instead of a single entry is returned. Perhaps we should do something to reduce code duplication, e.g. use a common method or a macro if not possible.

@foxcob

This comment has been minimized.

Copy link
Contributor Author

foxcob commented Nov 20, 2019

Sounds like a good idea. I had considered something similar before. I'll update the request when I've got something unless you want to close this one and have me submit another.

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Nov 20, 2019

Great, thanks a lot! I'm fine with keeping this PR open if you like.

@foxcob

This comment has been minimized.

Copy link
Contributor Author

foxcob commented Nov 22, 2019

I changed the index trait to operate on RangeBounds, but there's a consequence that I wasn't sure how to avoid without putting everything into an array. The existing named exception handlers cannot be returned since the index return type changed from &Entry to &[Entry]. One solution is to put each handler in an array of size 1. Not sure it is preferrable. I'm not sure what the impact to others would be by removing access to the exception handlers via the Index trait.

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Nov 22, 2019

We could use the SliceIndex to support indexing through both ranges and usize types. See the implementation of IndexMut for the primitive slice type in the standard library for an example. This way, we could avoid breaking changes and return a plain Entry on a usize index.

let lower_idx = match index.start_bound() {
Included(start) => *start,
Excluded(start) => *start + 1,
Unbounded => 32,

This comment has been minimized.

Copy link
@phil-opp

phil-opp Nov 22, 2019

Member

I would prefer to error in this case because it would be confusing that ..256 is a smaller range than 0..256.

@foxcob

This comment has been minimized.

Copy link
Contributor Author

foxcob commented Nov 22, 2019

I initially looked at SliceIndex and didn't implement since I didn't seem to have direct access to the range values passed. The only possible option I see is to use some pointer math to compute the index desired. The other option is to implement a custom generic trait similar to SliceIndex which provides a templated return type and index type, but which allows access to the index range. Do you have any thoughts as to which is more desirable (or another idea entirely)?

OT: I'm using a bit of mix between your first and second edition of your "Writing an OS in Rust" blogs. I'm very much enjoying the challenge and am happy to be able to contribute back in any way. I'm a seasoned C/C++ developer, but still in the learning phase with Rust.

@foxcob

This comment has been minimized.

Copy link
Contributor Author

foxcob commented Nov 24, 2019

I've tried everything I can think of to make this work, but I cannot find a way to make it work with the current feature set in Rust. Particularly the lack of trait overloading. I think the best solution I can come up with is to leave the Index trait as it was (don't make breaking changes) and add a slice() method to retrieve a slice. Any index outside of 32..=255 will cause an error.

@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Nov 25, 2019

I initially looked at SliceIndex and didn't implement since I didn't seem to have direct access to the range values passed. The only possible option I see is to use some pointer math to compute the index desired.

Ah, I see. I can't think of a good solution either…

I think the best solution I can come up with is to leave the Index trait as it was (don't make breaking changes) and add a slice() method to retrieve a slice. Any index outside of 32..=255 will cause an error.

Sounds good to me! It's still straightforward to use without breaking any existing code or introducing a complicated custom trait.

OT: I'm using a bit of mix between your first and second edition of your "Writing an OS in Rust" blogs. I'm very much enjoying the challenge and am happy to be able to contribute back in any way. I'm a seasoned C/C++ developer, but still in the learning phase with Rust.

Great to hear that you like it! And thanks again for submitting this pull request.

Copy link
Member

phil-opp left a comment

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

let lower_idx = match bounds.start_bound() {
Included(start) => *start,
Excluded(start) => *start + 1,
Unbounded => 32,
};
let upper_idx = match bounds.end_bound() {
Included(end) => *end + 1,
Excluded(end) => *end,
Unbounded => 255,
};

if lower_idx > 255 || upper_idx > 255 {
panic!("Index out of range [{}..{}]", lower_idx, upper_idx);
}
if lower_idx < 32 {
panic!("Cannot return slice from traps, faults, and exception handlers");
}
Comment on lines 477 to 493

This comment has been minimized.

Copy link
@phil-opp

phil-opp Nov 25, 2019

Member

Maybe we can factor out the index checks and adjustments into a separate private method to avoid the duplication for slice and slice_mut.

let lower_idx = match bounds.start_bound() {
Included(start) => *start,
Excluded(start) => *start + 1,
Unbounded => 32,

This comment has been minimized.

Copy link
@phil-opp

phil-opp Nov 25, 2019

Member

Unbounded should be equivalent to 0 for an usize range so that 0..100 and ..100 have the same effect.

This comment has been minimized.

Copy link
@foxcob

foxcob Nov 25, 2019

Author Contributor

Doh, I had this right, then ended up copying the mistake back in during my multiple refactors.

Unbounded => 255,
};

if lower_idx > 255 || upper_idx > 255 {

This comment has been minimized.

Copy link
@phil-opp

phil-opp Nov 25, 2019

Member

Shouldn't this be 256 since the upper bound is excluded so that 32..256 is a valid range?

@phil-opp phil-opp changed the title Make structures::idt.interrupts public Add `slice` and `slice_mut` methods to IDT Nov 26, 2019
@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Nov 26, 2019

Thanks a lot! Just for your information: I updated the pull request title and description to reflect the latest version of the PR.

@phil-opp phil-opp merged commit 6348cb3 into rust-osdev:master Nov 26, 2019
9 checks passed
9 checks passed
Test (ubuntu-latest)
Details
Test (macos-latest)
Details
Test (windows-latest)
Details
Check Formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rust-osdev.x86_64 #20191125.1 succeeded
Details
rust-osdev.x86_64 (Job linux) Job linux succeeded
Details
rust-osdev.x86_64 (Job mac) Job mac succeeded
Details
rust-osdev.x86_64 (Job windows) Job windows succeeded
Details
phil-opp added a commit that referenced this pull request Nov 26, 2019
@phil-opp

This comment has been minimized.

Copy link
Member

phil-opp commented Nov 26, 2019

Published as version v0.7.7.

@foxcob foxcob deleted the foxcob:public_idt_interrupts branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.