| Message ID | 20250125125137.1223277-5-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Add HPET timer device | expand |
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote: > + fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(&self, num_lines: u32, _f: F) { > + unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>( > + opaque: *mut c_void, > + line: c_int, > + level: c_int, > + ) { > + // SAFETY: the opaque was passed as a reference to `T` > + F::call((unsafe { &*(opaque.cast::<T>()) }, line as u32, level as u32)) > + } > + > + let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) = > + rust_irq_handler::<Self::Target, F>; Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the qdev_init_clock_in() patch. Paolo
On Wed, Jan 29, 2025 at 11:59:04AM +0100, Paolo Bonzini wrote: > Date: Wed, 29 Jan 2025 11:59:04 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} > initialization > > > > On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > + fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(&self, num_lines: u32, _f: F) { > > + unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>( > > + opaque: *mut c_void, > > + line: c_int, > > + level: c_int, > > + ) { > > + // SAFETY: the opaque was passed as a reference to `T` > > + F::call((unsafe { &*(opaque.cast::<T>()) }, line as u32, level as u32)) > > + } > > + > > + let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) = > > + rust_irq_handler::<Self::Target, F>; > > Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the > qdev_init_clock_in() patch. > Okay. I would add `assert!(F::is_some());` at the beginning of init_gpio_in(). There's a difference with origianl C version: In C side, qdev_get_gpio_in() family could accept a NULL handler, but there's no such case in current QEMU: * qdev_get_gpio_in * qdev_init_gpio_in_named * qdev_init_gpio_in_named_with_opaque And from code logic view, creating an input GPIO line but doing nothing on input, sounds also unusual. So, for simplicity, in the Rust version I make the handler non-optional.
Il ven 7 feb 2025, 09:24 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the > > qdev_init_clock_in() patch. > > > > Okay. > > I would add `assert!(F::is_some());` at the beginning of init_gpio_in(). > Use the "let" so that it's caught at compile time. There's a difference with origianl C version: > > In C side, qdev_get_gpio_in() family could accept a NULL handler, but > there's no such case in current QEMU: > > * qdev_get_gpio_in > * qdev_init_gpio_in_named > * qdev_init_gpio_in_named_with_opaque > > And from code logic view, creating an input GPIO line but doing nothing > on input, sounds also unusual. > Wouldn't it then crash in qemu_set_irq? Paolo So, for simplicity, in the Rust version I make the handler non-optional. > > >
> Use the "let" so that it's caught at compile time. Thanks! Fixed. > There's a difference with origianl C version: > > > > In C side, qdev_get_gpio_in() family could accept a NULL handler, but > > there's no such case in current QEMU: > > > > * qdev_get_gpio_in > > * qdev_init_gpio_in_named > > * qdev_init_gpio_in_named_with_opaque > > > > And from code logic view, creating an input GPIO line but doing nothing > > on input, sounds also unusual. > > > > Wouldn't it then crash in qemu_set_irq? > Yes! Thank you for the education. Regards, Zhao
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs index 32740c873604..96ca8b8aa9ad 100644 --- a/rust/qemu-api/src/qdev.rs +++ b/rust/qemu-api/src/qdev.rs @@ -6,16 +6,17 @@ use std::{ ffi::{CStr, CString}, - os::raw::c_void, + os::raw::{c_int, c_void}, ptr::NonNull, }; pub use bindings::{Clock, ClockEvent, DeviceClass, DeviceState, Property, ResetType}; use crate::{ - bindings::{self, Error, ResettableClass}, + bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error, ResettableClass}, callbacks::FnCall, cell::bql_locked, + irq::{IRQState, InterruptSource}, prelude::*, qom::{ClassInitImpl, ObjectClass, ObjectImpl, Owned}, vmstate::VMStateDescription, @@ -278,6 +279,38 @@ fn do_init_clock_in( // IsA<DeviceState> bound. do_init_clock_in(unsafe { self.as_mut_ptr() }, name, cb, events) } + + fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(&self, num_lines: u32, _f: F) { + unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>( + opaque: *mut c_void, + line: c_int, + level: c_int, + ) { + // SAFETY: the opaque was passed as a reference to `T` + F::call((unsafe { &*(opaque.cast::<T>()) }, line as u32, level as u32)) + } + + let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) = + rust_irq_handler::<Self::Target, F>; + + unsafe { + qdev_init_gpio_in( + self.as_mut_ptr::<DeviceState>(), + Some(gpio_in_cb), + num_lines as c_int, + ); + } + } + + fn init_gpio_out(&self, pins: &[InterruptSource]) { + unsafe { + qdev_init_gpio_out( + self.as_mut_ptr::<DeviceState>(), + InterruptSource::as_slice_of_qemu_irq(pins).as_ptr() as *mut *mut IRQState, + pins.len() as c_int, + ); + } + } } impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
Wrap qdev_init_gpio_{in|out} as methods in DeviceMethods. And for qdev_init_gpio_in, based on FnCall, it can support idiomatic Rust callback without the need for C style wrapper. Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- Changes since RFC: * Use FnCall to support gpio in callback. * Place gpio_{in|out} in DeviceMethods. * Accept &[InterruptSource] as the parameter of gpio_out. --- rust/qemu-api/src/qdev.rs | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)