Timers must be pinned in memory, because modify() stores a pointer to them
in the TimerList. To ensure this is the case, replace the separate new()
and init_full() with a single function that returns a pinned box. Because
the only way to obtain a Timer is through Timer::new_full(), modify()
knows that the timer it got is also pinned. In the future the pinning
requirement will be expressed through the pin_init crate instead.
Note that Timer is a bit different from other users of Opaque, in that
it is created in Rust code rather than C code. This is why it has to
use the unsafe Opaque::new() function.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
meson.build | 7 -----
rust/hw/timer/hpet/src/hpet.rs | 23 ++++++++---------
rust/qemu-api/src/timer.rs | 47 ++++++++++++++++++++++------------
3 files changed, 41 insertions(+), 36 deletions(-)
diff --git a/meson.build b/meson.build
index 0a2c61d2bfa..fc02e5fc763 100644
--- a/meson.build
+++ b/meson.build
@@ -4098,13 +4098,6 @@ if have_rust
foreach enum : c_bitfields
bindgen_args += ['--bitfield-enum', enum]
endforeach
- c_nocopy = [
- 'QEMUTimer',
- ]
- # Used to customize Drop trait
- foreach struct : c_nocopy
- bindgen_args += ['--no-copy', struct]
- endforeach
# TODO: Remove this comment when the clang/libclang mismatch issue is solved.
#
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index be27eb0eff4..ce4b289d0c8 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -4,6 +4,7 @@
use std::{
ffi::CStr,
+ pin::Pin,
ptr::{addr_of_mut, null_mut, NonNull},
slice::from_ref,
};
@@ -156,7 +157,7 @@ pub struct HPETTimer {
/// timer N index within the timer block (`HPETState`)
#[doc(alias = "tn")]
index: usize,
- qemu_timer: Option<Box<Timer>>,
+ qemu_timer: Option<Pin<Box<Timer>>>,
/// timer block abstraction containing this timer
state: Option<NonNull<HPETState>>,
@@ -189,18 +190,14 @@ fn init(&mut self, index: usize, state_ptr: *mut
HPETState) -> &mut Self {
}
fn init_timer_with_state(&mut self) {
- self.qemu_timer = Some(Box::new({
- let mut t = Timer::new();
- t.init_full(
- None,
- CLOCK_VIRTUAL,
- Timer::NS,
- 0,
- timer_handler,
- &self.get_state().timers[self.index],
- );
- t
- }));
+ self.qemu_timer = Some(Timer::new_full(
+ None,
+ CLOCK_VIRTUAL,
+ Timer::NS,
+ 0,
+ timer_handler,
+ &self.get_state().timers[self.index],
+ ));
}
fn get_state(&self) -> &HPETState {
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
index a593538917a..a8b2ab058b6 100644
--- a/rust/qemu-api/src/timer.rs
+++ b/rust/qemu-api/src/timer.rs
@@ -2,40 +2,51 @@
// Author(s): Zhao Liu <zhai1....@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-use std::os::raw::{c_int, c_void};
+use std::{
+ os::raw::{c_int, c_void},
+ pin::Pin,
+};
use crate::{
bindings::{self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod,
QEMUClockType},
callbacks::FnCall,
+ cell::Opaque,
};
-pub type Timer = bindings::QEMUTimer;
-pub type TimerListGroup = bindings::QEMUTimerListGroup;
+/// A safe wrapper around [`bindings::QEMUTimer`].
+#[repr(transparent)]
+#[derive(Debug, qemu_api_macros::Wrapper)]
+pub struct Timer(Opaque<bindings::QEMUTimer>);
+
+unsafe impl Send for Timer {}
+unsafe impl Sync for Timer {}
+
+#[repr(transparent)]
+#[derive(qemu_api_macros::Wrapper)]
+pub struct TimerListGroup(Opaque<bindings::QEMUTimerListGroup>);
+
+unsafe impl Send for TimerListGroup {}
+unsafe impl Sync for TimerListGroup {}
impl Timer {
pub const MS: u32 = bindings::SCALE_MS;
pub const US: u32 = bindings::SCALE_US;
pub const NS: u32 = bindings::SCALE_NS;
- pub fn new() -> Self {
- Default::default()
- }
-
- const fn as_mut_ptr(&self) -> *mut Self {
- self as *const Timer as *mut _
- }
-
- pub fn init_full<'timer, 'opaque: 'timer, T, F>(
- &'timer mut self,
+ pub fn new_full<'opaque, T, F>(
timer_list_group: Option<&TimerListGroup>,
clk_type: ClockType,
scale: u32,
attributes: u32,
_cb: F,
opaque: &'opaque T,
- ) where
+ ) -> Pin<Box<Self>>
F: for<'a> FnCall<(&'a T,)>,
{
+ // SAFETY: returning a pinned box makes it certain that the object
+ // will not move when added to a TimerList with `modify()`.
+ let t = unsafe { Box::pin(Self(Opaque::new())) };
let _: () = F::ASSERT_IS_SOME;
/// timer expiration callback
@@ -51,7 +62,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(
- self,
+ t.as_mut_ptr(),
if let Some(g) = timer_list_group {
g as *const TimerListGroup as *mut _
} else {
@@ -62,11 +73,14 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
attributes as c_int,
Some(timer_cb),
(opaque as *const T).cast::<c_void>() as *mut c_void,
- )
+ );
}
+ t
}
pub fn modify(&self, expire_time: u64) {
+ // SAFETY: the only way to obtain a Timer is via methods that return a
+ // Pin<Box<Self>>, therefore the timer is pinned
unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
}
@@ -75,6 +89,7 @@ pub fn delete(&self) {
}
}
+// FIXME: use something like PinnedDrop from the pinned_init crate
impl Drop for Timer {
fn drop(&mut self) {
self.delete()
--
2.48.1
