Re: [PATCH 2/2] rust: add bindings for interrupt sources

[Paolo Bonzini]

On 11/26/24 14:45, Zhao Liu wrote:
> On Fri, Nov 22, 2024 at 08:47:56AM +0100, Paolo Bonzini wrote:
> > Date: Fri, 22 Nov 2024 08:47:56 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: [PATCH 2/2] rust: add bindings for interrupt sources
> > X-Mailer: git-send-email 2.47.0
> >
> > The InterruptSource bindings let us call qemu_set_irq() and sysbus_init_irq()
> > as safe code.
> >
> > Interrupt sources, qemu_irq in C code, are pointers to IRQState objects.
> > They are QOM link properties and can be written to outside the control
> > of the device (i.e. from a shared reference); therefore they must be
> > interior-mutable in Rust.
>
> Out of curiosity, are there any examples of this situation?

Yes, qdev_connect_gpio_out_named() changes the pointer that is stored 
into the InterruptSource.

> > +    /// Send `level` to the interrupt sink.
> > +    pub fn set(&self, level: bool) {
> > +        unsafe {
> > +            qemu_set_irq(self.0.get(), level.into());
> > +        }
> > +    }
>
> Regarding the boolean discussion, the c_int/i32->boolean conversion
> seems unavoidable if it is changed to a boolean, for example, the
> level parameter in qemu_irq_handler is declared to be c_int, and
> there is a pattern of setting the level in qemu_irq_handler with the
> level irq:
> * hpet_handle_legacy_irq
> * split_irq_handler
> * a9mp_priv_set_irq
> ...
>
> So it feels like a more direct way to follow the use of c_int or i32?
> Inconsistent types for level are always confusing. Maybe we can change
> the type of rust after the C version can be standardized to boolean?

The problem is that auditing the C version would be quite some work, 
which is why I proposed adding the generic argument to Rust.

On the other hand, it's relatively common in C to write int when you 
mean bool, because bool is a relatively recent ("only" 25 years :)) 
addition to C.

> > +    pub(crate) const fn as_ptr(&self) -> *mut *mut IRQState {
> > +        self.0.as_ptr()
> > +    }
> > +}
> > +
> > +impl Default for InterruptSource {
> > +    fn default() -> Self {
> > +        InterruptSource(BqlCell::new(ptr::null_mut()))
> > +    }
> > +}
> > +
>
> I like this idea and this binding is very useful!
>
> HPET also needs qdev_init_gpio_in() and qdev_init_gpio_out().
> Should these two safe binding wrappers be implemented as methods of
> DeviceState, or just the public functions?

qdev_init_gpio_out() is basically the same as sysbus_init_irq() and it 
can be added to DeviceState.

qdev_init_gpio_in() is more complicated because it includes a function 
pointer.  For now please keep it as a direct call to 
bindings::qdev_init_gpio_in(), but we'll have to tackle callbacks soon 
because they appear in all of chardev, GPIO inputs and timers.

Paolo