Skip to content

Commit 477677b

Browse files
committed
refactor: remove the need for Vcpu::Drop
Instead of storing the pointer to the Vcpu in the TLS, store the mmapped `kvm_run` struct instead. This way the Drop implementation for Vcpu is no longer required, since the mmapped `kvm_run` will remain valid until TLS is destroyed by the thread Drop impl. This also removes `unsafe` code from the implementation. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
1 parent 653749e commit 477677b

File tree

2 files changed

+33
-53
lines changed

2 files changed

+33
-53
lines changed

Cargo.lock

Lines changed: 14 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/vmm/src/vstate/vcpu.rs

Lines changed: 19 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// Use of this source code is governed by a BSD-style license that can be
66
// found in the THIRD-PARTY file.
77

8-
use std::cell::Cell;
8+
use std::cell::RefCell;
99
#[cfg(feature = "gdb")]
1010
use std::os::fd::AsRawFd;
1111
use std::sync::atomic::{Ordering, fence};
@@ -14,9 +14,9 @@ use std::sync::{Arc, Barrier};
1414
use std::{fmt, io, thread};
1515

1616
use kvm_bindings::{KVM_SYSTEM_EVENT_RESET, KVM_SYSTEM_EVENT_SHUTDOWN};
17-
use kvm_ioctls::VcpuExit;
1817
#[cfg(feature = "gdb")]
1918
use kvm_ioctls::VcpuFd;
19+
use kvm_ioctls::{KvmRunWrapper, VcpuExit};
2020
use libc::{c_int, c_void, siginfo_t};
2121
use log::{error, info, warn};
2222
use vmm_sys_util::errno;
@@ -69,9 +69,6 @@ pub struct VcpuConfig {
6969
pub cpu_config: CpuConfiguration,
7070
}
7171

72-
// Using this for easier explicit type-casting to help IDEs interpret the code.
73-
type VcpuCell = Cell<Option<*mut Vcpu>>;
74-
7572
/// Error type for [`Vcpu::start_threaded`].
7673
#[derive(Debug, derive_more::From, thiserror::Error)]
7774
#[error("Failed to spawn vCPU thread: {0}")]
@@ -88,7 +85,10 @@ pub enum CopyKvmFdError {
8885
CreateVcpuError(#[from] kvm_ioctls::Error),
8986
}
9087

91-
thread_local!(static TLS_VCPU_PTR: VcpuCell = const { Cell::new(None) });
88+
// Stores the mmap region of `kvm_run` struct for the current Vcpu. This allows for the
89+
// signal handler to safely access the `kvm_run` even when Vcpu is dropped and vcpu fd
90+
// is closed.
91+
thread_local!(static TLS_VCPU_PTR: RefCell<Option<KvmRunWrapper>> = const { RefCell::new(None) });
9292

9393
/// A wrapper around creating and using a vcpu.
9494
#[derive(Debug)]
@@ -118,9 +118,16 @@ impl Vcpu {
118118
/// `run_on_thread_local()` on the current thread.
119119
/// This function will panic if there already is a `Vcpu` present in the TLS.
120120
fn init_thread_local_data(&mut self) {
121-
TLS_VCPU_PTR.with(|cell: &VcpuCell| {
122-
assert!(cell.get().is_none());
123-
cell.set(Some(self as *mut Vcpu));
121+
// Use of `kvm_run` size here is safe because we only
122+
// care about `immediate_exit` field which is at byte offset of 2.
123+
let kvm_run_ptr = KvmRunWrapper::mmap_from_fd(
124+
&self.kvm_vcpu.fd,
125+
std::mem::size_of::<kvm_bindings::kvm_run>(),
126+
)
127+
.unwrap();
128+
TLS_VCPU_PTR.with(|cell| {
129+
assert!(cell.borrow().is_none());
130+
*cell.borrow_mut() = Some(kvm_run_ptr);
124131
})
125132
}
126133

@@ -130,14 +137,9 @@ impl Vcpu {
130137
self.init_thread_local_data();
131138

132139
extern "C" fn handle_signal(_: c_int, _: *mut siginfo_t, _: *mut c_void) {
133-
TLS_VCPU_PTR.with(|cell: &VcpuCell| {
134-
if let Some(vcpu_ptr) = cell.get() {
135-
// SAFETY: Dereferencing here is safe since `TLS_VCPU_PTR` is
136-
// populated/non-empty, and it is being cleared on
137-
// `Vcpu::drop` so there is no dangling pointer.
138-
let vcpu = unsafe { &mut *vcpu_ptr };
139-
140-
vcpu.kvm_vcpu.fd.set_kvm_immediate_exit(1);
140+
TLS_VCPU_PTR.with(|cell| {
141+
if let Some(kvm_run_ptr) = &mut *cell.borrow_mut() {
142+
kvm_run_ptr.as_mut_ref().immediate_exit = 1;
141143
fence(Ordering::Release);
142144
}
143145
})
@@ -569,28 +571,6 @@ fn handle_kvm_exit(
569571
}
570572
}
571573

572-
impl Drop for Vcpu {
573-
fn drop(&mut self) {
574-
TLS_VCPU_PTR.with(|cell| {
575-
// The reason for not asserting TLS being set here is that
576-
// it can happen that Vcpu::Drop is called on vcpus which never were
577-
// put on their threads. This can happen if some error occurs during Vmm
578-
// setup before `start_threaded` call.
579-
if let Some(_vcpu_ptr) = cell.get() {
580-
// During normal runtime there is a strong assumption that vcpus will be
581-
// put on their own threads, thus TLS will be initialized with the
582-
// correct pointer.
583-
// In test we do not put vcpus on separate threads, so TLS will have a value
584-
// of the last created vcpu.
585-
#[cfg(not(test))]
586-
assert!(std::ptr::eq(_vcpu_ptr, self));
587-
588-
TLS_VCPU_PTR.take();
589-
}
590-
})
591-
}
592-
}
593-
594574
/// List of events that the Vcpu can receive.
595575
#[derive(Debug, Clone)]
596576
pub enum VcpuEvent {

0 commit comments

Comments
 (0)