diff mbox series

[06/10] rust: add bindings for timer

Message ID 20250125125137.1223277-7-zhao1.liu@intel.com (mailing list archive)
State New
Headers show
Series rust: Add HPET timer device | expand

Commit Message

Zhao Liu Jan. 25, 2025, 12:51 p.m. UTC
Add timer bindings to help handle idiomatic Rust callbacks.

Additionally, wrap QEMUClockType in ClockType binding to avoid unsafe
calls in device code.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
 * Use FnCall to support timer callback.
 * Only add timer_init_full() binding. timer_new() is unnecessary since
   device (HPET) could create and allocate QEMUTimer.
 * Implement Drop for QEMUTimer.
 * Add ClockType binding.
---
 meson.build                |  7 +++
 rust/qemu-api/meson.build  |  1 +
 rust/qemu-api/src/lib.rs   |  1 +
 rust/qemu-api/src/timer.rs | 92 ++++++++++++++++++++++++++++++++++++++
 rust/wrapper.h             |  1 +
 5 files changed, 102 insertions(+)
 create mode 100644 rust/qemu-api/src/timer.rs

Comments

Paolo Bonzini Jan. 29, 2025, 10:58 a.m. UTC | #1
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
>
Zhao Liu Feb. 7, 2025, 1:33 p.m. UTC | #2
> > +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, 

I agree and I would name it "init()" instead of "init_full()".

> 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;

I notice you've picked another way for IRQState, so I could follow that
like:

pub type Timer = bindings::QEMUTimer;

This style make it easy to add doc (timer binding currently lacks
doc, but I will add it as much as possible).

Another option may be to wrap QEMUTimer as what MemoryRegionOps did, but
timer has only 1 callback so I think it's not necessary.

> > +        &'timer mut self,
> > +        timer_list_group: Option<&QEMUTimerListGroup>,
> > +        clk_type: QEMUClockType,
> 
> Please take a ClockType instead.

Sure.

> > +        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.

Sure. Added it to the beginning of this function.

> > +        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) }
>     }

timer_mod means "modify a timer", so I'd rename this method to "modify"

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

Thanks!

> > +}
> > +
> > +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.

OK, will also rename "timer_del" to "delete".

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

Yes!

> > +pub struct ClockType {
> > +    pub id: QEMUClockType,
> > +}
> 
> The field does not have to be "pub" (maybe "pub(self)", I'm not sure
> offhand).
> 

You're right! Making it private is enough, since I should also make
timer_init_full accept ClockType instead of QEMUClockType.

Thanks,
Zhao
Paolo Bonzini Feb. 7, 2025, 2:55 p.m. UTC | #3
On 2/7/25 14:33, Zhao Liu wrote:
>>> +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,
> 
> I agree and I would name it "init()" instead of "init_full()".

Please keep init_full(); init() would be a version without some of the 
arguments (e.g. the TimerListGroup, or the attributes).

> I notice you've picked another way for IRQState, so I could follow that
> like:
> 
> pub type Timer = bindings::QEMUTimer;
> 
> This style make it easy to add doc (timer binding currently lacks
> doc, but I will add it as much as possible).

Good point.

> Another option may be to wrap QEMUTimer as what MemoryRegionOps did, but
> timer has only 1 callback so I think it's not necessary.

Yes, and we actually should do it sooner or later to add a PhantomPinned 
field, because timers can't move in memory!  But no need to do it now.

>>> +        scale: u32,

While at it, can you add constants for the scale, i.e.

     pub const NS: u32 = bindings::SCALE_NS;
     pub const US: u32 = bindings::SCALE_US;
     pub const MS: u32 = bindings::SCALE_MS;

? Using Timer::NS is clear enough and removes the need to import from 
"bindings".  At least in theory, bindings should not even have to be 
"pub" (or at least that's where the code should move towards).

>>> +    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) }
>>      }
> 
> timer_mod means "modify a timer", so I'd rename this method to "modify"

Yeah, changing mod/del to modify/delete is fine!

Paolo
Zhao Liu Feb. 8, 2025, 11:08 a.m. UTC | #4
> Please keep init_full(); init() would be a version without some of the
> arguments (e.g. the TimerListGroup, or the attributes).

Done.

...

> > > > +        scale: u32,
> 
> While at it, can you add constants for the scale, i.e.
> 
>     pub const NS: u32 = bindings::SCALE_NS;
>     pub const US: u32 = bindings::SCALE_US;
>     pub const MS: u32 = bindings::SCALE_MS;
> 
> ? Using Timer::NS is clear enough and removes the need to import from
> "bindings".  At least in theory, bindings should not even have to be "pub"
> (or at least that's where the code should move towards).

Great! I realized this case, too.

Thanks,
Zhao
diff mbox series

Patch

diff --git a/meson.build b/meson.build
index da91b47be48b..1ec88770a60a 100644
--- a/meson.build
+++ b/meson.build
@@ -4079,6 +4079,13 @@  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/qemu-api/meson.build b/rust/qemu-api/meson.build
index 80eafc7f6bd8..caed2b233991 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 8cc095b13f6f..88825b69cff8 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -24,6 +24,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 000000000000..a1c7c7b4d9e6
--- /dev/null
+++ b/rust/qemu-api/src/timer.rs
@@ -0,0 +1,92 @@ 
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use std::os::raw::{c_int, c_void};
+
+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>(
+        &'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,)>,
+    {
+        /// 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>()) },))
+        }
+
+        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) }
+    }
+}
+
+impl Drop for QEMUTimer {
+    fn drop(&mut self) {
+        unsafe { timer_del(self as *mut QEMUTimer) }
+    }
+}
+
+pub fn qemu_clock_get_virtual_ns() -> u64 {
+    // SAFETY:
+    // Valid parameter.
+    (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
+}
+
+pub struct ClockType {
+    pub id: QEMUClockType,
+}
+
+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"