-
Notifications
You must be signed in to change notification settings - Fork 155
Improve InterruptHandle::kill() documentation and clarify boolean return value #1121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
f936bc0
c34ddfa
c8e471e
5350a8c
ad4591d
0daa0fd
21cdb54
28890e7
8b7a60c
533f3a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,7 +166,6 @@ sequenceDiagram | |
| end | ||
|
|
||
| deactivate IH | ||
| IH-->>Caller: sent_signal | ||
| deactivate IH | ||
| ``` | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -208,12 +208,13 @@ pub(crate) trait InterruptHandleImpl: InterruptHandle { | |
| pub trait InterruptHandle: Send + Sync + Debug { | ||
| /// Interrupt the corresponding sandbox from running. | ||
| /// | ||
| /// - If this is called while the the sandbox currently executing a guest function call, it will interrupt the sandbox and return `true`. | ||
| /// - If this is called while the sandbox is not running (for example before or after calling a guest function), it will do nothing and return `false`. | ||
| /// This method sets a cancellation flag that prevents or stops the execution of guest code: | ||
| /// - If called while the sandbox is currently executing a guest function, it will interrupt the vCPU. | ||
| /// - If called before the sandbox starts executing (e.g., before a guest function call), it will prevent execution from starting. | ||
| /// | ||
| /// # Note | ||
| /// This function will block for the duration of the time it takes for the vcpu thread to be interrupted. | ||
| fn kill(&self) -> bool; | ||
| fn kill(&self); | ||
|
|
||
| /// Used by a debugger to interrupt the corresponding sandbox from running. | ||
| /// | ||
|
|
@@ -374,13 +375,13 @@ impl InterruptHandleImpl for LinuxInterruptHandle { | |
|
|
||
| #[cfg(any(kvm, mshv3))] | ||
| impl InterruptHandle for LinuxInterruptHandle { | ||
| fn kill(&self) -> bool { | ||
| fn kill(&self) { | ||
| // Release ordering ensures that any writes before kill() are visible to the vcpu thread | ||
| // when it checks is_cancelled() with Acquire ordering | ||
| self.state.fetch_or(Self::CANCEL_BIT, Ordering::Release); | ||
|
|
||
| // Send signals to interrupt the vcpu if it's currently running | ||
| self.send_signal() | ||
| self.send_signal(); | ||
| } | ||
|
|
||
| #[cfg(gdb)] | ||
|
|
@@ -513,7 +514,7 @@ impl InterruptHandleImpl for WindowsInterruptHandle { | |
|
|
||
| #[cfg(target_os = "windows")] | ||
| impl InterruptHandle for WindowsInterruptHandle { | ||
| fn kill(&self) -> bool { | ||
| fn kill(&self) { | ||
| use windows::Win32::System::Hypervisor::WHvCancelRunVirtualProcessor; | ||
|
|
||
| // Release ordering ensures that any writes before kill() are visible to the vcpu thread | ||
|
|
@@ -524,7 +525,7 @@ impl InterruptHandle for WindowsInterruptHandle { | |
| // This ensures we see the running state set by the vcpu thread | ||
| let state = self.state.load(Ordering::Acquire); | ||
| if state & Self::RUNNING_BIT == 0 { | ||
| return false; | ||
| return; | ||
| } | ||
|
|
||
| // Take read lock to prevent race with WHvDeletePartition in set_dropped(). | ||
|
|
@@ -534,15 +535,19 @@ impl InterruptHandle for WindowsInterruptHandle { | |
| Ok(guard) => guard, | ||
| Err(e) => { | ||
| log::error!("Failed to acquire partition_state read lock: {}", e); | ||
| return false; | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if guard.dropped { | ||
| return false; | ||
| return; | ||
| } | ||
|
|
||
| unsafe { WHvCancelRunVirtualProcessor(guard.handle, 0, 0).is_ok() } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot Lets skip this, we have logging in other places
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the error logging for WHvCancelRunVirtualProcessor in commit 8b7a60c. The method now simply returns |
||
| unsafe { | ||
| if let Err(e) = WHvCancelRunVirtualProcessor(guard.handle, 0, 0) { | ||
| log::error!("Failed to cancel running virtual processor: {}", e); | ||
| } | ||
| }; | ||
|
||
| } | ||
| #[cfg(gdb)] | ||
| fn kill_from_debugger(&self) -> bool { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot This sentence is not entirely accurate. If called before a guest call, indeed the kill() is ignored. But if called after guest call, but before entering the guest, it is still effective. Please analyze the code and update the docs to accurately describe the behavior of kill() to make it easier for readers to understand what kill() does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the documentation to accurately describe the timing behavior. The key clarification is that
kill()is only ignored if called beforeclear_cancel()in the guest function call lifecycle. If called after that point but before entering guest code, it will still prevent execution. See commit 5350a8c.