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

Allow GDT to be loaded with shared reference #381

Merged
merged 9 commits into from
Apr 16, 2022
Merged

Allow GDT to be loaded with shared reference #381

merged 9 commits into from
Apr 16, 2022

Conversation

josephlr
Copy link
Contributor

Depends on #380

In #322 it was pointed out that calling load_tss would modify the GDT entry for the TSS (to mark it as busy). This meant our GDT required some sort of interior mutability if we wanted to use it in a static context. #323 solved this problem by requiring &mut to call load and load_unsafe, thus ensuring that the GDT would be mutable after it was loaded. It also introduced SingleUseCell to make such manipulations more ergonomic.

This PR takes a different approach. The idea is basically the initial version of #323 using AtomicU64. The main difference is that:

  • We can use the Entry type from Add structures::gdt::Entry type #380 to make the code cleaner.
  • We only need to use AtomicU64 if it's possible for code to call load and load_tss, so we can gate this on #[cfg(feature = "instructions")].

By doing things this way, we can go back to using shared references for load and load_unsafe, which is much more ergonomic. We can then get rid of SingleUseCell (which doesn't really fit well with this x86_64-specific crate). Also, using an atomic, shared, static type better reflects how the processor uses the GDT. It's almost always read-only, can be shared between cores, and once in use is only updated atomically. In fact, the Intel documentation for LTR even says:

TSSsegmentDescriptor(busy) := 1;
(* Locked read-modify-write operation on the entire descriptor when setting busy flag *)

In the future, we could also use the interior mutability of our GDT structure to add a method like:

/// Like `GlobalDescriptorTable::append()` except that it uses a shared reference.
#[inline]
pub fn append_atomic(&self, entry: Descriptor) -> SegmentSelector {
   // Atomically update the length and write the entry/entries 
}

which would allow for a GDT to be a normal static item, and eliminate some uses of lazy_static!.

Finally, I also added some tests to make sure that load_tss changing the TSS entry is actually observable.

The current documentation for the GDT often confuses entries and `Descriptor`s.
For example, adding a new `Descriptor` uses a method confusingly named
`add_entry`.

An entry is a raw 64-bit value that is indexed by a segment selector.
The `MAX` length of the GDT is a number of _entries_, not `Descriptor`s.

To fix this confusion, this PR makes the following changes:
  - Adds a transparent `u64` newtype called `Entry`.
  - Updates the `GlobalDescriptorTable` documentation to correctly use
    `Entry` or `Descriptor` where appropriate.
  - Renames the `add_entry` to `append`.
    - This better expresses that this method might add multiple entries.
  - Renames `from_raw_slice` to `from_raw_entries`
  - Renames `as_raw_slice` to `entries`
    - This method now returns a slice of `Entry`s instead of `u64`s

This also fixes an issue where our `assert!`s in `empty()` wouldn't
trigger if the GDT was constructed from raw values.

Signed-off-by: Joe Richey <[email protected]>
Copy link
Member

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me!

* We only need to use `AtomicU64` if it's possible for code to call `load` and `load_tss`, so we can gate this on `#[cfg(feature = "instructions")]`.

That's very clever!

src/structures/gdt.rs Outdated Show resolved Hide resolved
Also update the documentation

Signed-off-by: Joe Richey <[email protected]>
Signed-off-by: Joe Richey <[email protected]>
This gives our GDT the appropriate atomic interior mutablity if we
could potentially call `lgdt` and `ltr`.

Signed-off-by: Joe Richey <[email protected]>
Now that the `Entry` type has the appropriate interior mutability, we
can safely load a GDT using `&'static self`.

Signed-off-by: Joe Richey <[email protected]>
It's no longer used, so we don't need it anymore.

Signed-off-by: Joe Richey <[email protected]>
Copy link
Member

@Freax13 Freax13 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, thanks!

@josephlr josephlr mentioned this pull request Apr 15, 2022
13 tasks
@phil-opp
Copy link
Member

Also, using an atomic, shared, static type better reflects how the processor uses the GDT. It's almost always read-only, can be shared between cores

I don't see any problems sharing the code/data segments, but sharing the same TSS can lead to problems, right? How would we avoid that? Add multiple TSS entries to the same TSS?

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.

The code changes look good to me, I just think that we need some additional docs about sharing a GDT between cores (i.e. what is allowed and what not).

@Freax13
Copy link
Member

Freax13 commented Apr 16, 2022

Also, using an atomic, shared, static type better reflects how the processor uses the GDT. It's almost always read-only, can be shared between cores

I don't see any problems sharing the code/data segments, but sharing the same TSS can lead to problems, right? How would we avoid that? Add multiple TSS entries to the same TSS?

Sharing a TSS is still not possible because the CPU marks the TSS descriptor as busy. Attempting to load a busy TSS descriptor will cause a general protection fault. If we really wanted to share a GDT between cores, we'd have to use multiple TSS descriptors.

@josephlr
Copy link
Contributor Author

The code changes look good to me, I just think that we need some additional docs about sharing a GDT between cores (i.e. what is allowed and what not).

So this roughly what I added in #366. Those additional docs make it clear that load_tss can only be called exactly once for each SegmentSelector. It also documents that users shouldn't use tss_segment to create multiple descriptors pointing to the same TSS.

I think @Freax13's answer mostly explains why it wouldn't be possible (without nasty unsafe code) for users to share TSS Descriptors between cores. Of course there's nothing stopping them from sharing the same TSS (though multiple descriptors) between cores. While this is silly, as the TSS usually contains stack pointers which "should" be per-core. The processor lets you do it and it's not inherently unsound. So I think our documentation is enough there.

@phil-opp
Copy link
Member

Thanks a lot for clarifying! I missed #366, looks very good!

@josephlr josephlr merged commit 156cfda into next Apr 16, 2022
@josephlr josephlr deleted the gdt_atomic branch April 16, 2022 20:42
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