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 <pbonz...@redhat.com>
> Subject: Re: [RFC 07/13] rust: add bindings for timer
>
> On Tue, Jan 14, 2025 at 4:18 PM Zhao Liu <zhao1....@intel.com> 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::MaybeUninit::<Self>::uninit();
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::<T, F>;
// 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<HPETTimer>) {
> > timer_cell.borrow_mut().callback()
> > }
> >
> > impl HPETTimer {
> > ...
> >
> > fn init_timer_with_state(&mut self) {
> > let index = self.index;
> > let cb = |cell: &BqlRefCell<HPETTimer>| {
> > 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
