-
Notifications
You must be signed in to change notification settings - Fork 155
Description
nit: Could we have
LinuxInterruptHandle::new(vm, config)andWindowsInterruptHandle::new(vm, config)? Can we also havestruct GenericInterruptHandle(Arc<dyn InterruptHandleImpl>)andGenericInterruptHandle::new(vm, config)? see here
========
suggestion: it would be nice if interupt_handle had a method
enum InterruptibleResult<T> { Completed(T), Interrupted, Cancelled, } fn run_interruptible<T>(&mut self, func: impl FnOnce() -> T) -> InterruptibleResult<T>and all the complexity of the interrupt handler can be contained there
then you can do
let exit_reason = self.interrupt_handle.run_interruptible(|| { #[cfg(feature = "trace_guest")] tc.setup_guest_trace(Span::current().context()); 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. #[cfg(feature = "trace_guest")] { // end tracing stuff } result })But I would say not for this PR, it would just add noise
see here
=====
Can we reach this point? is is possible to call this method, match a region, not be a guard page, and neither an access violation?
I assume this matches the previous behavior, if it does, and we are still not sure if this is reachable, consider adding a TODO comment to check if this is reachable
see here
=====
Restructure feature-gated fields in HyperlightVm struct
#[cfg(target_os = "windows")] handle: HandleWrapper,
#[cfg(target_os = "windows")] raw_size: usize,
#[cfg(gdb)] gdb_conn: Option<DebugCommChannel<DebugResponse, DebugMsg>>,
#[cfg(crashdump)] rt_cfg: SandboxRuntimeConfig,
#[cfg(feature = "mem_profile")] trace_info: MemTraceInfo,
=====
OK, so we no longer capture registers in the log? Again I think that was there because it was used in the past to try and narrow down where issues were (registers such as RIP are/were useful to try and determine what\where errors were happening) I suspect that this isnt as important now that we have crashdump capability
see here