Added MTRRs and other missing registers #311

Qubasa · 2021-09-26T22:22:22Z

Added new registers:

mtrrcap
mtrrdeftype
mtrrfix64k00000
mtrrfix16k80000
mtrrfix16ka0000
mtrrfix4kc0000
mtrrfix4kc8000
mtrrfix4kd0000
mtrrfix4kd8000
mtrrfix4ke0000
mtrrfix4ke8000
mtrrfix4kf8000
mtrrphysmask0
mtrrphysmask1
mtrrphysmask2
mtrrphysmask3
mtrrphysmask4
mtrrphysmask5
mtrrphysmask6
mtrrphysmask7
mtrrphysbase0
mtrrphysbase1
mtrrphysbase2
mtrrphysbase3
mtrrphysbase4
mtrrphysbase5
mtrrphysbase6
mtrrphysbase7
cstar
syscfg
ldtr
cr8

Whats missing?
Implementing the #[cfg(not(feature = "inline_asm"))] handles.
A write() function for mtrrfix4ke0000 and the like

phil-opp

Thanks a lot for the PR! I did not have time to review the MTRR code (it's a lot), but I left some comments on the other changes.

I don't know when I'll have time for a more detailed review, but I would be fine with merging this behind a feature gate. In #266 (review) I proposed a cargo feature named experimental for this, to indicate new code that hasn't been tested much yet. What do you think about this approach?

src/instructions/tables.rs

src/registers/control.rs

Qubasa · 2021-11-06T14:21:13Z

Thanks a lot for the PR! I did not have time to review the MTRR code (it's a lot), but I left some comments on the other changes.

I don't know when I'll have time for a more detailed review, but I would be fine with merging this behind a feature gate. In #266 (review) I proposed a cargo feature named experimental for this, to indicate new code that hasn't been tested much yet. What do you think about this approach?

Yeah sounds like a good idea! Right now I'm a bit busy so I will implement the fixups you mentioned in one or two weeks

Freax13 · 2021-12-01T13:35:15Z

src/registers/control.rs

+bitflags! {
+    /// Configuration flags of the [`Cr8`] register.
+    pub struct Cr8Flags: u64 {
+        /// A value corresponding to the highest-priority interrupt that is to be blocked
+        const TASK_PRIORITY = 0xf;
+    }
+}
+


I'm not sure bitflags is the right abstraction for this. The register doesn't contain any flags, it contains the task priority as a 4 bit number.

Freax13 · 2021-12-01T13:37:24Z

src/registers/control.rs

+/// interrupting a high-priority task. This is accomplished by loading TPR with a value corresponding to
+/// the highest-priority interrupt that is to be blocked.
+#[derive(Debug)]
+pub struct Cr8;


The other registers appear in order of their number. We should keep it that way.

Freax13 · 2021-12-01T13:37:38Z

src/registers/control.rs

@@ -162,6 +176,57 @@ mod x86_64 {
    use super::*;
    use crate::{instructions::tlb::Pcid, structures::paging::PhysFrame, PhysAddr, VirtAddr};

+    impl Cr8 {


Same thing about the order here.

Freax13 · 2021-12-01T13:43:43Z

src/registers/model_specific.rs

+        /// MtrrFixDramEn
+        const MFDE = 1 << 18;
+        /// MtrrFixDramModEn
+        const MFDM = 1 << 19;
+        /// MtrrVarDramEn
+        const MVDM = 1 << 20;
+        /// MtrrTom2En
+        const TOM2 = 1 << 21;
+        /// Tom2ForceMemTypeWB
+        const FWB = 1 << 22;
+        /// MemEncryptionModeEn
+        const MEME = 1 << 23;
+        /// SecureNestPagingEn
+        const SNPE = 1 << 24;
+        /// VMPLEn
+        const VMPLE = 1 << 25;


The documentation for these could be improved.

Freax13 · 2021-12-01T13:45:48Z

src/registers/mtrr.rs

+#[derive(Debug)]
+pub struct MTRRcap;
+
+#[allow(dead_code)]


This is not needed.

Freax13 · 2021-12-01T14:24:57Z

src/registers/mtrr.rs

+    /// Flags for the MTRRphysBase[n] registers
+    pub struct MTRRphysBaseFlags: u64 {
+        /// The memory range base-address in physical-address space
+        const PHYS_BASE = 0xffff_ffff_ff << 12;


Same thing here with PHYS_BASE as with PHYS_MASK.

Freax13 · 2021-12-01T14:29:22Z

src/registers/mtrr.rs

+    impl MTRRphysBase0 {
+        /// Read the current raw MTRRphysBase flags.
+        #[inline]
+        pub fn read_raw() -> u64 {
+            unsafe { Self::MSR.read() }
+        }
+
+        /// Write the MTRRphysBase flags.
+        ///
+        /// Does not preserve any bits, including reserved fields.
+        ///
+        /// ## Safety
+        ///
+        /// Unsafe because it's possible to
+        /// break memory safety with wrong flags
+        #[inline]
+        pub unsafe fn write_raw(flags: u64) {
+            let mut msr = Self::MSR;
+            msr.write(flags);
+        }
+
+        /// Write the MTRRphysBase flags, preserving reserved values.
+        ///
+        /// Preserves the value of reserved fields.
+        ///
+        /// ## Safety
+        ///
+        /// Unsafe because it's possible to break memory
+        /// safety with wrong flags
+        #[inline]
+        pub unsafe fn write(flags: MTRRphysBaseFlags) {
+            let old_value = Self::read_raw();
+            let reserved = old_value & !(MTRRphysBaseFlags::all().bits());
+            let new_value = reserved | flags.bits();
+
+            Self::write_raw(new_value);
+        }
+
+        /// Read the current MTRRphysBase flags.
+        #[inline]
+        pub fn read() -> MTRRphysBaseFlags {
+            MTRRphysBaseFlags::from_bits_truncate(Self::read_raw())
+        }
+    }


It might be worth considering putting this in a macro instead of repeating it 8 times. Also applies to MTRRphysMaskn and MTRRfix64K00000.

Alternatively you could consider making those registers enumerations. This would also have the advantage that it allows choosing the register at runtime.

Freax13 · 2021-12-01T14:32:00Z

src/registers/mtrr.rs

+            let two = FixMemRange::new(
+                0x10000,
+                0x1FFFF,
+                ((r & (0xff << 8)) >> 8).try_into().unwrap(),


You can use r.get_bits(8..16) instead of ((r & (0xff << 8)) >> 8).

Freax13 · 2021-12-01T14:37:39Z

src/structures/gdt.rs

@@ -44,7 +44,7 @@ use crate::registers::segmentation::{Segment, CS, SS};
 /// // Add entry for TSS, call gdt.load() then update segment registers
 /// ```

-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Copy)]


This was proposed and rejected in #166.

Freax13 · 2021-12-01T14:39:03Z

src/registers/control.rs

+bitflags! {
+    /// Configuration flags of the [`Cr8`] register.
+    pub struct Cr8Flags: u64 {
+        /// A value corresponding to the highest-priority interrupt that is to be blocked


We usually end all sentences in comments with a dot. This should be fixed in a others comments too.

Freax13 · 2021-12-01T15:15:17Z

Out of curiosity, is there a reason that you're using MTTRs instead of the PAT? This seems like a lot of code for something that has already been superseded.

Qubasa · 2021-12-02T12:03:56Z

Out of curiosity, is there a reason that you're using MTTRs instead of the PAT? This seems like a lot of code for something that has already been superseded.

Thanks for the throughout review! The problem is that MTTRs are still relevant in x64 and active and can override PAT. Which means it's very important to know how they are configured by default to guard against very hard to pin down bugs. An example: My AMD Bulldozer FX-8600 for example has the default mtrr memory type of uncached. Which means every memory region that is not covered by variable length mtrrs is set to uncached even if the PAT says something different.

Freax13 · 2021-12-02T16:20:10Z

The problem is that MTTRs are still relevant in x64 and active and can override PAT. Which means it's very important to know how they are configured by default to guard against very hard to pin down bugs.

Very interesting! How can this cause bugs? AFAICT the MTRRs can only overwrite the PAT in that they can disable caching that would have been allowed by the PAT. Is this not true or am I missing something else?

Qubasa · 2021-12-03T03:26:54Z

The problem is that MTTRs are still relevant in x64 and active and can override PAT. Which means it's very important to know how they are configured by default to guard against very hard to pin down bugs.

Very interesting! How can this cause bugs? AFAICT the MTRRs can only overwrite the PAT in that they can disable caching that would have been allowed by the PAT. Is this not true or am I missing something else?

Not only no, here is a table which illustrates what side effects can be caused (it's from the AMD manual vol 2 page 263):

Freax13 · 2021-12-03T22:18:46Z

Not only no, here is a table which illustrates what side effects can be caused (it's from the AMD manual vol 2 page 263):

Which one's of these show that the MTRR's can allow caching that isn't allowed by the PAT? The only one I see is the when the PAT is set to UC- and the MTRR is set to WC, but given that that is the sole purpose of UC- I don't think that counts.

I think I found another case where the MTRR's can allow more caching:

A MTRR can change the effective memory type from write-through to write-combine, this matters because write-combine can do out-of-order writes which aren't allowed by write-through.

Qubasa · 2021-12-03T23:02:35Z

@Freax13 Ah very nice! I must admit I didn't look too closely into the table because I just assumed that a different effective caching behavior then what the developer thought would be there can lead to bugs, know I know for real ^^

Qubasa · 2022-03-07T00:06:34Z

If someone wants to open up a new reworked pull request go ahead :-) I sadly won't be able to work on this for at least a couple of months sadly (maybe @Freax13 ?)

Qubasa added 5 commits September 27, 2021 00:15

Added MTRRs and other missing registers

9b5d13e

Fixed MTRRtype

8f1ede6

Fixed MTRRtype

bb44e2c

Changed tuple to array type

f9c28dd

Added derive Copy to gdt to make it static mut compatible

bd9a811

phil-opp added the waiting-for-review Waiting for a review from the maintainers. label Nov 6, 2021

phil-opp reviewed Nov 6, 2021

View reviewed changes

phil-opp added waiting-on-author Waiting for the author to act on review feedback. and removed waiting-for-review Waiting for a review from the maintainers. labels Nov 6, 2021

Qubasa added 2 commits November 17, 2021 23:56

Added suggested fixes from phil-opp

1e0d618

Added suggested fixes from phil-opp

2a370ce

Qubasa requested a review from phil-opp November 17, 2021 23:14

Qubasa added 3 commits November 18, 2021 00:23

Added #Saftey to doc

bac30bc

cargo fmt

df8104b

Added suggested fixes from phil-opp

ac9fae1

phil-opp added waiting-for-review Waiting for a review from the maintainers. and removed waiting-on-author Waiting for the author to act on review feedback. labels Nov 20, 2021

Freax13 requested changes Dec 1, 2021

View reviewed changes

Freax13 added waiting-on-author Waiting for the author to act on review feedback. and removed waiting-for-review Waiting for a review from the maintainers. labels Dec 13, 2021

Fixed warning

355bd52

Updated to latest version

d87eeda

Qubasa added 3 commits September 25, 2022 20:09

Updated to latest version

c0c929c

Fixed compiler complains

b9c8e2a

Hacky fix: binary feature issue of perf_bootloader

d93e721

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added MTRRs and other missing registers #311

Added MTRRs and other missing registers #311

Qubasa commented Sep 26, 2021 •

edited

phil-opp left a comment

Qubasa commented Nov 6, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 Dec 1, 2021

Freax13 commented Dec 1, 2021

Qubasa commented Dec 2, 2021

Freax13 commented Dec 2, 2021

Qubasa commented Dec 3, 2021

Freax13 commented Dec 3, 2021

Qubasa commented Dec 3, 2021

Qubasa commented Mar 7, 2022 •

edited

Added MTRRs and other missing registers #311

Are you sure you want to change the base?

Added MTRRs and other missing registers #311

Conversation

Qubasa commented Sep 26, 2021 • edited

phil-opp left a comment

Choose a reason for hiding this comment

Qubasa commented Nov 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freax13 commented Dec 1, 2021

Qubasa commented Dec 2, 2021

Freax13 commented Dec 2, 2021

Qubasa commented Dec 3, 2021

Freax13 commented Dec 3, 2021

Qubasa commented Dec 3, 2021

Qubasa commented Mar 7, 2022 • edited

Qubasa commented Sep 26, 2021 •

edited

Qubasa commented Mar 7, 2022 •

edited