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

[Zhao Liu <zhao1.liu@intel.com>]

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 <pbonzini@redhat.com>
> 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