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/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index e582a31e4d3..7e57634bba0 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -13,6 +13,7 @@
     c_str,
     definitions::ObjectImpl,
     device_class::TYPE_SYS_BUS_DEVICE,
+    irq::InterruptSource,
 };
 
 use crate::{
@@ -94,7 +95,7 @@ pub struct PL011State {
     ///  * sysbus IRQ 5: `UARTEINTR` (error interrupt line)
     /// ```
     #[doc(alias = "irq")]
-    pub interrupts: [qemu_irq; 6usize],
+    pub interrupts: [InterruptSource; IRQMASK.len()],
     #[doc(alias = "clk")]
     pub clock: NonNull<Clock>,
     #[doc(alias = "migrate_clk")]
@@ -139,7 +140,8 @@ impl PL011State {
     unsafe fn init(&mut self) {
         const CLK_NAME: &CStr = c_str!("clk");
 
-        let dev = addr_of_mut!(*self).cast::<DeviceState>();
+        let sbd = unsafe { &mut *(addr_of_mut!(*self).cast::<SysBusDevice>()) };
+
         // SAFETY:
         //
         // self and self.iomem are guaranteed to be valid at this point since callers
@@ -153,12 +155,15 @@ unsafe fn init(&mut self) {
                 Self::TYPE_INFO.name,
                 0x1000,
             );
-            let sbd = addr_of_mut!(*self).cast::<SysBusDevice>();
             sysbus_init_mmio(sbd, addr_of_mut!(self.iomem));
-            for irq in self.interrupts.iter_mut() {
-                sysbus_init_irq(sbd, irq);
-            }
         }
+
+        for irq in self.interrupts.iter() {
+            sbd.init_irq(irq);
+        }
+
+        let dev = addr_of_mut!(*self).cast::<DeviceState>();
+
         // SAFETY:
         //
         // self.clock is not initialized at this point; but since `NonNull<_>` is Copy,
@@ -498,10 +503,7 @@ pub fn put_fifo(&mut self, value: c_uint) {
     pub fn update(&self) {
         let flags = self.int_level & self.int_enabled;
         for (irq, i) in self.interrupts.iter().zip(IRQMASK) {
-            // SAFETY: self.interrupts have been initialized in init().
-            unsafe {
-                qemu_set_irq(*irq, i32::from(flags & i != 0));
-            }
+            irq.set(flags & i != 0);
         }
     }
 
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index edc21e1a3f8..973cfbcfb4a 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -17,7 +17,9 @@ _qemu_api_rs = static_library(
       'src/c_str.rs',
       'src/definitions.rs',
       'src/device_class.rs',
+      'src/irq.rs',
       'src/offset_of.rs',
+      'src/sysbus.rs',
       'src/vmstate.rs',
       'src/zeroable.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.
+///
+/// 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()))
+    }
+}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index b04d110b3f5..aa692939688 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -11,7 +11,9 @@
 pub mod cell;
 pub mod definitions;
 pub mod device_class;
+pub mod irq;
 pub mod offset_of;
+pub mod sysbus;
 pub mod vmstate;
 pub mod zeroable;
 
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
new file mode 100644
index 00000000000..1a9b8a1f971
--- /dev/null
+++ b/rust/qemu-api/src/sysbus.rs
@@ -0,0 +1,26 @@
+// Copyright 2024 Red Hat, Inc.
+// Author(s): Paolo Bonzini <pbonzini@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use std::ptr::addr_of;
+
+pub use bindings::{SysBusDevice, SysBusDeviceClass};
+
+use crate::{bindings, irq::InterruptSource};
+
+impl SysBusDevice {
+    /// Return `self` cast to a mutable pointer, for use in calls to C code.
+    const fn as_mut_ptr(&self) -> *mut SysBusDevice {
+        addr_of!(*self) as *mut _
+    }
+
+    /// Expose an interrupt source outside the device as a qdev GPIO output.
+    /// Note that the ordering of calls to `init_irq` is important, since
+    /// whoever creates the sysbus device will refer to the interrupts with
+    /// a number that corresponds to the order of calls to `init_irq`.
+    pub fn init_irq(&self, irq: &InterruptSource) {
+        unsafe {
+            bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr());
+        }
+    }
+}
-- 
2.47.0

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

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

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()))
> +    }
> +}

> > +/// 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).

Yeah, I mean that raise uses true (or 1 :)) and lower uses false.
an example?

> Is this deliberate to restrict the Rust binding to boolean? (Maybe you
> envision a VectoredInterruptSource implementation for that).

No, I simply wasn't aware of that.  I'll adjust; do you have
an example?

> > +/// Interrupt sources can only be triggered under the Big QEMU Lock; they are
> > +/// neither `Send` nor `Sync`.

Oops, this is incorrect.  BqlCell *is* Send/Sync, but checks the
BQL state at run-time.

Paolo

On 22/11/24 09:32, Paolo Bonzini wrote:
>>> +/// 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).
> 
> Yeah, I mean that raise uses true (or 1 :)) and lower uses false.
> an example?

I was thinking of an active-low line where you want to pulse 1 -> 0.
Just chiming in, not to worry about.

> 
>> Is this deliberate to restrict the Rust binding to boolean? (Maybe you
>> envision a VectoredInterruptSource implementation for that).
> 
> No, I simply wasn't aware of that.  I'll adjust; do you have
> an example?

I am having hard time to find one, in particular because I
removed one in c264c074d8 ("hw/intc: Remove TYPE_ETRAX_FS_PIC device"):

-static void pic_update(struct etrax_pic *fs)
-{
-    uint32_t vector = 0;
-    int i;
-
-    fs->regs[R_R_MASKED_VECT] = fs->regs[R_R_VECT] & fs->regs[R_RW_MASK];
-
-    /* The ETRAX interrupt controller signals interrupts to the core
-       through an interrupt request wire and an irq vector bus. If
-       multiple interrupts are simultaneously active it chooses vector
-       0x30 and lets the sw choose the priorities.  */
-    if (fs->regs[R_R_MASKED_VECT]) {
-        uint32_t mv = fs->regs[R_R_MASKED_VECT];
-        for (i = 0; i < 31; i++) {
-            if (mv & 1) {
-                vector = 0x31 + i;
-                /* Check for multiple interrupts.  */
-                if (mv > 1)
-                    vector = 0x30;
-                break;
-            }
-            mv >>= 1;
-        }
-    }
-
-    qemu_set_irq(fs->parent_irq, vector);
-}

See Peter's comment in 
https://lore.kernel.org/qemu-devel/CAFEAcA9cObnb11cSS_StbSHdP0aB6sDeqSHfjb3-qRBfy7K9Kw@mail.gmail.com/

>>> +/// Interrupt sources can only be triggered under the Big QEMU Lock; they are
>>> +/// neither `Send` nor `Sync`.
> 
> Oops, this is incorrect.  BqlCell *is* Send/Sync, but checks the
> BQL state at run-time.
> 
> Paolo
>

On 11/22/24 11:30, Philippe Mathieu-Daudé wrote:
> On 22/11/24 09:32, Paolo Bonzini wrote:
>>>> +/// 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).
>>
>> Yeah, I mean that raise uses true (or 1 :)) and lower uses false.
>> an example?
> 
> I was thinking of an active-low line where you want to pulse 1 -> 0.
> Just chiming in, not to worry about.

This is not happening at the device level, so I assume that such a line 
would not use raise/lower.  Rather, the board (which is on the interrupt 
sink side) would install a qemu_irq_invert() between the device and the 
interrupt controller or GPIO controller.

>>> Is this deliberate to restrict the Rust binding to boolean? (Maybe you
>>> envision a VectoredInterruptSource implementation for that).
>>
>> No, I simply wasn't aware of that.  I'll adjust; do you have
>> an example?
> 
> I am having hard time to find one, in particular because I
> removed one in c264c074d8 ("hw/intc: Remove TYPE_ETRAX_FS_PIC device"):

Ok, then we could put the type as a generic parameter, and use that 
parameter in InterruptSource::set().

pub struct InterruptSource<T = bool> where u32: From<T> {
     inner: BqlCell<*mut IrqState>,

     // this is only needed top ensure that T appears somehow in the
     // struct.  Random Rust type theory stuff. :)
     _marker: PhantomData<fn(&Self, T)>,
}

...

/// Send `level` to the interrupt sink.
pub fn set(&self, level: T) {
     let ptr = self.0.get();
     // SAFETY: the pointer is retrieved under the BQL and remains valid
     // until the BQL is released, which is after qemu_set_irq() is entered.
     unsafe {
         qemu_set_irq(ptr, level.into());
     }
}

and then only implement raise/lower/pulse for InterruptSource<bool>.

This is backwards compatible so we can do it either now, or later when 
needs arises.  You tell me. :)

Paolo

> See Peter's comment in https://lore.kernel.org/qemu-devel/ 
> CAFEAcA9cObnb11cSS_StbSHdP0aB6sDeqSHfjb3-qRBfy7K9Kw@mail.gmail.com/
> 
>>>> +/// Interrupt sources can only be triggered under the Big QEMU 
>>>> Lock; they are
>>>> +/// neither `Send` nor `Sync`.
>>
>> Oops, this is incorrect.  BqlCell *is* Send/Sync, but checks the
>> BQL state at run-time.
>>
>> Paolo
>>
> 
> 
> 

On 22/11/24 11:53, Paolo Bonzini wrote:
> On 11/22/24 11:30, Philippe Mathieu-Daudé wrote:
>> On 22/11/24 09:32, Paolo Bonzini wrote:
>>>>> +/// 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).
>>>
>>> Yeah, I mean that raise uses true (or 1 :)) and lower uses false.
>>> an example?
>>
>> I was thinking of an active-low line where you want to pulse 1 -> 0.
>> Just chiming in, not to worry about.
> 
> This is not happening at the device level, so I assume that such a line 
> would not use raise/lower.  Rather, the board (which is on the interrupt 
> sink side) would install a qemu_irq_invert() between the device and the 
> interrupt controller or GPIO controller.
> 
>>>> Is this deliberate to restrict the Rust binding to boolean? (Maybe you
>>>> envision a VectoredInterruptSource implementation for that).
>>>
>>> No, I simply wasn't aware of that.  I'll adjust; do you have
>>> an example?
>>
>> I am having hard time to find one, in particular because I
>> removed one in c264c074d8 ("hw/intc: Remove TYPE_ETRAX_FS_PIC device"):
> 
> Ok, then we could put the type as a generic parameter, and use that 
> parameter in InterruptSource::set().
> 
> pub struct InterruptSource<T = bool> where u32: From<T> {
>      inner: BqlCell<*mut IrqState>,
> 
>      // this is only needed top ensure that T appears somehow in the
>      // struct.  Random Rust type theory stuff. :)
>      _marker: PhantomData<fn(&Self, T)>,
> }
> 
> ...
> 
> /// Send `level` to the interrupt sink.
> pub fn set(&self, level: T) {
>      let ptr = self.0.get();
>      // SAFETY: the pointer is retrieved under the BQL and remains valid
>      // until the BQL is released, which is after qemu_set_irq() is 
> entered.
>      unsafe {
>          qemu_set_irq(ptr, level.into());
>      }
> }
> 
> and then only implement raise/lower/pulse for InterruptSource<bool>.
> 
> This is backwards compatible so we can do it either now, or later when 
> needs arises.  You tell me. :)

If there are no more vector uses, personally I'd convert qemu_set_irq()
to use an explicit boolean level. If vector need arises then I'd
add it using a new explicit method, i.e. qemu_set_irq_vector(); so
the arguments are obvious when we review qemu_set_irq*() uses.

Otherwise I'll defer to Peter who raised that point first.

> 
> Paolo
> 
>> See Peter's comment in https://lore.kernel.org/qemu-devel/ 
>> CAFEAcA9cObnb11cSS_StbSHdP0aB6sDeqSHfjb3-qRBfy7K9Kw@mail.gmail.com/
>>
>>>>> +/// Interrupt sources can only be triggered under the Big QEMU 
>>>>> Lock; they are
>>>>> +/// neither `Send` nor `Sync`.
>>>
>>> Oops, this is incorrect.  BqlCell *is* Send/Sync, but checks the
>>> BQL state at run-time.
>>>
>>> Paolo
>>>
>>
>>
>>
>