Re: [PATCH 06/10] rust: add bindings for timer

[Paolo Bonzini]

On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> +  c_nocopy = [
> +    'QEMUTimer',
> +  ]
> +  # Used to customize Drop trait
> +  foreach struct : c_nocopy
> +    bindgen_args += ['--no-copy', struct]
> +  endforeach

Nice.

> +pub use bindings::QEMUTimer;
> +
> +use crate::{
> +    bindings::{
> +        self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, 
> QEMUClockType,
> +        QEMUTimerListGroup,
> +    },
> +    callbacks::FnCall,
> +};
> +
> +impl QEMUTimer {
> +    pub fn new() -> Self {
> +        Default::default()
> +    }
> +
> +    pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>(

General question - should the names:

- include the "timer" part, matching QEMU C code, or exclude it to avoid 
repetition? I would say remove it, but I'm open to suggestions and other 
opinions

- include the "QEMU" part? I'd say remove it, similar to ClockType below 
that is:

-pub use bindings::QEMUTimer;
+pub use bindings::QEMUTimer as Timer;
+pub use bindings::QEMUTimerList as TimerList;
+pub use bindings::QEMUTimerListGroup as TimerListGroup;

> +        &'timer mut self,
> +        timer_list_group: Option<&QEMUTimerListGroup>,
> +        clk_type: QEMUClockType,

Please take a ClockType instead.

> +        scale: u32,
> +        attributes: u32,
> +        _f: F,
> +        opaque: &'opaque T,
> +    ) where
> +        F: for<'a> FnCall<(&'a T,)>,
> +    {
> +        /// timer expiration callback
> +        unsafe extern "C" fn rust_timer_handler<T, F: for<'a> FnCall<(&'a 
> T,)>>(
> +            opaque: *mut c_void,
> +        ) {
> +            // SAFETY: the opaque was passed as a reference to `T`.
> +            F::call((unsafe { &*(opaque.cast::<T>()) },))
> +        }

Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the 
qdev_init_clock_in() patch.

> +        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).cast::<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) }
> +    }

This can take &self, because timers are thread-safe:

    pub fn timer_mod(&self, expire_time: u64) {
        unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
    }

    const fn as_mut_ptr(&self) -> *mut Self {
        self as *const QEMUTimer as *mut _
    }

> +}
> +
> +impl Drop for QEMUTimer {
> +    fn drop(&mut self) {
> +        unsafe { timer_del(self as *mut QEMUTimer) }
> +    }

timer_del() can be useful even outside Drop, so

    pub fn timer_del(&self) {
        unsafe { timer_del(self.as_mut_ptr()) }
    }

and just use self.timer_del() here.

> +}
> +
> +pub fn qemu_clock_get_virtual_ns() -> u64 {
> +    // SAFETY:
> +    // Valid parameter.
> +    (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
> +}

Not needed.

> +pub struct ClockType {
> +    pub id: QEMUClockType,
> +}

The field does not have to be "pub" (maybe "pub(self)", I'm not sure 
offhand).

Paolo

> +impl ClockType {
> +    pub fn get_ns(&self) -> u64 {
> +        // SAFETY: cannot be created outside this module, therefore id
> +        // is valid
> +        (unsafe { qemu_clock_get_ns(self.id) }) as u64
> +    }
> +}
> +
> +pub const CLOCK_VIRTUAL: ClockType = ClockType {
> +    id: QEMUClockType::QEMU_CLOCK_VIRTUAL,
> +};
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 54839ce0f510..a35bfbd1760d 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -63,3 +63,4 @@ typedef enum memory_order {
>  #include "migration/vmstate.h"
>  #include "chardev/char-serial.h"
>  #include "exec/memattrs.h"
> +#include "qemu/timer.h"
> --
> 2.34.1