Add Vm trait

Signed-off-by: Ludvig Liljenberg <[email protected]>

Author: ludfjig

src/hyperlight_host/Cargo.toml

@@ -123,7 +123,7 @@ cfg_aliases = "0.2.1"
 built = { version = "0.8.0", optional = true, features = ["chrono", "git2"] }
 
 [features]
-default = ["kvm", "mshv3", "build-metadata", "init-paging", "gdb"]
+default = ["kvm", "mshv3", "build-metadata", "init-paging"]
 function_call_metrics = []
 executable_heap = []
 # This feature enables printing of debug information to stdout in debug builds

src/hyperlight_host/src/hypervisor/hyperlight_vm.rs

@@ -361,15 +361,6 @@ impl HyperlightVm {
         host_funcs: &Arc<Mutex<FunctionRegistry>>,
         #[cfg(gdb)] dbg_mem_access_fn: Arc<Mutex<SandboxMemoryManager<HostSharedMemory>>>,
     ) -> Result<()> {
-        // ===== KILL() TIMING POINT 1: Between guest function calls =====
-        // Clear any stale cancellation from a previous guest function call or if kill() was called too early.
-        // This ensures that kill() called BETWEEN different guest function calls doesn't affect the next call.
-        //
-        // If kill() was called and ran to completion BEFORE this line executes:
-        //    - kill() has NO effect on this guest function call because CANCEL_BIT is cleared here.
-        //    - NOTE: stale signals can still be delivered, but they will be ignored.
-        self.interrupt_handle.clear_cancel();
-
         // Keeps the trace context and open spans
         #[cfg(feature = "trace_guest")]
         let mut tc = crate::sandbox::trace::TraceContext::new();
@@ -378,9 +369,12 @@ impl HyperlightVm {
             // ===== KILL() TIMING POINT 2: Before set_tid() =====
             // If kill() is called and ran to completion BEFORE this line executes:
             //    - CANCEL_BIT will be set and we will return an early VmExit::Cancelled()
+            //      without sending any signals/WHV api calls
             #[cfg(any(kvm, mshv3))]
             self.interrupt_handle.set_tid();
             self.interrupt_handle.set_running();
+            // NOTE: `set_running()`` must be called before checking `is_cancelled()`
+            // otherwise we risk missing a call to `kill()` because the vcpu would not be marked as running yet so signals won't be sent
 
             let exit_reason = if self.interrupt_handle.is_cancelled()
                 || self.interrupt_handle.is_debug_interrupted()
@@ -390,13 +384,10 @@ impl HyperlightVm {
                 #[cfg(feature = "trace_guest")]
                 tc.setup_guest_trace(Span::current().context());
 
-                // ===== KILL() TIMING POINT 3: Before calling run_vcpu() =====
+                // ==== KILL() TIMING POINT 3: Before calling run() ====
                 // If kill() is called and ran to completion BEFORE this line executes:
-                //    - CANCEL_BIT will be set, but it's too late to prevent entering the guest this iteration
-                //    - Signals will interrupt the guest (RUNNING_BIT=true), causing VmExit::Cancelled()
-                //    - If the guest completes before any signals arrive, kill() may have no effect
-                //      - If there are more iterations to do (IO/host func, etc.), the next iteration will be cancelled
-                let exit_reason = self.vm.run_vcpu();
+                //    - Will still do a VM entry, but signals will be sent until VM exits
+                let result = self.vm.run_vcpu();
 
                 // End current host trace by closing the current span that captures traces
                 // happening when a guest exits and re-enters.
@@ -405,12 +396,15 @@ impl HyperlightVm {
 
                 // Handle the guest trace data if any
                 #[cfg(feature = "trace_guest")]
-                if let Err(e) = self.handle_trace(&mut tc, mem_mgr) {
-                    // If no trace data is available, we just log a message and continue
-                    // Is this the right thing to do?
-                    log::debug!("Error handling guest trace: {:?}", e);
+                {
+                    let regs = self.vm.regs()?;
+                    if let Err(e) = tc.handle_trace(&regs, mem_mgr) {
+                        // If no trace data is available, we just log a message and continue
+                        // Is this the right thing to do?
+                        log::debug!("Error handling guest trace: {:?}", e);
+                    }
                 }
-                exit_reason
+                result
             };
 
             // ===== KILL() TIMING POINT 4: Before clear_running() =====
@@ -511,9 +505,9 @@ impl HyperlightVm {
                         continue;
                     }
 
+                    // If the vcpu was interrupted by a debugger, we need to handle it
                     #[cfg(gdb)]
-                    if debug_interrupted {
-                        // If the vcpu was interrupted by a debugger, we need to handle it
+                    {
                         self.interrupt_handle.clear_debug_interrupt();
                         if let Err(e) =
                             self.handle_debug(dbg_mem_access_fn.clone(), VcpuStopReason::Interrupt)
@@ -772,16 +766,6 @@ impl HyperlightVm {
             Ok(None)
         }
     }
-
-    #[cfg(feature = "trace_guest")]
-    fn handle_trace(
-        &mut self,
-        tc: &mut crate::sandbox::trace::TraceContext,
-        mem_mgr: &mut SandboxMemoryManager<HostSharedMemory>,
-    ) -> Result<()> {
-        let regs = self.vm.regs()?;
-        tc.handle_trace(&regs, mem_mgr)
-    }
 }
 
 impl Drop for HyperlightVm {
@@ -798,7 +782,8 @@ enum MemoryAccess {
     StackGuardPageViolation,
 }
 
-/// Determines if a memory access violation occurred at the given address with the given action type.
+/// Determines if a known memory access violation occurred at the given address with the given action type.
+/// Returns Some(reason) if violation reason could be determined, or None if violation occurred but in unmapped region.
 fn get_memory_access_violation<'a>(
     gpa: usize,
     tried: MemoryRegionFlags,

src/hyperlight_host/src/hypervisor/hyperv_linux.rs

@@ -234,7 +234,6 @@ impl DebuggableVm for MshvVm {
         };
 
         use crate::hypervisor::gdb::arch::{BP_EX_ID, DB_EX_ID};
-        use crate::new_error;
 
         if enabled {
             self.vm_fd
@@ -278,7 +277,6 @@ impl DebuggableVm for MshvVm {
 
     fn add_hw_breakpoint(&mut self, addr: u64) -> Result<()> {
         use crate::hypervisor::gdb::arch::MAX_NO_OF_HW_BP;
-        use crate::new_error;
 
         let mut debug_regs = self.vcpu_fd.get_debug_regs()?;
 
@@ -315,8 +313,6 @@ impl DebuggableVm for MshvVm {
     }
 
     fn remove_hw_breakpoint(&mut self, addr: u64) -> Result<()> {
-        use crate::new_error;
-
         let mut debug_regs = self.vcpu_fd.get_debug_regs()?;
 
         let regs = [

src/hyperlight_host/src/hypervisor/kvm.rs

@@ -149,13 +149,10 @@ impl Vm for KvmVm {
                 exception: debug_exit.exception,
             }),
             Err(e) => match e.errno() {
+                // InterruptHandle::kill() sends a signal (SIGRTMIN+offset) to interrupt the vcpu, which causes EINTR
                 libc::EINTR => Ok(VmExit::Cancelled()),
                 libc::EAGAIN => Ok(VmExit::Retry()),
-
-                other => Ok(VmExit::Unknown(format!(
-                    "Unknown KVM VCPU error: {}",
-                    other
-                ))),
+                _ => Ok(VmExit::Unknown(format!("Unknown KVM VCPU error: {}", e))),
             },
             Ok(other) => Ok(VmExit::Unknown(format!(
                 "Unknown KVM VCPU exit: {:?}",
@@ -215,7 +212,6 @@ impl DebuggableVm for KvmVm {
 
     fn add_hw_breakpoint(&mut self, addr: u64) -> Result<()> {
         use crate::hypervisor::gdb::arch::MAX_NO_OF_HW_BP;
-        use crate::new_error;
 
         // Check if breakpoint already exists
         if self.debug_regs.arch.debugreg[..4].contains(&addr) {
@@ -238,8 +234,6 @@ impl DebuggableVm for KvmVm {
     }
 
     fn remove_hw_breakpoint(&mut self, addr: u64) -> Result<()> {
-        use crate::new_error;
-
         // Find the index of the breakpoint
         let index = self.debug_regs.arch.debugreg[..4]
             .iter()

src/hyperlight_host/src/hypervisor/vm.rs

@@ -56,7 +56,8 @@ pub(crate) trait Vm: Send + Sync + Debug {
     /// Unmap memory region from this VM that has previously been mapped using `map_memory`.
     fn unmap_memory(&mut self, region: (u32, &MemoryRegion)) -> Result<()>;
 
-    /// Runs the vCPU until it exits
+    /// Runs the vCPU until it exits.
+    /// Note: this function should not emit any traces or spans as it is called after guest span is setup
     fn run_vcpu(&mut self) -> Result<VmExit>;
 
     /// Get partition handle
@@ -92,7 +93,7 @@ pub(crate) enum VmExit {
         )
     )]
     Retry(),
-    #[cfg(gdb)]
     /// The vCPU has exited due to a debug event (usually breakpoint)
+    #[cfg(gdb)]
     Debug { dr6: u64, exception: u32 },
 }