[RFC,04/13] rust: add bindings for gpio_{in|out} initialization

Commit Message

Zhao Liu Dec. 5, 2024, 6:07 a.m. UTC

The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to
wrap them as DeviceState's methods in Rust API, which could eliminate
unsafe cases in the device lib.

Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl.

In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust
callback into a C-style callback qemu_irq_handler, add a handler pointer
member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it
needs to define a handler. And for device which just wants to initialize
GPIO out, it can leave the GPIO_IRQ_HANDLER as None.

Then device could use init_gpio_in() and init_gpio_out() to initialize
GPIO in and out, like C code.

Note, for qemu_irq_handler, assume the opaque parameter refers to the
self DeviceState, and this is enough as for now, as it's the most common
case in QEMU.

Signed-off-by: Zhao Liu <[e-mail]>
---
 rust/qemu-api/src/qdev.rs | 55 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Dec. 5, 2024, 6:53 p.m. UTC | #1

On 12/5/24 07:07, Zhao Liu wrote:
> The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to
> wrap them as DeviceState's methods in Rust API, which could eliminate
> unsafe cases in the device lib.
> 
> Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl.
>
> In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust
> callback into a C-style callback qemu_irq_handler, add a handler pointer
> member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it
> needs to define a handler. And for device which just wants to initialize
> GPIO out, it can leave the GPIO_IRQ_HANDLER as None.

This has the same issue as timers, in that you could have (especially 
once someone adds named GPIOs) multiple handlers.  So we need the same 
kind of Fn-based thing here too.

> +/// Trait that defines the irq handler for GPIO in.
> +pub trait DeviceGPIOImpl {
> +    const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None;

Ah, I see that you're placing the qdev_init_gpio_in here so that you
only make that accessible for devices that did implement DeviceGPIOImpl.
However you are not guaranteeing that this _is_ a DeviceState.

If the handler can be passed as a function, the problem of getting the 
GPIO_INT_HANDLER does not exist anymore.  So with the code in rust-next 
you can add these to a trait like

/// Trait for methods of [`DeviceState`] and its subclasses.
pub trait DeviceMethods: ObjectDeref
where
     Self::Target: IsA<DeviceState>,
{
     fn init_gpio_in<F: ...)(&self, lines_num: u32, f: &F) {
     }
}

impl<R: ObjectDeref> DeviceMethods for R where R::Target: 
IsA<DeviceState> {}


> +    fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) {
> +        unsafe {
> +            qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int);
> +        }
> +    }
> +}

Pass a slice &[InterruptSource], and get the "len" from the length of 
the slice.

Paolo

Zhao Liu Dec. 8, 2024, 4:27 p.m. UTC | #2

On Thu, Dec 05, 2024 at 07:53:42PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 19:53:42 +0100
> From: Paolo Bonzini <[e-mail]>
> Subject: Re: [RFC 04/13] rust: add bindings for gpio_{in|out} initialization
> 
> On 12/5/24 07:07, Zhao Liu wrote:
> > The qdev_init_gpio_{in|out} are qdev interfaces, so that it's natural to
> > wrap them as DeviceState's methods in Rust API, which could eliminate
> > unsafe cases in the device lib.
> > 
> > Wrap qdev_init_gpio_{in|out} as methods in a new trait DeviceGPIOImpl.
> > 
> > In addition, for qdev_init_gpio_in(), to convert the idiomatic Rust
> > callback into a C-style callback qemu_irq_handler, add a handler pointer
> > member in DeviceGPIOImpl. For any device needs to initialize GPIO in, it
> > needs to define a handler. And for device which just wants to initialize
> > GPIO out, it can leave the GPIO_IRQ_HANDLER as None.
> 
> This has the same issue as timers, in that you could have (especially once
> someone adds named GPIOs) multiple handlers.  So we need the same kind of
> Fn-based thing here too.

I will refer to the timer callback prototype you suggested and try that
way. Will you rework the timer binding soon? (I'm sorry for bringing such
burden to you).

> > +/// Trait that defines the irq handler for GPIO in.
> > +pub trait DeviceGPIOImpl {
> > +    const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None;
> 
> Ah, I see that you're placing the qdev_init_gpio_in here so that you
> only make that accessible for devices that did implement DeviceGPIOImpl.
> However you are not guaranteeing that this _is_ a DeviceState.

Thank you, I really couldn't think of a good way to implement the
DeviceState method...One reason is that DeviceImpl is a bit confusing to
me, and please see the comment below.

> If the handler can be passed as a function, the problem of getting the
> GPIO_INT_HANDLER does not exist anymore.  So with the code in rust-next you
> can add these to a trait like
> 
> /// Trait for methods of [`DeviceState`] and its subclasses.
> pub trait DeviceMethods: ObjectDeref
> where
>     Self::Target: IsA<DeviceState>,
> {
>     fn init_gpio_in<F: ...)(&self, lines_num: u32, f: &F) {
>     }
> }
> 
> impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState>
> {}
> 

Thank you for your idea! This is a true implementation of the DeviceState
method. I'll try this way!

Additionally, the current DeviceImpl trait is quite special. Although in
Rust, DeviceImpl traits are implemented for device states, DeviceImpl is
actually used for device classes.

Semantically, it might be more natural for DeviceImpl to be a trait for
device classes. However, parameters of its methods are DeviceState, so
it makes sense as a trait for states in Rust. 

This seems to be a different design before C and Rust Qdev.

> > +    fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) {
> > +        unsafe {
> > +            qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int);
> > +        }
> > +    }
> > +}
> 
> Pass a slice &[InterruptSource], and get the "len" from the length of the
> slice.

Thanks! Will change this.

Regards,
Zhao

Paolo Bonzini Dec. 9, 2024, 11:08 a.m. UTC | #3

On Sun, Dec 8, 2024 at 5:09 PM Zhao Liu <[e-mail]> wrote:
> > This has the same issue as timers, in that you could have (especially once
> > someone adds named GPIOs) multiple handlers.  So we need the same kind of
> > Fn-based thing here too.
>
> I will refer to the timer callback prototype you suggested and try that
> way. Will you rework the timer binding soon? (I'm sorry for bringing such
> burden to you).

No, I have written a utility that can be used for all callbacks but
I'll leave it to you to use it for timers. Both because you have
already started the work, and because it helps if one person writes
the code and one uses it.

> Additionally, the current DeviceImpl trait is quite special. Although in
> Rust, DeviceImpl traits are implemented for device states, DeviceImpl is
> actually used for device classes.
>
> Semantically, it might be more natural for DeviceImpl to be a trait for
> device classes. However, parameters of its methods are DeviceState, so
> it makes sense as a trait for states in Rust.
>
> This seems to be a different design before C and Rust Qdev.

I agree that there are differences in how you write the code, due to
the fact that Rust supports associating functions and traits to a
struct, while C only has a global namespace. Also, functions in a
trait can look like both "normal" and static methods, so it's easier
to place all functions in DeviceState. The DeviceClass part is mostly
automatic.

So if Xyz is a struct corresponding to a QOM type, it will:
- include a field of type Abc corresponding to the direct superclass
- implement virtual methods for all superclasses through traits such
as AbcImpl or DefImpl, up to ObjectImpl
- expose its virtual methods to C thorough a blanket implementation of
ClassInitImpl<AbcClass> or ClassInitImpl<DefClass>
- invoke methods through blanket implementations of AbcMethods,
AbcClassMethods etc. for all superclasses

and the structure of all the blanket implementation is always the same:

pub trait DeviceClassMethods: IsA<DeviceState> {...}
impl<T> DeviceClassMethods for T where T: IsA<DeviceState> {}

pub trait DeviceMethods: ObjectDeref
where
    Self::Target: IsA<DeviceState>,
{...}
impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}

impl<T> ClassInitImpl<DeviceClass> for T
where
    T: ClassInitImpl<ObjectClass> + DeviceImpl
{...}

In the future, developers will not need to worry much about these, but
for some time every new device will probably need a few new functions
or even modules in qemu_api.

Paolo

Zhao Liu Jan. 16, 2025, 3:04 a.m. UTC | #4

> and the structure of all the blanket implementation is always the same:
> 
> pub trait DeviceClassMethods: IsA<DeviceState> {...}
> impl<T> DeviceClassMethods for T where T: IsA<DeviceState> {}
> 
> pub trait DeviceMethods: ObjectDeref
> where
>     Self::Target: IsA<DeviceState>,
> {...}
> impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
> 
> impl<T> ClassInitImpl<DeviceClass> for T
> where
>     T: ClassInitImpl<ObjectClass> + DeviceImpl
> {...}
> 

Similiar to timer, now I've also worked out the gpio bindings (but one
thing that's different is that it uses `self` as an parameter) like:

* gpio_in and gpio_out:

/// Trait for methods of [`DeviceState`] and its subclasses.
pub trait DeviceMethods: ObjectDeref
where
    Self::Target: IsA<DeviceState>,
{
    fn init_gpio_in<F>(&self, lines_num: u32, _f: F)
    where
        F: for<'a> FnCall<(&'a Self::Target, u32, u32)>,
    {
        unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
            opaque: *mut c_void,
            lines_num: c_int,
            level: c_int,
        ) {
            // SAFETY: the opaque was passed as a reference to `T`
            F::call((
                unsafe { &*(opaque.cast::<T>()) },
                lines_num 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.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
                Some(gpio_in_cb),
                lines_num as c_int,
            );
        }
    }

    fn init_gpio_out(&self, pins: &[InterruptSource]) {
        assert!(pins.len() > 0);

        unsafe {
            qdev_init_gpio_out(
                self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
                pins[0].as_ptr(),
                pins.len() as c_int,
            );
        }
    }
}

impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}


* Use case in HPET:

impl HPETState {
    ...

    fn handle_legacy_irq(&self, irq: u32, level: u32) {
        if irq == HPET_LEGACY_PIT_INT {
            if !self.is_legacy_mode() {
                self.irqs[0].set(level != 0);
            }
        } else {
            self.rtc_irq_level.set(level as u8);
            if !self.is_legacy_mode() {
                self.irqs[RTC_ISA_IRQ].set(level != 0);
            }
        }
    }

    ...

    fn realize(&self) {
        ...
        self.init_gpio_in(2, HPETState::handle_legacy_irq);
        self.init_gpio_out(from_ref(&self.pit_enabled));
    }
}

---

I made the handler accept the inmuttable reference, but init_gpio_in()
is called in realize(), which now accepts the `&mut self`.

I think init_gpio_in() should be called in realize() so that realize()
needs to become safe in advance (before your plan).

The safe realize() mainly affects pl011, and before you formally
introduce char binding, if you don't mind, I can make this change to
pl011:

-    pub fn realize(&mut self) {
+    pub fn realize(&self) {
         // SAFETY: self.char_backend has the correct size and alignment for a
         // CharBackend object, and its callbacks are of the correct types.
         unsafe {
             qemu_chr_fe_set_handlers(
-                addr_of_mut!(self.char_backend),
+                addr_of!(self.char_backend) as *mut CharBackend,
                 Some(pl011_can_receive),
                 Some(pl011_receive),
                 Some(pl011_event),
                 None,
-                addr_of_mut!(*self).cast::<c_void>(),
+                addr_of!(*self).cast::<c_void>() as *mut c_void,
                 core::ptr::null_mut(),
                 true,
             );

Thanks,
Zhao

Paolo Bonzini Jan. 17, 2025, 9:40 a.m. UTC | #5

Like timer there are just a couple nits here.

On Thu, Jan 16, 2025 at 3:45 AM Zhao Liu <[e-mail]> wrote:
> * gpio_in and gpio_out:
>
> /// Trait for methods of [`DeviceState`] and its subclasses.
> pub trait DeviceMethods: ObjectDeref
> where
>     Self::Target: IsA<DeviceState>,
> {
>     fn init_gpio_in<F>(&self, lines_num: u32, _f: F)

num_lines :)

>     where
>         F: for<'a> FnCall<(&'a Self::Target, u32, u32)>,
>     {
>         unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
>             opaque: *mut c_void,
>             lines_num: c_int,

"line" instead of lines_num.
>         unsafe {
>             qdev_init_gpio_in(
>                 self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,

I think you can use self.as_mut_ptr::<DeviceState>() or something like that.

>         assert!(pins.len() > 0);

!pins.is_empty(). But I am not sure it's needed...
>
>         unsafe {
>             qdev_init_gpio_out(
>                 self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
>                 pins[0].as_ptr(),
>                 pins.len() as c_int,

... if you use instead pins.as_ptr() without the initial dereference.

> impl HPETState {
>     ...
>
>     fn handle_legacy_irq(&self, irq: u32, level: u32) {
>         if irq == HPET_LEGACY_PIT_INT {
>             if !self.is_legacy_mode() {
>                 self.irqs[0].set(level != 0);
>             }
>         } else {
>             self.rtc_irq_level.set(level as u8);

Any reason why you defined rtc_irq_level as InterruptSource<u8>
instead of InterruptSource<u32>?

>     fn realize(&self) {
>         ...
>         self.init_gpio_in(2, HPETState::handle_legacy_irq);
>         self.init_gpio_out(from_ref(&self.pit_enabled));
>     }
> }
>
> ---
>
> I made the handler accept the inmuttable reference, but init_gpio_in()
> is called in realize(), which now accepts the `&mut self`.
>
> I think init_gpio_in() should be called in realize() so that realize()
> needs to become safe in advance (before your plan).
>
> The safe realize() mainly affects pl011, and before you formally
> introduce char binding, if you don't mind, I can make this change to
> pl011:
>
> -    pub fn realize(&mut self) {
> +    pub fn realize(&self) {
>          // SAFETY: self.char_backend has the correct size and alignment for a
>          // CharBackend object, and its callbacks are of the correct types.
>          unsafe {
>              qemu_chr_fe_set_handlers(
> -                addr_of_mut!(self.char_backend),
> +                addr_of!(self.char_backend) as *mut CharBackend,
>                  Some(pl011_can_receive),
>                  Some(pl011_receive),
>                  Some(pl011_event),
>                  None,
> -                addr_of_mut!(*self).cast::<c_void>(),
> +                addr_of!(*self).cast::<c_void>() as *mut c_void,
>                  core::ptr::null_mut(),
>                  true,
>              );

That's fine, yes.

Paolo

Zhao Liu Jan. 17, 2025, 11:14 a.m. UTC | #6

> >         unsafe {
> >             qdev_init_gpio_in(
> >                 self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
> 
> I think you can use self.as_mut_ptr::<DeviceState>() or something like that.

Yes, thank you!

> 
> >         assert!(pins.len() > 0);
> 
> !pins.is_empty().

Yes.

> But I am not sure it's needed...
> >
> >         unsafe {
> >             qdev_init_gpio_out(
> >                 self.upcast::<DeviceState>() as *const DeviceState as *mut DeviceState,
> >                 pins[0].as_ptr(),
> >                 pins.len() as c_int,
> 
> ... if you use instead pins.as_ptr() without the initial dereference.

Emm, pins.as_ptr() returns `*const InterruptSource`, which can't be
converted to `*mut *mut IRQState` with InterruptSource::as_ptr().

So I haven't thought of a better way yet...

> > impl HPETState {
> >     ...
> >
> >     fn handle_legacy_irq(&self, irq: u32, level: u32) {
> >         if irq == HPET_LEGACY_PIT_INT {
> >             if !self.is_legacy_mode() {
> >                 self.irqs[0].set(level != 0);
> >             }
> >         } else {
> >             self.rtc_irq_level.set(level as u8);
> 
> Any reason why you defined rtc_irq_level as InterruptSource<u8>
> instead of InterruptSource<u32>?

Thanks! I missed to clean up this, having previously used u8.

Regards,
Zhao

Paolo Bonzini Jan. 17, 2025, 12:47 p.m. UTC | #7

Il ven 17 gen 2025, 11:55 Zhao Liu <[e-mail]> ha scritto:

> > >         assert!(pins.len() > 0);
> >
> > !pins.is_empty().
>
> Yes.
>
> > But I am not sure it's needed...
> > >
> > >         unsafe {
> > >             qdev_init_gpio_out(
> > >                 self.upcast::<DeviceState>() as *const DeviceState as
> *mut DeviceState,
> > >                 pins[0].as_ptr(),
> > >                 pins.len() as c_int,
> >
> > ... if you use instead pins.as_ptr() without the initial dereference.
>
> Emm, pins.as_ptr() returns `*const InterruptSource`, which can't be
> converted to `*mut *mut IRQState` with InterruptSource::as_ptr().
>

It can be cast, since an InterruptSource is essentially a *mut IRQState,
but I agree it's not pretty.

Maybe add a "pub(crate) fn as_slice_of_qemu_irq(slice: [Self]) -> [*mut
IRQState)" to InterruptSource? It would just do a transmute and build the
new slice with from_raw_parts. And then "let pins =
InterruptSource::as_slice_of_qemu_irq(pins)" lets you use pins.as_ptr().

Paolo

So I haven't thought of a better way yet...
>
> > > impl HPETState {
> > >     ...
> > >
> > >     fn handle_legacy_irq(&self, irq: u32, level: u32) {
> > >         if irq == HPET_LEGACY_PIT_INT {
> > >             if !self.is_legacy_mode() {
> > >                 self.irqs[0].set(level != 0);
> > >             }
> > >         } else {
> > >             self.rtc_irq_level.set(level as u8);
> >
> > Any reason why you defined rtc_irq_level as InterruptSource<u8>
> > instead of InterruptSource<u32>?
>
> Thanks! I missed to clean up this, having previously used u8.
>
> Regards,
> Zhao
>
>

Patch

diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 23a06b377b2c..5e6580b6f261 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -4,12 +4,17 @@ 
 
 //! Bindings to create devices and access device functionality from Rust.
 
-use std::ffi::CStr;
+use std::{
+    ffi::CStr,
+    os::raw::{c_int, c_void},
+    ptr::{addr_of, NonNull},
+};
 
 pub use bindings::{DeviceClass, DeviceState, Property};
 
 use crate::{
-    bindings::{self, Error},
+    bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error},
+    irq::InterruptSource,
     qom::{ClassInitImpl, Object, ObjectClass, ObjectType},
     qom_isa,
     vmstate::VMStateDescription,
@@ -144,3 +149,49 @@  unsafe impl ObjectType for DeviceState {
         unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_DEVICE) };
 }
 qom_isa!(DeviceState: Object);
+
+/// # Safety
+///
+/// We expect the FFI user of this function to pass a valid pointer that
+/// can be downcasted to type `T`. We also expect the device is
+/// readable/writeable from one thread at any time.
+///
+/// Note: Always assume opaque is referred to the self DeviceState, and
+/// this is also the most common case in QEMU.
+unsafe extern "C" fn rust_irq_handler<T: DeviceGPIOImpl>(
+    opaque: *mut c_void,
+    lines_num: c_int,
+    level: c_int,
+) {
+    // SAFETY:
+    // the pointer is convertible to a reference
+    let state = unsafe { NonNull::new(opaque.cast::<T>()).unwrap().as_mut() };
+
+    T::GPIO_IRQ_HANDLER.unwrap()(state, lines_num as u32, level as u32);
+}
+
+/// Trait that defines the irq handler for GPIO in.
+pub trait DeviceGPIOImpl {
+    const GPIO_IRQ_HANDLER: Option<fn(&mut Self, lines_num: u32, level: u32)> = None;
+
+    fn init_gpio_in(&self, lines_num: u32)
+    where
+        Self: Sized,
+    {
+        assert!(Self::GPIO_IRQ_HANDLER.is_some());
+
+        unsafe {
+            qdev_init_gpio_in(
+                addr_of!(*self) as *mut _,
+                Some(rust_irq_handler::<Self>),
+                lines_num as c_int,
+            );
+        }
+    }
+
+    fn init_gpio_out(&self, pins: &InterruptSource, lines_num: u32) {
+        unsafe {
+            qdev_init_gpio_out(addr_of!(*self) as *mut _, pins.as_ptr(), lines_num as c_int);
+        }
+    }
+}