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 <pbonzini@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.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 _
- }
-
- 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> - 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,)>,
> {
Ah, the lifetime here isn't effectively bound... However, I also
referred to your latest code [1] :), and it seems that this issue
has already been fixed. (Nit: The code still has a complaint from
`cargo fmt`)
[1]: https://gitlab.com/bonzini/qemu/-/commit/ccb9f6dc738f503a696d8d50f1b5e4576ee80bc6
Regards,
Zhao
On 3/3/25 15:28, Zhao Liu wrote:
>> - 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,)>,
>> {
>
> Ah, the lifetime here isn't effectively bound... However, I also
> referred to your latest code [1] :), and it seems that this issue
> has already been fixed. (Nit: The code still has a complaint from
> `cargo fmt`)
I am not sure if the change I have in that commit actually does
anything, unfortunately... :( which is why I wanted to use init_full
instead of new_full.
It's easiest to marked new_full() unsafe for now.
Paolo
> [1]: https://gitlab.com/bonzini/qemu/-/commit/ccb9f6dc738f503a696d8d50f1b5e4576ee80bc6
On Mon, Mar 03, 2025 at 03:51:25PM +0100, Paolo Bonzini wrote:
> Date: Mon, 3 Mar 2025 15:51:25 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
> express pinning requirements
>
> On 3/3/25 15:28, Zhao Liu wrote:
> > > - 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,)>,
> > > {
> >
> > Ah, the lifetime here isn't effectively bound... However, I also
> > referred to your latest code [1] :), and it seems that this issue
> > has already been fixed. (Nit: The code still has a complaint from
> > `cargo fmt`)
>
> I am not sure if the change I have in that commit actually does anything,
> unfortunately... :( which is why I wanted to use init_full instead of
> new_full.
EMM, I tried with the below case...
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index a440c9f4cb98..c167d69eef4c 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -190,14 +190,17 @@ fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
}
fn init_timer_with_state(&mut self) {
- self.qemu_timer = Some(Timer::new_full(
- None,
- CLOCK_VIRTUAL,
- Timer::NS,
- 0,
- timer_handler,
- &self.get_state().timers[self.index],
- ));
+ {
+ let tmp = &self.get_state().timers[self.index];
+ self.qemu_timer = Some(Timer::new_full(
+ None,
+ CLOCK_VIRTUAL,
+ Timer::NS,
+ 0,
+ timer_handler,
+ tmp,
+ ));
+ }
}
fn get_state(&self) -> &HPETState {
---
It can compile and seems lifetime doesn't work... I think if we want the
lifetime check, it would be necessary to store &opaque in Rust's Timer
wrapper and specify a lifetime for Timer.
Maybe we need something like (similar to MemoryRegionOps):
pub struct Timer<'timer, T> {
Opaque<bindings::QEMUTimer>,
PhantomData<&'timer T>,
}
> It's easiest to marked new_full() unsafe for now.
Yes, I agree.
Thanks,
Zhao
On Thu, Feb 27, 2025 at 03:22:11PM +0100, Paolo Bonzini wrote:
> Date: Thu, 27 Feb 2025 15:22:11 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
> express pinning requirements
> X-Mailer: git-send-email 2.48.1
>
> 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 <pbonzini@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(-)
Great! LGTM,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
(But pls wait, I have a question below...)
> @@ -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-7c99ed862982@redhat.com/
Thanks,
Zhao
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-7c99ed862982@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 <pbonzini@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.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@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 <pbonzini@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 <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(-)
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.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()
--
2.48.1
On Mon, Mar 03, 2025 at 04:58:57PM +0100, Paolo Bonzini wrote:
> Date: Mon, 3 Mar 2025 16:58:57 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/12] rust: timer: wrap QEMUTimer with Opaque<> and
> express pinning requirements
>
> 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-7c99ed862982@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!
Yes! Good point.
> 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 overall design is okay for me too.
> The replacements for this patch are below.
I have comments about current Opaque<> implemation below...
> From 2d74bdf176b2fbeb6205396d0021f68a9e72bde1 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@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.
Good catch!
> Suggested-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/timer/hpet/src/hpet.rs | 59 ++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 31 deletions(-)
Thanks. This cleanup is fine for me!
...
> From 276020645786b6537c50bb37795f281b5d630f27 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@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 <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(-)
...
> 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() })
We should use Opaque::uninit()? Because MaybeUninit::<bindings::QEMUTimer>::zeroed()
marks the timer as initialized, which disables MaybeUninit's ability to check for
initialization. e.g.,
// No compiling error or runtime panic
let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
let _t = unsafe { t.assume_init() };
Further more, I spent some time trying to figure out if MaybeUninit in
Opaque<> could help identify UB caused by uninitialized Timer, but I found
it doesn't work. :-(
There're 2 cases:
// No compiling error or runtime panic
let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
let _v = unsafe { v.get_mut().assume_init() };
// Runtime panic: Illegal instruction
let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::uninit();
let _t = unsafe { t.assume_init() };
I understand that the outer UnsafeCell wrapper makes MaybeUninit's
checks not work.
But when I adjust MaybeUninit as the outer wrapper, the UB check can
work:
// Runtime panic: Illegal instruction
let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
let _v = unsafe { v.assume_init() };
And there's another example:
https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get
Compared with linux's Opaque, it also puts MaybeUninit on the outermost
layer.
Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
mutability is expected... but this layout breaks MaybeUninit's functionality.
> + /// 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
The SAFETY should also be ensured by MaybeUninit, I think. But I haven't
verified if MaybeUninit<UnsafeCell<>> can work on FFI case...
> 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
>
>
>
On 3/4/25 10:13, Zhao Liu wrote:
>> - 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() })
>
> We should use Opaque::uninit()? Because MaybeUninit::<bindings::QEMUTimer>::zeroed()
> marks the timer as initialized, which disables MaybeUninit's ability to check for
> initialization. e.g.,
While neither is good, a zeroed area of memory behaves better than an
uninitialized one... In particular, Drop calls timer_del() which works
fine with a zeroed QEMUTimer. With Opaque::uninit() you could have a
crash just with
drop(Timer::new());
> // No compiling error or runtime panic
> let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
> let _t = unsafe { t.assume_init() };
>
> Further more, I spent some time trying to figure out if MaybeUninit in
> Opaque<> could help identify UB caused by uninitialized Timer, but I found
> it doesn't work. :-(
>
> // No compiling error or runtime panic
> let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
> let _v = unsafe { v.get_mut().assume_init() };
>
> But when I adjust MaybeUninit as the outer wrapper, the UB check can
> work:
>
> // Runtime panic: Illegal instruction
> let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
> let _v = unsafe { v.assume_init() };
>
> Compared with linux's Opaque, it also puts MaybeUninit on the outermost
> layer.
Yes, I admit I just copied what Linux does. :)
> And there's another example:
>
> https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get
>
> Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
> mutability is expected... but this layout breaks MaybeUninit's functionality.
Thanks for the example from the documentation! Indeed it should be
possible to do
/// Returns a raw mutable pointer to the opaque data.
pub const fn as_mut_ptr(&self) -> *mut T {
UnsafeCell::raw_get(self.value.as_ptr())
}
/// Returns a raw pointer to the opaque data that can be passed to a
/// C function as `void *`.
pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
self.as_mut_ptr().cast()
}
pub const fn raw_get(slot: *const Self) -> *mut T {
// SAFETY: even if uninitialized, slot points to a MaybeUninit
let slot = slot.cast::<MaybeUninit<UnsafeCell<T>>>;
UnsafeCell::raw_get(slot.as_ptr())
}
if Opaque<> uses a MaybeUninit<UnsafeCell<T>>. I'm a bit worried of
deviating from what Linux does though...
Paolo
> While neither is good, a zeroed area of memory behaves better than an
> uninitialized one... In particular, Drop calls timer_del() which works fine
> with a zeroed QEMUTimer. With Opaque::uninit() you could have a crash just
> with
>
> drop(Timer::new());
Good point.
> > // No compiling error or runtime panic
> > let t: MaybeUninit<bindings::QEMUTimer> = MaybeUninit::zeroed();
> > let _t = unsafe { t.assume_init() };
> >
> > Further more, I spent some time trying to figure out if MaybeUninit in
> > Opaque<> could help identify UB caused by uninitialized Timer, but I found
> > it doesn't work. :-(
> >
> > // No compiling error or runtime panic
> > let mut v: UnsafeCell<MaybeUninit<bindings::QEMUTimer>> = UnsafeCell::new(MaybeUninit::uninit());
> > let _v = unsafe { v.get_mut().assume_init() };
> >
> > But when I adjust MaybeUninit as the outer wrapper, the UB check can
> > work:
> >
> > // Runtime panic: Illegal instruction
> > let v: MaybeUninit<UnsafeCell<bindings::QEMUTimer>> = MaybeUninit::uninit();
> > let _v = unsafe { v.assume_init() };
> >
> > Compared with linux's Opaque, it also puts MaybeUninit on the outermost
> > layer.
>
> Yes, I admit I just copied what Linux does. :)
Thanks for pointing this! I realized I referred the old code, since this
commit, linux puts the UnsafeCell to the outer layer [2]
[2]: https://github.com/torvalds/linux/commit/35cad617df2eeef8440a38e82bb2d81ae32ca50d
It seems that, at least from the Linux view, here the role of MaybeUninit
(as the cases I tested) is not a main concern, and Rust convention is
superior...
> > And there's another example:
> >
> > https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#method.raw_get
> >
> > Emm, I guess now we have UnsafeCell<MaybeUninit<>> because interior
> > mutability is expected... but this layout breaks MaybeUninit's functionality.
>
> Thanks for the example from the documentation! Indeed it should be possible
> to do
>
> /// Returns a raw mutable pointer to the opaque data.
> pub const fn as_mut_ptr(&self) -> *mut T {
> UnsafeCell::raw_get(self.value.as_ptr())
> }
>
> /// Returns a raw pointer to the opaque data that can be passed to a
> /// C function as `void *`.
> pub const fn as_void_ptr(&self) -> *mut std::ffi::c_void {
> self.as_mut_ptr().cast()
> }
>
> pub const fn raw_get(slot: *const Self) -> *mut T {
> // SAFETY: even if uninitialized, slot points to a MaybeUninit
> let slot = slot.cast::<MaybeUninit<UnsafeCell<T>>>;
> UnsafeCell::raw_get(slot.as_ptr())
> }
>
> if Opaque<> uses a MaybeUninit<UnsafeCell<T>>. I'm a bit worried of
> deviating from what Linux does though...
Thank you, this convertion to UnsafeCell<MaybeUninit<T>> in Linux
history convinces me... I also agree that we should follow it for now :-).
Regards,
Zhao
© 2016 - 2025 Red Hat, Inc.