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 <pbonz...@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 <pbonz...@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
