@@ -4100,13 +4100,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.
#
@@ -4,6 +4,7 @@
use std::{
ffi::CStr,
+ pin::Pin,
ptr::{addr_of_mut, null_mut, NonNull},
slice::from_ref,
};
@@ -184,7 +185,9 @@ impl HPETTimer {
fn init(&mut self, index: usize, state: &HPETState) {
*self = HPETTimer {
index,
- qemu_timer: Timer::new(),
+ // SAFETY: the HPETTimer will only be used after the timer
+ // is initialized below.
+ qemu_timer: unsafe { Timer::new() },
state: NonNull::new(state as *const _ as *mut _).unwrap(),
config: 0,
cmp: 0,
@@ -195,7 +198,10 @@ fn init(&mut self, index: usize, state: &HPETState) {
last: 0,
};
- self.qemu_timer.init_full(
+ // SAFETY: HPETTimer is only used as part of HPETState, which is
+ // always pinned.
+ let qemu_timer = unsafe { Pin::new_unchecked(&mut self.qemu_timer) };
+ qemu_timer.init_full(
None,
CLOCK_VIRTUAL,
Timer::NS,
@@ -2,31 +2,51 @@
// Author(s): Zhao Liu <zhai1.liu@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 _
+ /// Create a `Timer` struct without initializing it.
+ ///
+ /// # Safety
+ ///
+ /// The timer must be initialized before it is armed with
+ /// [`modify`](Self::modify).
+ pub unsafe fn new() -> Self {
+ // SAFETY: requirements relayed to callers of Timer::new
+ Self(unsafe { Opaque::zeroed() })
}
+ /// Create a new timer with the given attributes.
pub fn init_full<'timer, 'opaque: 'timer, T, F>(
- &'timer mut self,
+ self: Pin<&'timer mut Self>,
timer_list_group: Option<&TimerListGroup>,
clk_type: ClockType,
scale: u32,
@@ -51,7 +71,7 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
// SAFETY: the opaque outlives the timer
unsafe {
timer_init_full(
- self,
+ self.as_mut_ptr(),
if let Some(g) = timer_list_group {
g as *const TimerListGroup as *mut _
} else {
@@ -67,14 +87,19 @@ pub fn init_full<'timer, 'opaque: 'timer, T, F>(
}
pub fn modify(&self, expire_time: u64) {
+ // SAFETY: the only way to obtain a Timer safely is via methods that
+ // take a Pin<&mut Self>, therefore the timer is pinned
unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
}
pub fn delete(&self) {
+ // SAFETY: the only way to obtain a Timer safely is via methods that
+ // take a Pin<&mut Self>, therefore the timer is pinned
unsafe { timer_del(self.as_mut_ptr()) }
}
}
+// FIXME: use something like PinnedDrop from the pinned_init crate
impl Drop for Timer {
fn drop(&mut self) {
self.delete()
Timers must be pinned in memory, because modify() stores a pointer to them in the TimerList. To express this requirement, change init_full() to take a pinned reference. Because the only way to obtain a Timer is through Timer::new(), which is unsafe, modify() can assume that the timer it got was later initialized; and because the initialization takes a Pin<&mut Timer> modify() can assume that the timer is 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 constructors provided by Opaque; and in fact Timer::new() is also unsafe, because it leaves it to the caller to invoke init_full() before modify(). Without a call to init_full(), modify() will cause a NULL pointer dereference. An alternative could be to combine new() + init_full() by returning a pinned box; however, using a reference makes it easier to express the requirement that the opaque outlives the timer. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- meson.build | 7 ----- rust/hw/timer/hpet/src/hpet.rs | 10 ++++++-- rust/qemu-api/src/timer.rs | 47 ++++++++++++++++++++++++++-------- 3 files changed, 44 insertions(+), 20 deletions(-)