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

[Zhao Liu]

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?

> Since thread-safety is provided by the BQL,
> what we want here is the newly-introduced BqlCell.  A pointer to the
> contents of the BqlCell (an IRQState**, or equivalently qemu_irq*)
> is then passed to the C sysbus_init_irq function.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 22 ++++++-----
>  rust/qemu-api/meson.build        |  2 +
>  rust/qemu-api/src/irq.rs         | 66 ++++++++++++++++++++++++++++++++
>  rust/qemu-api/src/lib.rs         |  2 +
>  rust/qemu-api/src/sysbus.rs      | 26 +++++++++++++
>  5 files changed, 108 insertions(+), 10 deletions(-)
>  create mode 100644 rust/qemu-api/src/irq.rs
>  create mode 100644 rust/qemu-api/src/sysbus.rs

...

> +    /// 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?

> +    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?

Regards,
Zhao