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

[Paolo Bonzini <pbonzini@redhat.com>]

Like timer there are just a couple nits here.

On Thu, Jan 16, 2025 at 3:45 AM Zhao Liu <zhao1.liu@intel.com> 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