refactor: assert on vcpu TLS set and use

The logic for setting and using vcpu TLS is Firecacker
internal and critical to Firecracker function properly.
Additionally, errors returned by TLS touching code was
always ignored, making these errors useless.
In addition, remove the TLS reset function, since Firecracker
does not support vcpu movement across different threads.

Signed-off-by: Egor Lazarchuk <[email protected]>

Author: ShadowCurse

src/vmm/src/vstate/vcpu.rs

@@ -51,10 +51,6 @@ pub enum VcpuError {
     VcpuResponse(KvmVcpuError),
     /// Cannot spawn a new vCPU thread: {0}
     VcpuSpawn(io::Error),
-    /// Cannot clean init vcpu TLS
-    VcpuTlsInit,
-    /// Vcpu not present in TLS
-    VcpuTlsNotPresent,
     /// Error with gdb request sent
     #[cfg(feature = "gdb")]
     GdbRequest(GdbTargetError),
@@ -119,33 +115,10 @@ impl Vcpu {
     /// It is a prerequisite to successfully run `init_thread_local_data()` before using
     /// `run_on_thread_local()` on the current thread.
     /// This function will return an error if there already is a `Vcpu` present in the TLS.
-    fn init_thread_local_data(&mut self) -> Result<(), VcpuError> {
+    fn init_thread_local_data(&mut self) {
         Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| {
-            if cell.get().is_some() {
-                return Err(VcpuError::VcpuTlsInit);
-            }
+            assert!(cell.get().is_none());
             cell.set(Some(self as *mut Vcpu));
-            Ok(())
-        })
-    }
-
-    /// Deassociates `self` from the current thread.
-    ///
-    /// Should be called if the current `self` had called `init_thread_local_data()` and
-    /// now needs to move to a different thread.
-    ///
-    /// Fails if `self` was not previously associated with the current thread.
-    fn reset_thread_local_data(&mut self) -> Result<(), VcpuError> {
-        // Best-effort to clean up TLS. If the `Vcpu` was moved to another thread
-        // _before_ running this, then there is nothing we can do.
-        Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| {
-            if let Some(vcpu_ptr) = cell.get() {
-                if std::ptr::eq(vcpu_ptr, self) {
-                    Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| cell.take());
-                    return Ok(());
-                }
-            }
-            Err(VcpuError::VcpuTlsNotPresent)
         })
     }
 
@@ -159,20 +132,15 @@ impl Vcpu {
     ///
     /// This is marked unsafe as it allows temporary aliasing through
     /// dereferencing from pointer an already borrowed `Vcpu`.
-    unsafe fn run_on_thread_local<F>(func: F) -> Result<(), VcpuError>
+    unsafe fn run_on_thread_local<F>(func: F)
     where
         F: FnOnce(&mut Vcpu),
     {
         Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| {
-            if let Some(vcpu_ptr) = cell.get() {
-                // SAFETY: Dereferencing here is safe since `TLS_VCPU_PTR` is populated/non-empty,
-                // and it is being cleared on `Vcpu::drop` so there is no dangling pointer.
-                let vcpu_ref = unsafe { &mut *vcpu_ptr };
-                func(vcpu_ref);
-                Ok(())
-            } else {
-                Err(VcpuError::VcpuTlsNotPresent)
-            }
+            let vcpu_ptr = cell.get().unwrap();
+            // SAFETY: Dereferencing here is safe since `TLS_VCPU_PTR` is populated/non-empty.
+            let vcpu_ref = unsafe { &mut *vcpu_ptr };
+            func(vcpu_ref);
         })
     }
 
@@ -183,7 +151,7 @@ impl Vcpu {
             // SAFETY: This is safe because it's temporarily aliasing the `Vcpu` object, but we are
             // only reading `vcpu.fd` which does not change for the lifetime of the `Vcpu`.
             unsafe {
-                let _ = Vcpu::run_on_thread_local(|vcpu| {
+                Vcpu::run_on_thread_local(|vcpu| {
                     vcpu.kvm_vcpu.fd.set_kvm_immediate_exit(1);
                     fence(Ordering::Release);
                 });
@@ -254,8 +222,7 @@ impl Vcpu {
             .name(format!("fc_vcpu {}", self.kvm_vcpu.index))
             .spawn(move || {
                 let filter = &*seccomp_filter;
-                self.init_thread_local_data()
-                    .expect("Cannot cleanly initialize vcpu TLS.");
+                self.init_thread_local_data();
                 // Synchronization to make sure thread local data is initialized.
                 barrier.wait();
                 self.run(filter);
@@ -617,12 +584,6 @@ fn handle_kvm_exit(
     }
 }
 
-impl Drop for Vcpu {
-    fn drop(&mut self) {
-        let _ = self.reset_thread_local_data();
-    }
-}
-
 /// List of events that the Vcpu can receive.
 #[derive(Debug, Clone)]
 pub enum VcpuEvent {
@@ -1025,43 +986,37 @@ pub(crate) mod tests {
     }
 
     #[test]
-    fn test_vcpu_tls() {
-        let (_, _, mut vcpu) = setup_vcpu(0x1000);
-
+    #[should_panic]
+    fn test_vcpu_tls_not_set_panic() {
         // Running on the TLS vcpu should fail before we actually initialize it.
         unsafe {
-            Vcpu::run_on_thread_local(|_| ()).unwrap_err();
+            Vcpu::run_on_thread_local(|_| ());
         }
+    }
+
+    #[test]
+    fn test_vcpu_tls_set() {
+        let (_, _, mut vcpu) = setup_vcpu(0x1000);
 
         // Initialize vcpu TLS.
-        vcpu.init_thread_local_data().unwrap();
+        vcpu.init_thread_local_data();
 
         // Validate TLS vcpu is the local vcpu by changing the `id` then validating against
         // the one in TLS.
         vcpu.kvm_vcpu.index = 12;
         unsafe {
-            Vcpu::run_on_thread_local(|v| assert_eq!(v.kvm_vcpu.index, 12)).unwrap();
-        }
-
-        // Reset vcpu TLS.
-        vcpu.reset_thread_local_data().unwrap();
-
-        // Running on the TLS vcpu after TLS reset should fail.
-        unsafe {
-            Vcpu::run_on_thread_local(|_| ()).unwrap_err();
+            Vcpu::run_on_thread_local(|v| assert_eq!(v.kvm_vcpu.index, 12));
         }
-
-        // Second reset should return error.
-        vcpu.reset_thread_local_data().unwrap_err();
     }
 
     #[test]
+    #[should_panic]
     fn test_invalid_tls() {
         let (_, _, mut vcpu) = setup_vcpu(0x1000);
         // Initialize vcpu TLS.
-        vcpu.init_thread_local_data().unwrap();
+        vcpu.init_thread_local_data();
         // Trying to initialize non-empty TLS should error.
-        vcpu.init_thread_local_data().unwrap_err();
+        vcpu.init_thread_local_data();
     }
 
     #[test]
@@ -1080,7 +1035,7 @@ pub(crate) mod tests {
         let handle = std::thread::Builder::new()
             .name("test_vcpu_kick".to_string())
             .spawn(move || {
-                vcpu.init_thread_local_data().unwrap();
+                vcpu.init_thread_local_data();
                 // Notify TLS was populated.
                 vcpu_barrier.wait();
                 // Loop for max 1 second to check if the signal handler has run.