| Message ID | 20241205060714.256270-8-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Reinvent the wheel for HPET timer in Rust | expand |
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~
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<T: QEMUTimerImpl>( > + 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
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 <pbonzini@redhat.com> > 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<T: QEMUTimerImpl>( > > + 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
> 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
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 000000000000..4f9e8c9277c6 --- /dev/null +++ b/rust/qemu-api/src/timer.rs @@ -0,0 +1,123 @@ +// Copyright (C) 2024 Intel Corporation. +// Author(s): Zhao Liu <zhai1.liu@intel.com> +// 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<T: QEMUTimerImpl>( + timer_list_group: Option<&mut QEMUTimerListGroup>, + clk_type: QEMUClockType, + scale: u32, + attributes: u32, + opaque: &mut T::Opaque, + ) -> Self { + let mut ts: Box<QEMUTimer> = 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::<T>), + (opaque as *mut T::Opaque).cast::<c_void>(), + ) + }; + + *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<T: QEMUTimerImpl>(opaque: *mut c_void) { + // SAFETY: + // the pointer is convertible to a reference + let para = unsafe { NonNull::new(opaque.cast::<T::Opaque>()).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<fn(opaque: &mut Self::Opaque)> = None; + + fn timer_new(clk_type: QEMUClockType, scale: u32, opaque: &mut Self::Opaque) -> QEMUTimer + where + Self: Sized, + { + QEMUTimer::timer_new_full::<Self>(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(clk_type: QEMUClockType, opaque: &mut Self::Opaque) -> QEMUTimer + where + Self: Sized, + { + Self::timer_new(clk_type, SCALE_MS, opaque) + } +} + +pub fn qemu_clock_get_virtual_ns() -> u64 { + // SAFETY: + // Valid parameter. + (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64 +} diff --git a/rust/wrapper.h b/rust/wrapper.h index 033f3e9cf32c..0da42e84933a 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"
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 <zhao1.liu@intel.com> --- 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