diff --git a/src/hyperlight_host/src/hypervisor/hyperv_linux.rs b/src/hyperlight_host/src/hypervisor/hyperv_linux.rs index e0cd6631f..512cf3a2c 100644 --- a/src/hyperlight_host/src/hypervisor/hyperv_linux.rs +++ b/src/hyperlight_host/src/hypervisor/hyperv_linux.rs @@ -48,7 +48,7 @@ use super::gdb::{ DebugCommChannel, DebugMemoryAccess, DebugMsg, DebugResponse, GuestDebug, MshvDebug, VcpuStopReason, }; -use super::{HyperlightExit, Hypervisor, InterruptHandle, LinuxInterruptHandle, VirtualCPU}; +use super::{HyperlightExit, Hypervisor, LinuxInterruptHandle, VirtualCPU}; #[cfg(gdb)] use crate::HyperlightError; use crate::hypervisor::get_memory_access_violation; @@ -647,11 +647,15 @@ impl Hypervisor for HypervLinuxDriver { .tid .store(unsafe { libc::pthread_self() as u64 }, Ordering::Release); // Note: if `InterruptHandle::kill()` is called while this thread is **here** + // Cast to internal trait for access to internal methods + let interrupt_handle_internal = + self.interrupt_handle.as_ref() as &dyn super::InterruptHandleInternal; + // (after set_running_bit but before checking cancel_requested): // - kill() will stamp cancel_requested with the current generation // - We will check cancel_requested below and skip the VcpuFd::run() call // - This is the desired behavior - the kill takes effect immediately - let generation = self.interrupt_handle.set_running_bit(); + let generation = interrupt_handle_internal.set_running_bit(); #[cfg(not(gdb))] let debug_interrupt = false; @@ -668,8 +672,7 @@ impl Hypervisor for HypervLinuxDriver { // - kill() will stamp cancel_requested with the current generation // - We will proceed with vcpu.run(), but signals will be sent to interrupt it // - The vcpu will be interrupted and return EINTR (handled below) - let exit_reason = if self - .interrupt_handle + let exit_reason = if interrupt_handle_internal .is_cancel_requested_for_generation(generation) || debug_interrupt { @@ -690,9 +693,8 @@ impl Hypervisor for HypervLinuxDriver { // - kill() continues sending signals to this thread (running bit is still set) // - The signals are harmless (no-op handler), we just need to check cancel_requested // - We load cancel_requested below to determine if this run was cancelled - let cancel_requested = self - .interrupt_handle - .is_cancel_requested_for_generation(generation); + let cancel_requested = + interrupt_handle_internal.is_cancel_requested_for_generation(generation); #[cfg(gdb)] let debug_interrupt = self .interrupt_handle @@ -704,7 +706,7 @@ impl Hypervisor for HypervLinuxDriver { // - kill() continues sending signals until running bit is cleared // - The newly stamped cancel_requested will affect the NEXT vcpu.run() call // - Signals sent now are harmless (no-op handler) - self.interrupt_handle.clear_running_bit(); + interrupt_handle_internal.clear_running_bit(); // At this point, running bit is clear so kill() will stop sending signals. // However, we may still receive delayed signals that were sent before clear_running_bit. // These stale signals are harmless because: @@ -799,7 +801,7 @@ impl Hypervisor for HypervLinuxDriver { // - A signal meant for a different sandbox on the same thread // In these cases, we return Retry to continue execution. if cancel_requested { - self.interrupt_handle.clear_cancel_requested(); + interrupt_handle_internal.clear_cancel_requested(); HyperlightExit::Cancelled() } else { #[cfg(gdb)] @@ -868,7 +870,7 @@ impl Hypervisor for HypervLinuxDriver { self as &mut dyn Hypervisor } - fn interrupt_handle(&self) -> Arc { + fn interrupt_handle(&self) -> Arc { self.interrupt_handle.clone() } diff --git a/src/hyperlight_host/src/hypervisor/hyperv_windows.rs b/src/hyperlight_host/src/hypervisor/hyperv_windows.rs index 6039441ac..6e880c943 100644 --- a/src/hyperlight_host/src/hypervisor/hyperv_windows.rs +++ b/src/hyperlight_host/src/hypervisor/hyperv_windows.rs @@ -45,8 +45,8 @@ use super::surrogate_process_manager::*; use super::windows_hypervisor_platform::{VMPartition, VMProcessor}; use super::wrappers::HandleWrapper; use super::{HyperlightExit, Hypervisor, InterruptHandle, VirtualCPU}; -use crate::hypervisor::get_memory_access_violation; use crate::hypervisor::regs::{CommonFpu, CommonRegisters}; +use crate::hypervisor::{InterruptHandleInternal, get_memory_access_violation}; use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags}; use crate::mem::mgr::SandboxMemoryManager; use crate::mem::ptr::{GuestPtr, RawPtr}; @@ -550,8 +550,12 @@ impl Hypervisor for HypervWindowsDriver { &mut self, #[cfg(feature = "trace_guest")] tc: &mut crate::sandbox::trace::TraceContext, ) -> Result { + // Cast to internal trait for access to internal methods + let interrupt_handle_internal = + self.interrupt_handle.as_ref() as &dyn super::InterruptHandleInternal; + // Get current generation and set running bit - let generation = self.interrupt_handle.set_running_bit(); + let generation = interrupt_handle_internal.set_running_bit(); #[cfg(not(gdb))] let debug_interrupt = false; @@ -562,8 +566,7 @@ impl Hypervisor for HypervWindowsDriver { .load(Ordering::Relaxed); // Check if cancellation was requested for THIS generation - let exit_context = if self - .interrupt_handle + let exit_context = if interrupt_handle_internal .is_cancel_requested_for_generation(generation) || debug_interrupt { @@ -581,18 +584,17 @@ impl Hypervisor for HypervWindowsDriver { }; // Clear running bit - self.interrupt_handle.clear_running_bit(); + interrupt_handle_internal.clear_running_bit(); let is_canceled = exit_context.ExitReason == WHV_RUN_VP_EXIT_REASON(8193i32); // WHvRunVpExitReasonCanceled // Check if this was a manual cancellation (vs internal Windows cancellation) - let cancel_was_requested_manually = self - .interrupt_handle - .is_cancel_requested_for_generation(generation); + let cancel_was_requested_manually = + interrupt_handle_internal.is_cancel_requested_for_generation(generation); // Only clear cancel_requested if we're actually processing a cancellation for this generation if is_canceled && cancel_was_requested_manually { - self.interrupt_handle.clear_cancel_requested(); + interrupt_handle_internal.clear_cancel_requested(); } #[cfg(gdb)] @@ -752,7 +754,7 @@ impl Hypervisor for HypervWindowsDriver { self.processor.set_sregs(sregs) } - fn interrupt_handle(&self) -> Arc { + fn interrupt_handle(&self) -> Arc { self.interrupt_handle.clone() } @@ -1045,6 +1047,7 @@ impl InterruptHandle for WindowsInterruptHandle { // Only call WHvCancelRunVirtualProcessor if VCPU is actually running in guest mode running && unsafe { WHvCancelRunVirtualProcessor(self.partition_handle, 0, 0).is_ok() } } + #[cfg(gdb)] fn kill_from_debugger(&self) -> bool { self.debug_interrupt.store(true, Ordering::Relaxed); @@ -1052,12 +1055,14 @@ impl InterruptHandle for WindowsInterruptHandle { running && unsafe { WHvCancelRunVirtualProcessor(self.partition_handle, 0, 0).is_ok() } } - fn get_call_active(&self) -> &AtomicBool { - &self.call_active + fn dropped(&self) -> bool { + self.dropped.load(Ordering::Relaxed) } +} - fn get_dropped(&self) -> &AtomicBool { - &self.dropped +impl InterruptHandleInternal for WindowsInterruptHandle { + fn get_call_active(&self) -> &AtomicBool { + &self.call_active } fn get_running(&self) -> &AtomicU64 { diff --git a/src/hyperlight_host/src/hypervisor/kvm.rs b/src/hyperlight_host/src/hypervisor/kvm.rs index 9173c3fd9..03f3cd7a3 100644 --- a/src/hyperlight_host/src/hypervisor/kvm.rs +++ b/src/hyperlight_host/src/hypervisor/kvm.rs @@ -33,7 +33,7 @@ use super::gdb::{ DebugCommChannel, DebugMemoryAccess, DebugMsg, DebugResponse, GuestDebug, KvmDebug, VcpuStopReason, }; -use super::{HyperlightExit, Hypervisor, InterruptHandle, LinuxInterruptHandle, VirtualCPU}; +use super::{HyperlightExit, Hypervisor, LinuxInterruptHandle, VirtualCPU}; #[cfg(gdb)] use crate::HyperlightError; use crate::hypervisor::get_memory_access_violation; @@ -623,11 +623,15 @@ impl Hypervisor for KVMDriver { .tid .store(unsafe { libc::pthread_self() as u64 }, Ordering::Release); // Note: if `InterruptHandle::kill()` is called while this thread is **here** + // Cast to internal trait for access to internal methods + let interrupt_handle_internal = + self.interrupt_handle.as_ref() as &dyn super::InterruptHandleInternal; + // (after set_running_bit but before checking cancel_requested): // - kill() will stamp cancel_requested with the current generation // - We will check cancel_requested below and skip the VcpuFd::run() call // - This is the desired behavior - the kill takes effect immediately - let generation = self.interrupt_handle.set_running_bit(); + let generation = interrupt_handle_internal.set_running_bit(); #[cfg(not(gdb))] let debug_interrupt = false; @@ -643,8 +647,7 @@ impl Hypervisor for KVMDriver { // - kill() will stamp cancel_requested with the current generation // - We will proceed with vcpu.run(), but signals will be sent to interrupt it // - The vcpu will be interrupted and return EINTR (handled below) - let exit_reason = if self - .interrupt_handle + let exit_reason = if interrupt_handle_internal .is_cancel_requested_for_generation(generation) || debug_interrupt { @@ -666,9 +669,8 @@ impl Hypervisor for KVMDriver { // - kill() continues sending signals to this thread (running bit is still set) // - The signals are harmless (no-op handler), we just need to check cancel_requested // - We load cancel_requested below to determine if this run was cancelled - let cancel_requested = self - .interrupt_handle - .is_cancel_requested_for_generation(generation); + let cancel_requested = + interrupt_handle_internal.is_cancel_requested_for_generation(generation); #[cfg(gdb)] let debug_interrupt = self .interrupt_handle @@ -680,7 +682,7 @@ impl Hypervisor for KVMDriver { // - kill() continues sending signals until running bit is cleared // - The newly stamped cancel_requested will affect the NEXT vcpu.run() call // - Signals sent now are harmless (no-op handler) - self.interrupt_handle.clear_running_bit(); + interrupt_handle_internal.clear_running_bit(); // At this point, running bit is clear so kill() will stop sending signals. // However, we may still receive delayed signals that were sent before clear_running_bit. // These stale signals are harmless because: @@ -744,7 +746,7 @@ impl Hypervisor for KVMDriver { // - A signal meant for a different sandbox on the same thread // In these cases, we return Retry to continue execution. if cancel_requested { - self.interrupt_handle.clear_cancel_requested(); + interrupt_handle_internal.clear_cancel_requested(); HyperlightExit::Cancelled() } else { #[cfg(gdb)] @@ -818,7 +820,7 @@ impl Hypervisor for KVMDriver { self as &mut dyn Hypervisor } - fn interrupt_handle(&self) -> Arc { + fn interrupt_handle(&self) -> Arc { self.interrupt_handle.clone() } diff --git a/src/hyperlight_host/src/hypervisor/mod.rs b/src/hyperlight_host/src/hypervisor/mod.rs index 743860583..f5575b989 100644 --- a/src/hyperlight_host/src/hypervisor/mod.rs +++ b/src/hyperlight_host/src/hypervisor/mod.rs @@ -63,7 +63,6 @@ pub(crate) mod crashdump; use std::fmt::Debug; use std::str::FromStr; -#[cfg(any(kvm, mshv3))] use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; #[cfg(any(kvm, mshv3))] @@ -178,8 +177,8 @@ pub(crate) trait Hypervisor: Debug + Send { #[cfg(feature = "trace_guest")] tc: &mut crate::sandbox::trace::TraceContext, ) -> Result; - /// Get InterruptHandle to underlying VM - fn interrupt_handle(&self) -> Arc; + /// Get InterruptHandle to underlying VM (returns internal trait) + fn interrupt_handle(&self) -> Arc; /// Get regs #[allow(dead_code)] @@ -462,7 +461,7 @@ impl VirtualCPU { } } -/// A trait for handling interrupts to a sandbox's vcpu +/// A trait for handling interrupts to a sandbox's vcpu (public API) pub trait InterruptHandle: Debug + Send + Sync { /// Interrupt the corresponding sandbox from running. /// @@ -524,33 +523,28 @@ pub trait InterruptHandle: Debug + Send + Sync { #[cfg(gdb)] fn kill_from_debugger(&self) -> bool; - /// Returns the call_active atomic bool reference for default implementations. - /// - /// This is used by default trait methods to access the common call_active field. - fn get_call_active(&self) -> &std::sync::atomic::AtomicBool; + /// Check if the corresponding VM has been dropped. + fn dropped(&self) -> bool; +} - /// Returns the dropped atomic bool reference for default implementations. - /// - /// This is used by default trait methods to access the common dropped field. - fn get_dropped(&self) -> &std::sync::atomic::AtomicBool; +/// Internal trait for interrupt handle implementation details (private, cross-platform). +/// +/// This trait contains all the internal atomics access methods and helper functions +/// that are shared between Linux and Windows implementations. It extends InterruptHandle +/// to inherit the public API. +/// +/// This trait should NOT be used outside of hypervisor implementations. +pub(crate) trait InterruptHandleInternal: InterruptHandle { + /// Returns the call_active atomic bool reference for internal implementations. + fn get_call_active(&self) -> &AtomicBool; - /// Returns the running atomic u64 reference for default implementations. - /// - /// This is used by default trait methods to access the common running field. - fn get_running(&self) -> &std::sync::atomic::AtomicU64; + /// Returns the running atomic u64 reference for internal implementations. + fn get_running(&self) -> &AtomicU64; - /// Returns the cancel_requested atomic u64 reference for default implementations. - /// - /// This is used by default trait methods to access the common cancel_requested field. - fn get_cancel_requested(&self) -> &std::sync::atomic::AtomicU64; + /// Returns the cancel_requested atomic u64 reference for internal implementations. + fn get_cancel_requested(&self) -> &AtomicU64; - /// Default implementation of dropped() - checks the dropped atomic flag. - fn dropped(&self) -> bool { - self.get_dropped() - .load(std::sync::atomic::Ordering::Relaxed) - } - - /// Default implementation of set_call_active() - increments generation and sets flag. + /// Set call_active - increments generation and sets flag. /// /// Increments the generation counter and sets the call_active flag to true, /// indicating that a guest function call is now in progress. This allows @@ -563,11 +557,10 @@ pub trait InterruptHandle: Debug + Send + Sync { /// false otherwise. fn set_call_active(&self) -> bool { self.increment_generation(); - self.get_call_active() - .swap(true, std::sync::atomic::Ordering::AcqRel) + self.get_call_active().swap(true, Ordering::AcqRel) } - /// Default implementation of clear_call_active() - clears the call_active flag. + /// Clear call_active - clears the call_active flag. /// /// Clears the call_active flag, indicating that no guest function call is /// in progress. After this, kill() will have no effect and will return false. @@ -575,13 +568,9 @@ pub trait InterruptHandle: Debug + Send + Sync { /// Must be called at the end of call_guest_function_by_name_no_reset(), /// after the guest call has fully completed (whether successfully or with error). fn clear_call_active(&self) { - self.get_call_active() - .store(false, std::sync::atomic::Ordering::Release) + self.get_call_active().store(false, Ordering::Release) } - // Default implementations for common cancel_requested operations - // These are identical between Linux and Windows implementations - /// Set cancel_requested to true with the given generation. /// /// This stamps the cancellation request with the current generation number, @@ -591,8 +580,7 @@ pub trait InterruptHandle: Debug + Send + Sync { const CANCEL_REQUESTED_BIT: u64 = 1 << 63; const MAX_GENERATION: u64 = CANCEL_REQUESTED_BIT - 1; let value = CANCEL_REQUESTED_BIT | (generation & MAX_GENERATION); - self.get_cancel_requested() - .store(value, std::sync::atomic::Ordering::Release); + self.get_cancel_requested().store(value, Ordering::Release); } /// Clear cancel_requested (reset to no cancellation). @@ -600,8 +588,7 @@ pub trait InterruptHandle: Debug + Send + Sync { /// This is called after a cancellation has been processed to reset the /// cancellation flag for the next guest call. fn clear_cancel_requested(&self) { - self.get_cancel_requested() - .store(0, std::sync::atomic::Ordering::Release); + self.get_cancel_requested().store(0, Ordering::Release); } /// Check if cancel_requested is set for the given generation. @@ -614,9 +601,7 @@ pub trait InterruptHandle: Debug + Send + Sync { fn is_cancel_requested_for_generation(&self, generation: u64) -> bool { const CANCEL_REQUESTED_BIT: u64 = 1 << 63; const MAX_GENERATION: u64 = CANCEL_REQUESTED_BIT - 1; - let raw = self - .get_cancel_requested() - .load(std::sync::atomic::Ordering::Acquire); + let raw = self.get_cancel_requested().load(Ordering::Acquire); let is_set = raw & CANCEL_REQUESTED_BIT != 0; let stored_generation = raw & MAX_GENERATION; is_set && stored_generation == generation @@ -629,11 +614,9 @@ pub trait InterruptHandle: Debug + Send + Sync { fn set_running_bit(&self) -> u64 { const RUNNING_BIT: u64 = 1 << 63; self.get_running() - .fetch_update( - std::sync::atomic::Ordering::Release, - std::sync::atomic::Ordering::Acquire, - |raw| Some(raw | RUNNING_BIT), - ) + .fetch_update(Ordering::Release, Ordering::Acquire, |raw| { + Some(raw | RUNNING_BIT) + }) .map(|raw| raw & !RUNNING_BIT) // Return the current generation .unwrap_or(0) } @@ -649,19 +632,15 @@ pub trait InterruptHandle: Debug + Send + Sync { const RUNNING_BIT: u64 = 1 << 63; const MAX_GENERATION: u64 = RUNNING_BIT - 1; self.get_running() - .fetch_update( - std::sync::atomic::Ordering::Release, - std::sync::atomic::Ordering::Acquire, - |raw| { - let current_generation = raw & !RUNNING_BIT; - let running_bit = raw & RUNNING_BIT; - if current_generation == MAX_GENERATION { - // Restart generation from 0 - return Some(running_bit); - } - Some((current_generation + 1) | running_bit) - }, - ) + .fetch_update(Ordering::Release, Ordering::Acquire, |raw| { + let current_generation = raw & !RUNNING_BIT; + let running_bit = raw & RUNNING_BIT; + if current_generation == MAX_GENERATION { + // Restart generation from 0 + return Some(running_bit); + } + Some((current_generation + 1) | running_bit) + }) .map(|raw| (raw & !RUNNING_BIT) + 1) // Return the NEW generation .unwrap_or(1) // If wrapped, return 1 } @@ -673,9 +652,7 @@ pub trait InterruptHandle: Debug + Send + Sync { /// - generation: current generation counter value fn get_running_and_generation(&self) -> (bool, u64) { const RUNNING_BIT: u64 = 1 << 63; - let raw = self - .get_running() - .load(std::sync::atomic::Ordering::Acquire); + let raw = self.get_running().load(Ordering::Acquire); let running = raw & RUNNING_BIT != 0; let generation = raw & !RUNNING_BIT; (running, generation) @@ -689,7 +666,7 @@ pub trait InterruptHandle: Debug + Send + Sync { fn clear_running_bit(&self) -> u64 { const RUNNING_BIT: u64 = 1 << 63; self.get_running() - .fetch_and(!RUNNING_BIT, std::sync::atomic::Ordering::Release) + .fetch_and(!RUNNING_BIT, Ordering::Release) } } @@ -884,34 +861,31 @@ impl InterruptHandle for LinuxInterruptHandle { // right before sending each signal, ensuring they're always in sync self.send_signal(true) } + #[cfg(gdb)] fn kill_from_debugger(&self) -> bool { self.debug_interrupt.store(true, Ordering::Relaxed); self.send_signal(false) } - fn get_call_active(&self) -> &AtomicBool { - &self.call_active + fn dropped(&self) -> bool { + self.dropped.load(Ordering::Relaxed) } +} - fn get_dropped(&self) -> &AtomicBool { - &self.dropped +#[cfg(any(kvm, mshv3))] +impl InterruptHandleInternal for LinuxInterruptHandle { + fn get_call_active(&self) -> &AtomicBool { + &self.call_active } - #[cfg(any(kvm, mshv))] fn get_running(&self) -> &AtomicU64 { &self.running } - #[cfg(any(kvm, mshv))] fn get_cancel_requested(&self) -> &AtomicU64 { &self.cancel_requested } - - // #[cfg(any(kvm, mshv))] - // fn increment_generation_internal(&self) -> u64 { - // self.increment_generation() - // } } #[cfg(all(test, any(target_os = "windows", kvm)))] diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index abbfb7fb9..2f83b1f5c 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -59,17 +59,17 @@ static SANDBOX_ID_COUNTER: AtomicU64 = AtomicU64::new(0); /// /// Only one guard can exist per interrupt handle at a time - attempting to create /// a second guard will return an error. -struct CallActiveGuard { - interrupt_handle: Arc, +struct CallActiveGuard { + interrupt_handle: Arc, } -impl CallActiveGuard { +impl CallActiveGuard { /// Creates a new guard and marks a guest function call as active. /// /// # Errors /// /// Returns an error if `call_active` is already true (i.e., another guard already exists). - fn new(interrupt_handle: Arc) -> Result { + fn new(interrupt_handle: Arc) -> Result { // Atomically check that call_active is false and set it to true. // This prevents creating multiple guards for the same interrupt handle. let was_active = interrupt_handle.set_call_active(); @@ -82,7 +82,7 @@ impl CallActiveGuard { } } -impl Drop for CallActiveGuard { +impl Drop for CallActiveGuard { fn drop(&mut self) { self.interrupt_handle.clear_call_active(); }