Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
> 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
Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
Il ven 7 feb 2025, 09:24 Zhao Liu 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. > > >
Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
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
> Subject: Re: [PATCH 04/10] rust: add bindings for gpio_{in|out}
> initialization
>
>
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu wrote:
> > +fn init_gpio_in FnCall<(&'a Self::Target, u32,
> > u32)>>(&self, num_lines: u32, _f: F) {
> > +unsafe extern "C" fn rust_irq_handler 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::()) }, line as u32, level
> > as u32))
> > +}
> > +
> > +let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) =
> > +rust_irq_handler::;
>
> 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.
Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu wrote:
+fn init_gpio_in FnCall<(&'a Self::Target, u32, u32)>>(&self,
num_lines: u32, _f: F) {
+unsafe extern "C" fn rust_irq_handler 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::()) }, line as u32, level as
u32))
+}
+
+let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) =
+rust_irq_handler::;
Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the
qdev_init_clock_in() patch.
Paolo
