From bf4950965b7f65c928be0ca56173e904efd84428 Mon Sep 17 00:00:00 2001
From: Ruihan Li <lrh2000@pku.edu.cn>
Date: Fri, 18 Apr 2025 16:15:17 +0800
Subject: [PATCH] Adjust `unsafe` blocks in `syscall.rs`

---
 ostd/src/arch/x86/trap/mod.rs     |  4 ++-
 ostd/src/arch/x86/trap/syscall.rs | 49 ++++++++++++++++++++-----------
 2 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/ostd/src/arch/x86/trap/mod.rs b/ostd/src/arch/x86/trap/mod.rs
index 37cfce988..581ce0759 100644
--- a/ostd/src/arch/x86/trap/mod.rs
+++ b/ostd/src/arch/x86/trap/mod.rs
@@ -123,7 +123,9 @@ pub unsafe fn init() {
     unsafe { gdt::init() };
 
     idt::init();
-    syscall::init();
+
+    // SAFETY: `gdt::init` has been called before.
+    unsafe { syscall::init() };
 }
 
 /// User space context.
diff --git a/ostd/src/arch/x86/trap/syscall.rs b/ostd/src/arch/x86/trap/syscall.rs
index f2179548e..44f7f1e75 100644
--- a/ostd/src/arch/x86/trap/syscall.rs
+++ b/ostd/src/arch/x86/trap/syscall.rs
@@ -36,31 +36,46 @@ global_asm!(
     USER_SS = const super::gdt::USER_SS.0,
 );
 
-pub(super) fn init() {
+/// # Safety
+///
+/// The caller needs to ensure that `gdt::init` has been called before, so the segment selectors
+/// used in the `syscall` and `sysret` instructions have been properly initialized.
+pub(super) unsafe fn init() {
     let cpuid = CpuId::new();
+
+    assert!(cpuid
+        .get_extended_processor_and_feature_identifiers()
+        .unwrap()
+        .has_syscall_sysret());
+    assert!(cpuid.get_extended_feature_info().unwrap().has_fsgsbase());
+
+    // Flags to clear on syscall.
+    //
+    // Linux 5.0 uses TF|DF|IF|IOPL|AC|NT. Reference:
+    // <https://github.com/torvalds/linux/blob/v5.0/arch/x86/kernel/cpu/common.c#L1559-L1562>
+    const RFLAGS_MASK: u64 = 0x47700;
+
+    // SAFETY: The segment selectors are correctly initialized (as upheld by the caller), and the
+    // entry point and flags to clear are also correctly set, so enabling the `syscall` and
+    // `sysret` instructions is safe.
     unsafe {
-        // Enable `syscall` instruction.
-        assert!(cpuid
-            .get_extended_processor_and_feature_identifiers()
-            .unwrap()
-            .has_syscall_sysret());
+        LStar::write(VirtAddr::new(syscall_entry as usize as u64));
+        SFMask::write(RFlags::from_bits(RFLAGS_MASK).unwrap());
+
+        // Enable the `syscall` and `sysret` instructions.
         Efer::update(|efer| {
             efer.insert(EferFlags::SYSTEM_CALL_EXTENSIONS);
         });
+    }
 
-        // Enable `FSGSBASE` instructions.
-        assert!(cpuid.get_extended_feature_info().unwrap().has_fsgsbase());
+    // SAFETY: Enabling the `rdfsbase`, `wrfsbase`, `rdgsbase`, and `wrgsbase` instructions is safe
+    // as long as the kernel properly deals with the arbitrary base values set by the userspace
+    // program. (FIXME: Do we really need to unconditionally enable them?)
+    unsafe {
         Cr4::update(|cr4| {
             cr4.insert(Cr4Flags::FSGSBASE);
-        });
-
-        // Flags to clear on syscall.
-        // Copy from Linux 5.0, TF|DF|IF|IOPL|AC|NT
-        const RFLAGS_MASK: u64 = 0x47700;
-
-        LStar::write(VirtAddr::new(syscall_entry as usize as u64));
-        SFMask::write(RFlags::from_bits(RFLAGS_MASK).unwrap());
-    }
+        })
+    };
 }
 
 extern "sysv64" {