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(-)

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> {}
-- 
2.34.1

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