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

idt: Fixup Options structure and cleanup set_handler_fn #226

Merged
merged 2 commits into from Jan 12, 2021
Merged

idt: Fixup Options structure and cleanup set_handler_fn #226

merged 2 commits into from Jan 12, 2021

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Jan 12, 2021

This change is an alternative to #225:

  • Moves the IDT entry's CS GDT selector to the options structure
    • It also now uses SegmentSelector
  • The set_handler_fn is now a proper generic function
    • This is done by adding the InterruptFn trait
  • Change behavior (and document) so set_handler_fn resets all the
    options (as opposed to some). This is a breaking change.

Signed-off-by: Joe Richey joerichey@google.com

This change:

- Moves the IDT entry's CS GDT selector to the options structure
  - It also now uses SegmentSelector
- The `set_handler_fn` is now a proper generic function
  - This is done by adding the `InterruptFn` trait
- Change behavior (and document) so `set_handler_fn` resets _all_ the
  options (as opposed to some).

Signed-off-by: Joe Richey <joerichey@google.com>
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.

Looks good to me overall, thanks! There is one small documentation issue about the default CS value though (see inline comments).

src/structures/idt.rs Outdated Show resolved Hide resolved
Comment on lines 671 to 672
/// Set the code segment that will be used by this interrupt. By default,
/// the current code segment is used.
Copy link
Member

Choose a reason for hiding this comment

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

By default, the current code segment is used.

This is not true, as minimal() uses 0 as default CS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I just removed this line as to avoid confusion.

Saying "By default" here is confusing, as it could mean the defaults from set_handler_fn or the defaults from minimal().

@phil-opp
Copy link
Member

This change should be non-breaking, right?

@mental32
Copy link
Member

mental32 commented Jan 12, 2021

This change should be non-breaking, right?

Pretty sure the change on set_handler_fn is breaking since there's no blanket trait impl for u64 which was the previous type of the argument.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Contributor Author

This change should be non-breaking, right?

Correct, as the only function that's changed is set_handler_fn which went from an explicit implementation on 5 types to an implementation on an interface that is implemented by those types, so all existing code should still work.

There is a functional change. Before, set_handler_fn would not change things like disable_interrupts, set_privilege_level, or set_stack_index. Now, it will reset those values to their defaults.

@josephlr
Copy link
Contributor Author

Pretty sure the change on set_handler_fn is breaking since there's no blanket trait impl for u64 which was the previous type of the argument.

I don't think the u64 stuff should be an issue. The u64 argument was only used w/ set_handler_addr, which was a private function. The only publicly exported functions were set_handler_fn which all only used function pointers.

@phil-opp phil-opp changed the base branch from master to next January 12, 2021 12:57
@phil-opp
Copy link
Member

There is a functional change. Before, set_handler_fn would not change things like disable_interrupts, set_privilege_level, or set_stack_index. Now, it will reset those values to their defaults.

Ah, good point! However, I think the only way to change the entry options is to call set_handler_fn, so it should only make a difference if set_handler_fn is called twice on the same entry, e.g.:

let mut entry = idt[42];
entry.set_handler_fn(foo).disable_interrupts(false);
entry.set_handler_fn(bar); // this now disables interrupts again

So I don't expect much breakage from this in practice. However, I would still classify this as a breaking change to be on the safe side. I therefore changed the base branch of this PR to the next branch.

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 doc updates! Looks ready for me now.

@josephlr josephlr merged commit 74d67e7 into next Jan 12, 2021
@josephlr josephlr deleted the idt branch January 12, 2021 13:01
@mental32
Copy link
Member

Pretty sure the change on set_handler_fn is breaking since there's no blanket trait impl for u64 which was the previous type of the argument.

I don't think the u64 stuff should be an issue. The u64 argument was only used w/ set_handler_addr, which was a private function. The only publicly exported functions were set_handler_fn which all only used function pointers.

Right! I'm muddling those two up, my bad 😅

josephlr added a commit that referenced this pull request Jun 4, 2021
This moves the code_selector to the `EntryOptions` structure.

While rebasing this, I also updated the `Debug` implementation to
actually print out useful information instead of just hex codes.

I left the type field as binary, as that's what the SDM does.

Signed-off-by: Joe Richey <joerichey@google.com>
This was referenced Jun 4, 2021
josephlr added a commit that referenced this pull request Jun 12, 2021
This moves the code_selector to the `EntryOptions` structure.

While rebasing this, I also updated the `Debug` implementation to
actually print out useful information instead of just hex codes.

I left the type field as binary, as that's what the SDM does.

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit to josephlr/x86_64 that referenced this pull request Jul 18, 2021
This moves the code_selector to the `EntryOptions` structure.

While rebasing this, I also updated the `Debug` implementation to
actually print out useful information instead of just hex codes.

I left the type field as binary, as that's what the SDM does.

Signed-off-by: Joe Richey <joerichey@google.com>
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.

None yet

3 participants