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 flags for CR0, CR4 and XCR0, as well as extra checks for modification of XCR0 #273

Merged
merged 3 commits into from Jul 17, 2021

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Jul 7, 2021

Replacement for #272. @ethindp can you take a look?

My changes

  • Remove the breaking changes
  • Updated the documentation
  • Fix the asserts when setting XCR0

Original PR description

This PR adds some extras to the three control registers CR0, CR4, and XCR0. It also adds some checks for XCR0. (We could extend this to other registers as well.)

Bits added

  • CR0:
    • Extension Type (ET, bit 4 of CR0)
  • CR4:
    • Key-Locker-Enable Bit (KL, bit 19 of CR4)
    • Control-Flow Enforcement Technology (CET) enable (CET, bit 23 of CR4)
    • Enable protection keys for supervisor-mode pages (PKS, bit 24 of CR4)
  • XCR0:
    • Enable XSAVE to manage the bound registers (BNDREG, bit 3 of XCR0)
    • Enable XSAVE to manage the BCFGU and BNDSTATUS registers (BNDCSR, bit 4 of XCR0)
    • Enable AVX-512 and allow XSAVE to manage the mask registers K0-K7 (OPMASK, bit 5 of XCR0)
    • Enable AVX-512 and allow XSAVE to manage the upper halves of the lower ZMM registers (ZMM_HI256, bit 6 of XCR0)
    • Enable AVX-512 and allow XSAVE to manage the upper ZMM registers (HI16_ZMM, bit 7 of XCR0)

Checks

This PR adds invariant checks that cannot be disabled via the assert! macro when using the write() function on the XCr0 type. Primarily, these invariants enforce those listed on pg. 2-20 of volume 3A of the intel SDMs; they disallow attempts to:

  • clear XCR0.x87;
  • clear XCR0.SSE and set XCR0.AVX;
  • clear XCR0.AVX and set any of XCR0.opmask, XCR0.ZMM_Hi256, and XCR0.Hi16_ZMM;
  • set either XCR0.BNDREG and XCR0.BNDCSR while not setting the other; and
  • set any of XCR0.opmask, XCR0.ZMM_Hi256, and XCR0.Hi16_ZMM while not setting all of them.

I missed the initial invariant (that setting any reserved bits causes a general-protection fault) but I will add that if you guys would like me to (somebody else can do so as well). We should probably consider enforcing all invariants on all control registers to catch problems like this before they arise and generate exceptions, since these APIs are supposed to be "safe". This PR unfortunately has a few breaking changes as well.

@josephlr
Copy link
Contributor Author

josephlr commented Jul 7, 2021

Note, I intentionally didn't add the following AMD only flags to XCr0Flags

        /// Enables using the CET user state
        /// with `XSAVE`/`XRSTOR` (AMD Only).
        const CET_U =  1 << 11;
        /// Enables using the CET supervisor state
        /// with `XSAVE`/`XRSTOR` (AMD Only).
        const CET_S =  1 << 12;

This is because Intel (as well as AMD weirdly enough) also have these same flags in the XSS register, so we should encourage users to just use that (as their code will then be supported by both vendors).

@ethindp
Copy link
Contributor

ethindp commented Jul 7, 2021

Why did you want to eliminate the breaking changes? I only ask because I felt that it was a much better solution if our constants aligned with the intel documentation instead of coming up with our own. People would get less confused IMO. But if it truly must not exist, I'm alright with that.

Alos, add extra checks when writing to XCR0.

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

josephlr commented Jul 16, 2021

Why did you want to eliminate the breaking changes? I only ask because I felt that it was a much better solution if our constants aligned with the intel documentation instead of coming up with our own. People would get less confused IMO.

Sorry @ethindp I didn't make myself clear. I absolutely agree with you that some of the existing flag names are bad and should be changed. I just wanted to get most of your changes in as part of 0.14.4 (because it would be very helpful to people). As part of our breaking changes for 0.15 (see #262), I would want to change the flag names to be correct. Specifically:

  • Cr4Flags::PROTECTION_KEY => Cr4Flags::PROTECTION_KEY_USER
  • XCr0Flags::YMM => XCr0Flags::AVX

Are there any other flag names that you think should be changed?

Signed-off-by: Joe Richey <joerichey@google.com>
This avoids getting a #GP when writing.

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

@phil-opp @ethindp PTAL

BTW #275 is a draft PR for the breaking changes

@josephlr josephlr added the waiting-for-review Waiting for a review from the maintainers. label Jul 16, 2021
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.

I didn't check the new flags against the manuals, but the changes look good to me overall.

Comment on lines +4 to +5
#[cfg(docsrs)]
use crate::{registers::rflags::RFlags, structures::paging::PageTableFlags};
Copy link
Member

Choose a reason for hiding this comment

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

What is this about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for the doc comments to properly reference these other structures (like PageTableFlags::GLOBAL). Without them, the docs don't properly link to the other sections.

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok. But there imports are then also needed for local doc builds, e.g. on cargo doc --open. So we should probably use cfg(doc) instead. I opened #287 for this.

@josephlr josephlr merged commit 759cb32 into master Jul 17, 2021
@josephlr josephlr deleted the flags branch July 17, 2021 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-review Waiting for a review from the maintainers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants