Re: [RFC 07/13] rust: add bindings for timer
On Tue, Jan 14, 2025 at 05:14:48PM +0100, Paolo Bonzini wrote: > Date: Tue, 14 Jan 2025 17:14:48 +0100 > From: Paolo Bonzini > Subject: Re: [RFC 07/13] rust: add bindings for timer > > On Tue, Jan 14, 2025 at 4:18 PM Zhao Liu wrote: > > ...Now I have a draft for timer binding: > > > > * timer binding: > > > > impl QEMUTimer { > > pub fn new() -> Self { > > Zeroable::ZERO > > } > > Maybe Default too (not sure if you even need new())? Yes, I find bindgen has already implemented Default for QEMUTimer: #[repr(C)] #[derive(Debug)] pub struct QEMUTimer { pub expire_time: i64, pub timer_list: *mut QEMUTimerList, pub cb: QEMUTimerCB, pub opaque: *mut std::os::raw::c_void, pub next: *mut QEMUTimer, pub attributes: std::os::raw::c_int, pub scale: std::os::raw::c_int, } impl Default for QEMUTimer { fn default() -> Self { let mut s = ::std::mem::MaybeUninituninit(); unsafe { ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); s.assume_init() } } } HPETTimer just has a pointer to a QEMUTimer, so I need the new() in init_timer_with_state() to create QEMUTimer within a Box<>. I'll use Default instead in new(). > > pub fn timer_init_full<'a, 'b: 'a, T, F>( > > It's better to use longer names like 'timer and 'opaque. But it's also > always possible to pass a longer lifetime, so 'opaque: 'timer is > strictly speaking not needed: you can just use &'timer T which in turn > means that lifetime elision applies. That said, I think I like the > idea of using 'timer and 'opaque lifetimes here, for clarity. Thanks, I'll change the lifetime names. > > &'a mut self, > > timer_list_group: Option<&mut QEMUTimerListGroup>, > > I think QEMUTimerListGroup can (should) be shared because it's thread safe. I understand here I can pass the immutable reference like (and that's the updated timer_init_full()): pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>( &'timer mut self, timer_list_group: Option<&QEMUTimerListGroup>, clk_type: QEMUClockType, scale: u32, attributes: u32, _f: F, opaque: &'opaque T, ) where F: for<'a> FnCall<(&'a T,)>, { let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::; // SAFETY: the opaque outlives the timer unsafe { timer_init_full( self, if let Some(g) = timer_list_group { g as *const QEMUTimerListGroup as *mut QEMUTimerListGroup } else { ::core::ptr::null_mut() }, clk_type, scale as c_int, attributes as c_int, Some(timer_cb), opaque as *const T as *const c_void as *mut c_void, ) } } > > clk_type: QEMUClockType, > > scale: u32, > > attributes: u32, > > _f: &F, > > Better: "_f: &'static F", or even "_f: F" if it works. "_f: F" can work since I passed a function reference (&cb, not a function pointer). With "_f: F", passing timer_handler directly is better. > > opaque: &'b T, > > ) where > > F: for<'c> FnCall<(&'c T,)> + 'b, > > 'b ('opaque) is not needed here because the opaque is passed _into_ > the function (thus its lifetime is 'c). 'timer would make sense, but > in fact the function itself is always 'static (see FnCall declaration) > so it is unnecessary to add a lifetime to FnCall. I see! Thank you for clarification. > > fn timer_handler(timer_cell: &BqlRefCell) { > > timer_cell.borrow_mut().callback() > > } > > > > impl HPETTimer { > > ... > > > > fn init_timer_with_state(&mut self) { > > let index = self.index; > > let cb = |cell: &BqlRefCell| { > > timer_handler(cell); > > }; > > Why is the anonymous function needed? Can you just pass "timer_handler"? Yes, I should clean up this closure... > > Is this timer binding as you expected? Hope I am on the right track. :-) > > If the only correction is to the function declaration, that's as good > as it can be for Rust! ;) > Thank you! Now I have a updated timer_init_full() (like I pasted above). Regards, Zhao
Re: [RFC 07/13] rust: add bindings for timer
On Tue, Jan 14, 2025 at 4:18 PM Zhao Liu wrote: > ...Now I have a draft for timer binding: > > * timer binding: > > impl QEMUTimer { > pub fn new() -> Self { > Zeroable::ZERO > } Maybe Default too (not sure if you even need new())? > pub fn timer_init_full<'a, 'b: 'a, T, F>( It's better to use longer names like 'timer and 'opaque. But it's also always possible to pass a longer lifetime, so 'opaque: 'timer is strictly speaking not needed: you can just use &'timer T which in turn means that lifetime elision applies. That said, I think I like the idea of using 'timer and 'opaque lifetimes here, for clarity. > &'a mut self, > timer_list_group: Option<&mut QEMUTimerListGroup>, I think QEMUTimerListGroup can (should) be shared because it's thread safe. > clk_type: QEMUClockType, > scale: u32, > attributes: u32, > _f: &F, Better: "_f: &'static F", or even "_f: F" if it works. > opaque: &'b T, > ) where > F: for<'c> FnCall<(&'c T,)> + 'b, 'b ('opaque) is not needed here because the opaque is passed _into_ the function (thus its lifetime is 'c). 'timer would make sense, but in fact the function itself is always 'static (see FnCall declaration) so it is unnecessary to add a lifetime to FnCall. > fn timer_handler(timer_cell: &BqlRefCell) { > timer_cell.borrow_mut().callback() > } > > impl HPETTimer { > ... > > fn init_timer_with_state(&mut self) { > let index = self.index; > let cb = |cell: &BqlRefCell| { > timer_handler(cell); > }; Why is the anonymous function needed? Can you just pass "timer_handler"? > Is this timer binding as you expected? Hope I am on the right track. :-) If the only correction is to the function declaration, that's as good as it can be for Rust! ;) Thanks, Paolo
Re: [RFC 07/13] rust: add bindings for timer
On Tue, Jan 14, 2025 at 11:36:48PM +0800, Zhao Liu wrote: > Date: Tue, 14 Jan 2025 23:36:48 +0800 > From: Zhao Liu > Subject: Re: [RFC 07/13] rust: add bindings for timer > > Hi Paolo, > > Thanks for your FnCall and the guidance below... > > > This gets tricky when you have more than one timer per device. With the > > right > > infrastructure we can make this something like > > > > fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>( > > &'a mut self, > > timer_list_group: Option<&mut QEMUTimerListGroup>, > > clk_type: QEMUClockType, > > scale: u32, > > attributes: u32, > > f: &F, > > opaque: &'b T) { > > // SAFETY: the opaque outlives the timer > > unsafe { > > timer_init_full(...) > > } > > } > > ... > > > pub struct Clock { > > id: QEMUClockType > > } > > > > impl Clock { > > pub fn get_ns() -> u64 { > > // SAFETY: cannot be created outside this module, therefore id > > // is valid > > (unsafe { qemu_clock_get_ns(self.id) }) as u64 > > } > > } > > > > pub static CLOCK_VIRTUAL: Clock = Clock { id: > > QEMUClockType::QEMU_CLOCK_VIRTUAL }; > > // etc. > > > > and then the user can do timer::CLOCK_VIRTUAL::get_ns(). > > > > ...Now I have a draft for timer binding: > > * timer binding: Missed rust_timer_handler, which follows the good example of FnCall doc: unsafe extern "C" fn rust_timer_handler FnCall<(&'a T,)>>(opaque: *mut c_void) { // SAFETY: the opaque was passed as a reference to `T`. F::call((unsafe { &*(opaque.cast::()) },)) } > impl QEMUTimer { > pub fn new() -> Self { > Zeroable::ZERO > } > > pub fn timer_init_full<'a, 'b: 'a, T, F>( > &'a mut self, > timer_list_group: Option<&mut QEMUTimerListGroup>, > clk_type: QEMUClockType, > scale: u32, > attributes: u32, > _f: &F, > opaque: &'b T, > ) where > F: for<'c> FnCall<(&'c T,)> + 'b, > { > let timer_cb: unsafe extern "C" fn(*mut c_void) = > rust_timer_handler::; > > // SAFETY: the opaque outlives the timer > unsafe { > timer_init_full( > self, > if let Some(g) = timer_list_group { > g > } else { > ::core::ptr::null_mut() > }, > clk_type, > scale as c_int, > attributes as c_int, > Some(timer_cb), > opaque as *const T as *const c_void as *mut c_void, > ) > } > } > > pub fn timer_mod(&mut self, expire_time: u64) { > unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) } > } > } > > > * use of timer binding: > > fn timer_handler(timer_cell: &BqlRefCell) { > timer_cell.borrow_mut().callback() > } > > impl HPETTimer { > ... > > fn init_timer_with_state(&mut self) { > let index = self.index; > let cb = |cell: &BqlRefCell| { > timer_handler(cell); > }; > > self.qemu_timer = Some(Box::new({ > let mut t = QEMUTimer::new(); > t.timer_init_full( > None, > CLOCK_VIRTUAL.id, > SCALE_NS, > 0, > &cb, > &self.get_state().timer[index], > ); > t > })); > } > ... > } > > --- > > Is this timer binding as you expected? Hope I am on the right track. :-) > > Thanks, > Zhao > > >
Re: [RFC 07/13] rust: add bindings for timer
Hi Paolo, Thanks for your FnCall and the guidance below... > This gets tricky when you have more than one timer per device. With the right > infrastructure we can make this something like > > fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>( > &'a mut self, > timer_list_group: Option<&mut QEMUTimerListGroup>, > clk_type: QEMUClockType, > scale: u32, > attributes: u32, > f: &F, > opaque: &'b T) { > // SAFETY: the opaque outlives the timer > unsafe { > timer_init_full(...) > } > } ... > pub struct Clock { > id: QEMUClockType > } > > impl Clock { > pub fn get_ns() -> u64 { > // SAFETY: cannot be created outside this module, therefore id > // is valid > (unsafe { qemu_clock_get_ns(self.id) }) as u64 > } > } > > pub static CLOCK_VIRTUAL: Clock = Clock { id: > QEMUClockType::QEMU_CLOCK_VIRTUAL }; > // etc. > > and then the user can do timer::CLOCK_VIRTUAL::get_ns(). > ...Now I have a draft for timer binding: * timer binding: impl QEMUTimer { pub fn new() -> Self { Zeroable::ZERO } pub fn timer_init_full<'a, 'b: 'a, T, F>( &'a mut self, timer_list_group: Option<&mut QEMUTimerListGroup>, clk_type: QEMUClockType, scale: u32, attributes: u32, _f: &F, opaque: &'b T, ) where F: for<'c> FnCall<(&'c T,)> + 'b, { let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::; // SAFETY: the opaque outlives the timer unsafe { timer_init_full( self, if let Some(g) = timer_list_group { g } else { ::core::ptr::null_mut() }, clk_type, scale as c_int, attributes as c_int, Some(timer_cb), opaque as *const T as *const c_void as *mut c_void, ) } } pub fn timer_mod(&mut self, expire_time: u64) { unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) } } } * use of timer binding: fn timer_handler(timer_cell: &BqlRefCell) { timer_cell.borrow_mut().callback() } impl HPETTimer { ... fn init_timer_with_state(&mut self) { let index = self.index; let cb = |cell: &BqlRefCell| { timer_handler(cell); }; self.qemu_timer = Some(Box::new({ let mut t = QEMUTimer::new(); t.timer_init_full( None, CLOCK_VIRTUAL.id, SCALE_NS, 0, &cb, &self.get_state().timer[index], ); t })); } ... } --- Is this timer binding as you expected? Hope I am on the right track. :-) Thanks, Zhao
Re: [RFC 07/13] rust: add bindings for timer
> It might be cleaner to move all of those static inline into timer.c. > I don't see anything performance sensitive there... I agree, thanks! I'll submit a patch to clean up this. Regards, Zhao
Re: [RFC 07/13] rust: add bindings for timer
On Thu, Dec 05, 2024 at 07:46:52PM +0100, Paolo Bonzini wrote: > Date: Thu, 5 Dec 2024 19:46:52 +0100 > From: Paolo Bonzini > Subject: Re: [RFC 07/13] rust: add bindings for timer > > On 12/5/24 07:07, Zhao Liu wrote: > > +impl QEMUTimer { > > +fn new() -> Self { > > +QEMUTimer { > > +expire_time: 0, > > +timer_list: ::core::ptr::null_mut(), > > +cb: None, > > +opaque: ::core::ptr::null_mut(), > > +next: ::core::ptr::null_mut(), > > +attributes: 0, > > +scale: 0, > > +} > > +} > > Please implement Zeroable instead and make this just Self::ZERO. Sure, will do. > > +// TODO: Consider how to avoid passing in C style callbacks directly. > > +fn timer_new_full( > > +timer_list_group: Option<&mut QEMUTimerListGroup>, > > +clk_type: QEMUClockType, > > +scale: u32, > > +attributes: u32, > > +opaque: &mut T::Opaque, > > This gets tricky when you have more than one timer per device. With the right > infrastructure we can make this something like > > fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>( > &'a mut self, > timer_list_group: Option<&mut QEMUTimerListGroup>, > clk_type: QEMUClockType, > scale: u32, > attributes: u32, > f: &F, > opaque: &'b T) { > // SAFETY: the opaque outlives the timer > unsafe { > timer_init_full(...) > } > } > > So I guess that's another thing I have to extract and post. :) Thank you so much for your help! I'm happy to help test and review. (if you feel you don't have enough time, I'm also willing to try this way.. and if there's any issue, I'll let you know...) whichever way you think is solid, I'm both happy with it. > BTW don't bother with timer_new, only do the non-pointer timer_init. Do you mean timer_init is enough, and timer_new and its variants are not necessary? > > +pub fn timer_del(&mut self) { > > +unsafe { timer_del(self as *mut QEMUTimer) }; > > +} > > +} > > Please also add a Drop implementation that calls timer_del. Sure! > > +pub fn qemu_clock_get_virtual_ns() -> u64 { > > +// SAFETY: > > +// Valid parameter. > > +(unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as > > u64 > > +} > > Maybe something like > > pub struct Clock { > id: QEMUClockType > } > > impl Clock { > pub fn get_ns() -> u64 { > // SAFETY: cannot be created outside this module, therefore id > // is valid > (unsafe { qemu_clock_get_ns(self.id) }) as u64 > } > } > > pub static CLOCK_VIRTUAL: Clock = Clock { id: > QEMUClockType::QEMU_CLOCK_VIRTUAL }; > // etc. > > and then the user can do timer::CLOCK_VIRTUAL::get_ns(). More idiomatic, thank you! (I feel like I need to switch my mindset even more from current C style.) Regards, Zhao
Re: [RFC 07/13] rust: add bindings for timer
On 12/5/24 07:07, Zhao Liu wrote: +impl QEMUTimer { +fn new() -> Self { +QEMUTimer { +expire_time: 0, +timer_list: ::core::ptr::null_mut(), +cb: None, +opaque: ::core::ptr::null_mut(), +next: ::core::ptr::null_mut(), +attributes: 0, +scale: 0, +} +} Please implement Zeroable instead and make this just Self::ZERO. +// TODO: Consider how to avoid passing in C style callbacks directly. +fn timer_new_full( +timer_list_group: Option<&mut QEMUTimerListGroup>, +clk_type: QEMUClockType, +scale: u32, +attributes: u32, +opaque: &mut T::Opaque, This gets tricky when you have more than one timer per device. With the right infrastructure we can make this something like fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>( &'a mut self, timer_list_group: Option<&mut QEMUTimerListGroup>, clk_type: QEMUClockType, scale: u32, attributes: u32, f: &F, opaque: &'b T) { // SAFETY: the opaque outlives the timer unsafe { timer_init_full(...) } } So I guess that's another thing I have to extract and post. :) BTW don't bother with timer_new, only do the non-pointer timer_init. +pub fn timer_del(&mut self) { +unsafe { timer_del(self as *mut QEMUTimer) }; +} +} Please also add a Drop implementation that calls timer_del. +pub fn qemu_clock_get_virtual_ns() -> u64 { +// SAFETY: +// Valid parameter. +(unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64 +} Maybe something like pub struct Clock { id: QEMUClockType } impl Clock { pub fn get_ns() -> u64 { // SAFETY: cannot be created outside this module, therefore id // is valid (unsafe { qemu_clock_get_ns(self.id) }) as u64 } } pub static CLOCK_VIRTUAL: Clock = Clock { id: QEMUClockType::QEMU_CLOCK_VIRTUAL }; // etc. and then the user can do timer::CLOCK_VIRTUAL::get_ns(). Paolo
Re: [RFC 07/13] rust: add bindings for timer
On 12/5/24 00:07, Zhao Liu wrote: The bindgen supports `static inline` function binding since v0.64.0 as an experimental feature (`--wrap-static-fns`), and stabilizes it after v0.70.0. But the oldest version of bindgen supported by QEMU is v0.60.1, so there's no way to generate the bindings for timer_new() and its variants which are `static inline` (in include/qemu/timer.h). Manually implement bindings to help create new timers in Rust. Additionally, wrap timer_mod(), timer_del() and qemu_clock_get_virtual_ns() as safe functions to make timer interfaces more Rust-idiomatic. In addition, for timer_new() and its variants, to convert the idiomatic Rust callback into a C-style callback QEMUTimerCB, introduce a trait QEMUTimerImpl. For any object needs to initialize a new timer, it needs to implement QEMUTimerImpl trait and define a handler. It might be cleaner to move all of those static inline into timer.c. I don't see anything performance sensitive there... r~
[RFC 07/13] rust: add bindings for timer
The bindgen supports `static inline` function binding since v0.64.0 as an experimental feature (`--wrap-static-fns`), and stabilizes it after v0.70.0. But the oldest version of bindgen supported by QEMU is v0.60.1, so there's no way to generate the bindings for timer_new() and its variants which are `static inline` (in include/qemu/timer.h). Manually implement bindings to help create new timers in Rust. Additionally, wrap timer_mod(), timer_del() and qemu_clock_get_virtual_ns() as safe functions to make timer interfaces more Rust-idiomatic. In addition, for timer_new() and its variants, to convert the idiomatic Rust callback into a C-style callback QEMUTimerCB, introduce a trait QEMUTimerImpl. For any object needs to initialize a new timer, it needs to implement QEMUTimerImpl trait and define a handler. Signed-off-by: Zhao Liu --- rust/qemu-api/meson.build | 1 + rust/qemu-api/src/lib.rs | 1 + rust/qemu-api/src/timer.rs | 123 + rust/wrapper.h | 1 + 4 files changed, 126 insertions(+) create mode 100644 rust/qemu-api/src/timer.rs diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 508986948883..5bf3c3dfab67 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -29,6 +29,7 @@ _qemu_api_rs = static_library( 'src/qdev.rs', 'src/qom.rs', 'src/sysbus.rs', + 'src/timer.rs', 'src/vmstate.rs', 'src/zeroable.rs', ], diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index e60c9ac16409..495261976dbc 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -17,6 +17,7 @@ pub mod qdev; pub mod qom; pub mod sysbus; +pub mod timer; pub mod vmstate; pub mod zeroable; diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs new file mode 100644 index ..4f9e8c9277c6 --- /dev/null +++ b/rust/qemu-api/src/timer.rs @@ -0,0 +1,123 @@ +// Copyright (C) 2024 Intel Corporation. +// Author(s): Zhao Liu +// SPDX-License-Identifier: GPL-2.0-or-later + +use std::{ +borrow::BorrowMut, +boxed::Box, +os::raw::{c_int, c_void}, +ptr::NonNull, +}; + +pub use bindings::QEMUTimer; + +use crate::bindings::{self, *}; + +impl QEMUTimer { +fn new() -> Self { +QEMUTimer { +expire_time: 0, +timer_list: ::core::ptr::null_mut(), +cb: None, +opaque: ::core::ptr::null_mut(), +next: ::core::ptr::null_mut(), +attributes: 0, +scale: 0, +} +} + +// TODO: Consider how to avoid passing in C style callbacks directly. +fn timer_new_full( +timer_list_group: Option<&mut QEMUTimerListGroup>, +clk_type: QEMUClockType, +scale: u32, +attributes: u32, +opaque: &mut T::Opaque, +) -> Self { +let mut ts: Box = Box::new(QEMUTimer::new()); +let group_ptr = if let Some(g) = timer_list_group { +g +} else { +::core::ptr::null_mut() +}; + +// Safety: +// ts is a valid Box object which can borrow a valid mutable +// pointer, and opaque is converted from a reference so it's +// also valid. +unsafe { +timer_init_full( +ts.borrow_mut(), +group_ptr, +clk_type, +scale as c_int, +attributes as c_int, +Some(rust_timer_handler::), +(opaque as *mut T::Opaque).cast::(), +) +}; + +*ts +} + +pub fn timer_mod(&mut self, expire_time: u64) { +unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) } +} + +pub fn timer_del(&mut self) { +unsafe { timer_del(self as *mut QEMUTimer) }; +} +} + +/// timer expiration callback +unsafe extern "C" fn rust_timer_handler(opaque: *mut c_void) { +// SAFETY: +// the pointer is convertible to a reference +let para = unsafe { NonNull::new(opaque.cast::()).unwrap().as_mut() }; + +T::QEMU_TIMER_CB.unwrap()(para); +} + +pub trait QEMUTimerImpl { +type Opaque; + +// To be more general, opaque is mutable here. But it still should +// be protected by BqlCell/BqlRefCell. +// +// FIXME: limit opaque to immutable? +const QEMU_TIMER_CB: Option = None; + +fn timer_new(clk_type: QEMUClockType, scale: u32, opaque: &mut Self::Opaque) -> QEMUTimer +where +Self: Sized, +{ +QEMUTimer::timer_new_full::(None, clk_type, scale, 0, opaque) +} + +fn timer_new_ns(clk_type: QEMUClockType, opaque: &mut Self::Opaque) -> QEMUTimer +where +Self: Sized, +{ +Self::timer_new(clk_type, SCALE_NS, opaque) +} + +fn timer_new_us(clk_type: QEMUClockType, opaque: &mut Self::Opaque) -> QEMUTimer +where +Self: Sized, +{ +Self::timer_new(clk_type, SCALE_US, opaque) +} + +fn timer_new_ms(cl