On 3/3/25 14:48, Zhao Liu wrote:
@@ -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>>>,
I'm removing this Option<> wrapper in migration series. This is because
Option<> can't be treated as pointer as you mentioned in [*].
So for this reason, does this mean that VMStateField cannot accept
Option<>? I realize that all the current VMStateFlags don't seem
compatible with Option<> unless a new flag is introduced.
[*]:
https://lore.kernel.org/qemu-devel/9a0389fa-765c-443b-ac2f-7c99ed862...@redhat.com/
Ok, so let's get rid of the option. I didn't really like it anyway...
If the Timer is embedded in the HPETTimer, there needs to be some
"unsafe" in order to make sure that the pinning is observed, and also
because an uninitialized Timer is bad and can cause a NULL pointer
dereference in modify()... i.e. Timer shouldn't have implemented
Default!
However, the lifetime checks in init_full() are preserved, so overall
this is better---at least for now. Linux also had unsafe initialization
for quite some time, so I'm okay with it.
The replacements for this patch are below.
Paolo
From 2d74bdf176b2fbeb6205396d0021f68a9e72bde1 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonz...@redhat.com>
Date: Mon, 3 Mar 2025 16:27:08 +0100
Subject: [PATCH 1/2] rust: hpet: embed Timer without the Option and Box
indirection
This simplifies things for migration, since Option<Box<QEMUTimer>> does not
implement VMState.
This also shows a soundness issue because Timer::new() will leave a NULL
timer list pointer, which can then be dereferenced by Timer::modify(). It
will be fixed shortly.
Suggested-by: Zhao Liu <zhao1....@intel.com>
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
rust/hw/timer/hpet/src/hpet.rs | 59 ++++++++++++++++------------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index be27eb0eff4..02c81ae048f 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -151,14 +151,14 @@ fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
/// HPET Timer Abstraction
#[repr(C)]
-#[derive(Debug, Default, qemu_api_macros::offsets)]
+#[derive(Debug, qemu_api_macros::offsets)]
pub struct HPETTimer {
/// timer N index within the timer block (`HPETState`)
#[doc(alias = "tn")]
index: usize,
- qemu_timer: Option<Box<Timer>>,
+ qemu_timer: Timer,
/// timer block abstraction containing this timer
- state: Option<NonNull<HPETState>>,
+ state: NonNull<HPETState>,
// Memory-mapped, software visible timer registers
/// Timer N Configuration and Capability Register
@@ -181,32 +181,34 @@ pub struct HPETTimer {
}
impl HPETTimer {
- fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
- *self = HPETTimer::default();
- self.index = index;
- self.state = NonNull::new(state_ptr);
- self
- }
+ fn init(&mut self, index: usize, state: &HPETState) {
+ *self = HPETTimer {
+ index,
+ qemu_timer: Timer::new(),
+ state: NonNull::new(state as *const _ as *mut _).unwrap(),
+ config: 0,
+ cmp: 0,
+ fsb: 0,
+ cmp64: 0,
+ period: 0,
+ wrap_flag: 0,
+ last: 0,
+ };
- 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.init_full(
+ None,
+ CLOCK_VIRTUAL,
+ Timer::NS,
+ 0,
+ timer_handler,
+ &state.timers[self.index],
+ )
}
fn get_state(&self) -> &HPETState {
// SAFETY:
// the pointer is convertible to a reference
- unsafe { self.state.unwrap().as_ref() }
+ unsafe { self.state.as_ref() }
}
fn is_int_active(&self) -> bool {
@@ -330,7 +332,7 @@ fn arm_timer(&mut self, tick: u64) {
}
self.last = ns;
- self.qemu_timer.as_ref().unwrap().modify(self.last);
+ self.qemu_timer.modify(self.last);
}
fn set_timer(&mut self) {
@@ -353,7 +355,7 @@ fn set_timer(&mut self) {
fn del_timer(&mut self) {
// Just remove the timer from the timer_list without destroying
// this timer instance.
- self.qemu_timer.as_ref().unwrap().delete();
+ self.qemu_timer.delete();
if self.is_int_active() {
// For level-triggered interrupt, this leaves interrupt status
@@ -581,13 +583,8 @@ fn handle_legacy_irq(&self, irq: u32, level: u32) {
}
fn init_timer(&self) {
- let raw_ptr: *mut HPETState = self as *const HPETState as *mut
HPETState;
-
for (index, timer) in self.timers.iter().enumerate() {
- timer
- .borrow_mut()
- .init(index, raw_ptr)
- .init_timer_with_state();
+ timer.borrow_mut().init(index, self);
}
}
--
2.48.1
From 276020645786b6537c50bb37795f281b5d630f27 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonz...@redhat.com>
Date: Fri, 14 Feb 2025 12:06:13 +0100
Subject: [PATCH 2/2] rust: timer: wrap QEMUTimer with Opaque<> and express
pinning requirements
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 <pbonz...@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(-)
diff --git a/meson.build b/meson.build
index 9b1c0ba6346..4e8f0a6c00d 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 02c81ae048f..3d3d6ef8eec 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,
};
@@ -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,
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
index a593538917a..f0b04ef95d7 100644
--- a/rust/qemu-api/src/timer.rs
+++ b/rust/qemu-api/src/timer.rs
@@ -2,31 +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 _
+ /// 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()
--
2.48.1
