Re: [RFC 07/13] rust: add bindings for timer

[Zhao Liu <zhao1.liu@intel.com>]

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 <pbonzini@redhat.com>
> Subject: Re: [RFC 07/13] rust: add bindings for timer
> 
> On Tue, Jan 14, 2025 at 4:18 PM Zhao Liu <zhao1.liu@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