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

[Philippe Mathieu-Daudé]

Hi Paolo,

On 22/11/24 08:47, Paolo Bonzini wrote:
> 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.  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


> diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs
> new file mode 100644
> index 00000000000..7dbff007995
> --- /dev/null
> +++ b/rust/qemu-api/src/irq.rs
> @@ -0,0 +1,66 @@
> +// Copyright 2024 Red Hat, Inc.
> +// Author(s): Paolo Bonzini <pbonzini@redhat.com>
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +//! Bindings for interrupt sources
> +
> +use core::ptr;
> +
> +use crate::{
> +    bindings::{qemu_set_irq, IRQState},
> +    cell::BqlCell,
> +};
> +
> +/// Interrupt sources are used by devices to pass changes to a boolean value to
> +/// other devices (typically interrupt or GPIO controllers).  QEMU interrupt
> +/// sources are always active-high.

So 'always active-high' = true below? (Wondering about pulsation, if the
true -> false transition is always correct).

I understand polarity is not part of this interrupt description, so for
GPIO it has to be modelled elsewhere.

Note the C API allows using qemu_set_irq() for vectored interrupts,
which is why the prototype takes an integer argument and not a boolean.
Is this deliberate to restrict the Rust binding to boolean? (Maybe you
envision a VectoredInterruptSource implementation for that).

> +///
> +/// Interrupts are implemented as a pointer to the interrupt "sink", which has
> +/// type [`IRQState`].  A device exposes its source as a QOM link property 
> using
> +/// a function such as
> +/// [`SysBusDevice::init_irq`](crate::sysbus::SysBusDevice::init_irq), and
> +/// initially leaves the pointer to a NULL value, representing an unconnected
> +/// interrupt. To connect it, whoever creates the device fills the pointer with
> +/// the sink's `IRQState *`, for example using `sysbus_connect_irq`.  Because
> +/// devices are generally shared objects, interrupt sources are an example of
> +/// the interior mutability pattern.
> +///
> +/// Interrupt sources can only be triggered under the Big QEMU Lock; they are
> +/// neither `Send` nor `Sync`.
> +#[derive(Debug)]
> +pub struct InterruptSource(BqlCell<*mut IRQState>);
> +
> +impl InterruptSource {
> +    /// Send a low (`false`) value to the interrupt sink.
> +    pub fn lower(&self) {
> +        self.set(false);
> +    }
> +
> +    /// Send a high-low pulse to the interrupt sink.
> +    pub fn pulse(&self) {
> +        self.set(true);
> +        self.set(false);
> +    }
> +
> +    /// Send a high (`true`) value to the interrupt sink.
> +    pub fn raise(&self) {
> +        self.set(true);
> +    }
> +
> +    /// Send `level` to the interrupt sink.
> +    pub fn set(&self, level: bool) {
> +        unsafe {
> +            qemu_set_irq(self.0.get(), level.into());
> +        }
> +    }
> +
> +    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()))
> +    }
> +}