Merge pull request #1188 from rust-osdev/bishop-clean-up-globals

Clean up duplicate global system table pointer in `uefi::helpers`

Author: phip1611

uefi/CHANGELOG.md

@@ -19,6 +19,7 @@
   documentation for details of obligations for callers.
 - `BootServices::allocate_pool` now returns `NonZero<u8>` instead of
   `*mut u8`.
+- `helpers::system_table` is deprecated, use `table::system_table_boot` instead.
 
 ## Removed
 - Removed the `panic-on-logger-errors` feature of the `uefi` crate. Logger

uefi/src/helpers/mod.rs

@@ -19,13 +19,9 @@
 //! [println_macro]: uefi::println!
 
 use crate::prelude::{Boot, SystemTable};
-use crate::{Result, StatusExt};
-use core::ffi::c_void;
-use core::ptr;
-use core::sync::atomic::{AtomicPtr, Ordering};
+use crate::{table, Result};
 #[doc(hidden)]
 pub use println::_print;
-use uefi_raw::Status;
 
 #[cfg(feature = "global_allocator")]
 mod global_allocator;
@@ -35,25 +31,6 @@ mod logger;
 mod panic_handler;
 mod println;
 
-/// Reference to the system table.
-///
-/// This table is only fully safe to use until UEFI boot services have been exited.
-/// After that, some fields and methods are unsafe to use, see the documentation of
-/// UEFI's ExitBootServices entry point for more details.
-static SYSTEM_TABLE: AtomicPtr<c_void> = AtomicPtr::new(core::ptr::null_mut());
-
-#[must_use]
-fn system_table_opt() -> Option<SystemTable<Boot>> {
-    let ptr = SYSTEM_TABLE.load(Ordering::Acquire);
-    // Safety: the `SYSTEM_TABLE` pointer either be null or a valid system
-    // table.
-    //
-    // Null is the initial value, as well as the value set when exiting boot
-    // services. Otherwise, the value is set by the call to `init`, which
-    // requires a valid system table reference as input.
-    unsafe { SystemTable::from_ptr(ptr) }
-}
-
 /// Obtains a pointer to the system table.
 ///
 /// This is meant to be used by higher-level libraries,
@@ -63,9 +40,9 @@ fn system_table_opt() -> Option<SystemTable<Boot>> {
 ///
 /// The returned pointer is only valid until boot services are exited.
 #[must_use]
-// TODO do we want to keep this public?
+#[deprecated(note = "use uefi::table::system_table_boot instead")]
 pub fn system_table() -> SystemTable<Boot> {
-    system_table_opt().expect("The system table handle is not available")
+    table::system_table_boot().expect("boot services are not active")
 }
 
 /// Initialize all helpers defined in [`uefi::helpers`] whose Cargo features
@@ -77,15 +54,8 @@ pub fn system_table() -> SystemTable<Boot> {
 /// **PLEASE NOTE** that these helpers are meant for the pre exit boot service
 /// epoch. Limited functionality might work after exiting them, such as logging
 /// to the debugcon device.
+#[allow(unused_variables)] // `st` is unused if logger and allocator are disabled
 pub fn init(st: &mut SystemTable<Boot>) -> Result<()> {
-    if system_table_opt().is_some() {
-        // Avoid double initialization.
-        return Status::SUCCESS.to_result_with_val(|| ());
-    }
-
-    // Setup the system table singleton
-    SYSTEM_TABLE.store(st.as_ptr().cast_mut(), Ordering::Release);
-
     // Setup logging and memory allocation
 
     #[cfg(feature = "logger")]
@@ -102,14 +72,6 @@ pub fn init(st: &mut SystemTable<Boot>) -> Result<()> {
 }
 
 pub(crate) fn exit() {
-    // DEBUG: The UEFI spec does not guarantee that this printout will work, as
-    //        the services used by logging might already have been shut down.
-    //        But it works on current OVMF, and can be used as a handy way to
-    //        check that the callback does get called.
-    //
-    // info!("Shutting down the UEFI utility library");
-    SYSTEM_TABLE.store(ptr::null_mut(), Ordering::Release);
-
     #[cfg(feature = "logger")]
     logger::disable();
 

uefi/src/helpers/panic_handler.rs

@@ -1,13 +1,13 @@
-use crate::helpers::system_table_opt;
 use crate::println;
+use crate::table::system_table_boot;
 use cfg_if::cfg_if;
 
 #[panic_handler]
 fn panic_handler(info: &core::panic::PanicInfo) -> ! {
     println!("[PANIC]: {}", info);
 
     // Give the user some time to read the message
-    if let Some(st) = system_table_opt() {
+    if let Some(st) = system_table_boot() {
         st.boot_services().stall(10_000_000);
     } else {
         let mut dummy = 0u64;
@@ -28,7 +28,7 @@ fn panic_handler(info: &core::panic::PanicInfo) -> ! {
             qemu_exit_handle.exit_failure();
         } else {
             // If the system table is available, use UEFI's standard shutdown mechanism
-            if let Some(st) = system_table_opt() {
+            if let Some(st) = system_table_boot() {
                 use crate::table::runtime::ResetType;
                 st.runtime_services()
                     .reset(ResetType::SHUTDOWN, crate::Status::ABORTED, None);

uefi/src/helpers/println.rs

@@ -1,10 +1,11 @@
-use crate::helpers::system_table;
+use crate::table::system_table_boot;
 use core::fmt::Write;
 
 /// INTERNAL API! Helper for print macros.
 #[doc(hidden)]
 pub fn _print(args: core::fmt::Arguments) {
-    system_table()
+    system_table_boot()
+        .expect("boot services are not active")
         .stdout()
         .write_fmt(args)
         .expect("Failed to write to stdout");

uefi/src/table/mod.rs

@@ -34,45 +34,36 @@ pub unsafe fn set_system_table(ptr: *const uefi_raw::table::system::SystemTable)
 }
 
 /// Get the system table while boot services are active.
-///
-/// # Panics
-///
-/// Panics if the system table has not been set with `set_system_table`, or if
-/// boot services are not available (e.g. if [`exit_boot_services`] has been
-/// called).
-///
-/// [`exit_boot_services`]: SystemTable::exit_boot_services
-pub fn system_table_boot() -> SystemTable<Boot> {
+pub fn system_table_boot() -> Option<SystemTable<Boot>> {
     let st = SYSTEM_TABLE.load(Ordering::Acquire);
-    assert!(!st.is_null());
+    if st.is_null() {
+        return None;
+    }
 
     // SAFETY: the system table is valid per the requirements of `set_system_table`.
     unsafe {
         if (*st).boot_services.is_null() {
-            panic!("boot services are not active");
+            None
+        } else {
+            Some(SystemTable::<Boot>::from_ptr(st.cast()).unwrap())
         }
-
-        SystemTable::<Boot>::from_ptr(st.cast()).unwrap()
     }
 }
 
 /// Get the system table while runtime services are active.
-///
-/// # Panics
-///
-/// Panics if the system table has not been set with `set_system_table`, or if
-/// runtime services are not available.
-pub fn system_table_runtime() -> SystemTable<Runtime> {
+pub fn system_table_runtime() -> Option<SystemTable<Runtime>> {
     let st = SYSTEM_TABLE.load(Ordering::Acquire);
-    assert!(!st.is_null());
+    if st.is_null() {
+        return None;
+    }
 
     // SAFETY: the system table is valid per the requirements of `set_system_table`.
     unsafe {
         if (*st).runtime_services.is_null() {
-            panic!("runtime services are not active");
+            None
+        } else {
+            Some(SystemTable::<Runtime>::from_ptr(st.cast()).unwrap())
         }
-
-        SystemTable::<Runtime>::from_ptr(st.cast()).unwrap()
     }
 }