[PATCH 0/2] rust: safe wrappers for interrupt sources

[PATCH 0/2] rust: safe wrappers for interrupt sources

[Paolo Bonzini]

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 their Rust
representation must be an interior-mutable field.

This make interrupt sources similar to a Cell<*mut IRQState>.  However,
a Cell can only live within one thread, while here the semantics are
"accessible by multiple threads but only under the Big QEMU Lock".

Therefore, this series adds to QEMU a specialized cell type that checks
locking rules with respect to the "Big QEMU Lock".  In particular,
qemu_api::cell::BqlCell only allows get()/set() under BQL protection and
therefore is Send/Sync.  It comes with documentation and doctests.
I am not fully satisfied with the solution I used for mocking the BQL;
I have a prototype that runs doctests from "meson test" but that may
take some more time to cook.

Likewise, qemu_api::cell::RefCell would be a RefCell that is Send/Sync,
because it checks that borrow()/borrow_mut() is only done under BQL;
but this is not added here because there is no use case (yet).

The interrupt sources prat was previously posted as RFC at
https://lore.kernel.org/qemu-devel/20241104085159.76841-1-pbonzini@redhat.com/,
while the BqlCell is new.  The code is a bit long but most of it is
lifted from the standard library and almost half is documentation.

Please review!

Paolo Bonzini (2):
  rust: add BQL-enforcing Cell variant
  rust: add bindings for interrupt sources

 rust/hw/char/pl011/src/device.rs |  22 +--
 rust/qemu-api/meson.build        |   3 +
 rust/qemu-api/src/cell.rs        | 294 +++++++++++++++++++++++++++++++
 rust/qemu-api/src/irq.rs         |  66 +++++++
 rust/qemu-api/src/lib.rs         |   3 +
 rust/qemu-api/src/sysbus.rs      |  26 +++
 6 files changed, 404 insertions(+), 10 deletions(-)
 create mode 100644 rust/qemu-api/src/cell.rs
 create mode 100644 rust/qemu-api/src/irq.rs
 create mode 100644 rust/qemu-api/src/sysbus.rs

-- 
2.47.0

[PATCH 1/2] rust: add BQL-enforcing Cell variant

[Paolo Bonzini]

QEMU objects usually have their pointer shared with the "outside
world" very early in their lifetime, for example when they create their
MemoryRegions.  Because at this point it is not valid anymore to
create a &mut reference to the device, individual parts of the
device struct must be made mutable in a controlled manner.

QEMU's Big Lock (BQL) effectively turns multi-threaded code into
single-threaded code while device code runs, as long as the BQL is not
released while the device is borrowed (because C code could sneak in and
mutate the device).  We can then introduce custom interior mutability primitives
that are semantically similar to the standard library's (single-threaded)
Cell and RefCell, but account for QEMU's threading model.  Accessing
the "BqlCell" or borrowing the "BqlRefCell" requires proving that the
BQL is held, and attempting to access without the BQL is a runtime panic,
similar to RefCell's already-borrowed panic.

With respect to naming I also considered omitting the "Bql" prefix or
moving it to the module, e.g.  qemu_api::bql::{Cell, RefCell}.  However,
this could easily lead to mistakes and confusion; for example rustc could
suggest the wrong import, leading to subtle bugs.

As a start introduce the an equivalent of Cell.
Almost all of the code was taken from Rust's standard library, while
removing unstable features and probably-unnecessary functionality that
amounts to 60% of the original code.  A lot of what's left is documentation,
as well as unit tests in the form of doctests.  These are not yet integrated
in "make check" but can be run with "cargo test --doc".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/meson.build |   1 +
 rust/qemu-api/src/cell.rs | 294 ++++++++++++++++++++++++++++++++++++++
 rust/qemu-api/src/lib.rs  |   1 +
 3 files changed, 296 insertions(+)
 create mode 100644 rust/qemu-api/src/cell.rs

diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index d719c13f46d..edc21e1a3f8 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -13,6 +13,7 @@ _qemu_api_rs = static_library(
     [
       'src/lib.rs',
       'src/bindings.rs',
+      'src/cell.rs',
       'src/c_str.rs',
       'src/definitions.rs',
       'src/device_class.rs',
diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
new file mode 100644
index 00000000000..8842d43228b
--- /dev/null
+++ b/rust/qemu-api/src/cell.rs
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: MIT
+//
+// This file is based on library/core/src/cell.rs from
+// Rust 1.82.0.
+//
+// Permission is hereby granted, free of charge, to any
+// person obtaining a copy of this software and associated
+// documentation files (the "Software"), to deal in the
+// Software without restriction, including without
+// limitation the rights to use, copy, modify, merge,
+// publish, distribute, sublicense, and/or sell copies of
+// the Software, and to permit persons to whom the Software
+// is furnished to do so, subject to the following
+// conditions:
+//
+// The above copyright notice and this permission notice
+// shall be included in all copies or substantial portions
+// of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF
+// ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED
+// TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A
+// PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
+// SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
+// CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR
+// IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+// DEALINGS IN THE SOFTWARE.
+
+//! BQL-protected mutable containers.
+//!
+//! Rust memory safety is based on this rule: Given an object `T`, it is only
+//! possible to have one of the following:
+//!
+//! - Having several immutable references (`&T`) to the object (also known as
+//!   **aliasing**).
+//! - Having one mutable reference (`&mut T`) to the object (also known as
+//!   **mutability**).
+//!
+//! This is enforced by the Rust compiler. However, there are situations where
+//! this rule is not flexible enough. Sometimes it is required to have multiple
+//! references to an object and yet mutate it. In particular, QEMU objects
+//! usually have their pointer shared with the "outside world very early in
+//! their lifetime", for example when they create their
+//! [`MemoryRegion`s](crate::bindings::MemoryRegion).  Therefore, individual
+//! parts of a  device must be made mutable in a controlled manner through the
+//! use of cell types.
+//!
+//! This module provides a way to do so via the Big QEMU Lock.  While
+//! [`BQLCell<T>`] is essentially the same single-threaded primitive that is
+//! available in `std::cell`, the BQL allows it to be used from a multi-threaded
+//! context and to share references across threads, while maintaining Rust's
+//! safety guarantees.  For this reason, unlike its `std::cell` counterpart,
+//! `BqlCell` implements the `Sync` trait.
+//!
+//! BQL checks are performed in debug builds but can be optimized away in
+//! release builds, providing runtime safety during development with no overhead
+//! in production.
+//!
+//! Warning: While `BqlCell` is similar to its `std::cell` counterpart, the two
+//! are not interchangeable. Using `std::cell` types in QEMU device
+//! implementations is usually incorrect and can lead to thread-safety issues.
+//!
+//! ## `BqlCell<T>`
+//!
+//! [`BqlCell<T>`] implements interior mutability by moving values in and out of
+//! the cell. That is, an `&mut T` to the inner value can never be obtained as
+//! long as the cell is shared. The value itself cannot be directly obtained
+//! without copying it, cloning it, or replacing it with something else. This
+//! type provides the following methods, all of which can be called only while
+//! the BQL is held:
+//!
+//!  - For types that implement [`Copy`], the [`get`](BqlCell::get) method
+//!    retrieves the current interior value by duplicating it.
+//!  - For types that implement [`Default`], the [`take`](BqlCell::take) method
+//!    replaces the current interior value with [`Default::default()`] and
+//!    returns the replaced value.
+//!  - All types have:
+//!    - [`replace`](BqlCell::replace): replaces the current interior value and
+//!      returns the replaced value.
+//!    - [`set`](BqlCell::set): this method replaces the interior value,
+//!      dropping the replaced value.
+
+use std::{cell::UnsafeCell, cmp::Ordering, fmt, mem};
+
+use crate::bindings;
+
+// TODO: When building doctests do not include the actual BQL, because cargo
+// does not know how to link them to libqemuutil.  This can be fixed by
+// running rustdoc from "meson test" instead of relying on cargo.
+fn bql_locked() -> bool {
+    // SAFETY: the function does nothing but return a thread-local bool
+    !cfg!(MESON) || unsafe { bindings::bql_locked() }
+}
+
+/// A mutable memory location that is protected by the Big QEMU Lock.
+///
+/// # Memory layout
+///
+/// `BqlCell<T>` has the same in-memory representation as its inner type `T`.
+#[repr(transparent)]
+pub struct BqlCell<T> {
+    value: UnsafeCell<T>,
+}
+
+// SAFETY: Same as for std::sync::Mutex.  In the end this *is* a Mutex,
+// except it is stored out-of-line
+unsafe impl<T: Send> Send for BqlCell<T> {}
+unsafe impl<T: Send> Sync for BqlCell<T> {}
+
+impl<T: Copy> Clone for BqlCell<T> {
+    #[inline]
+    fn clone(&self) -> BqlCell<T> {
+        BqlCell::new(self.get())
+    }
+}
+
+impl<T: Default> Default for BqlCell<T> {
+    /// Creates a `BqlCell<T>`, with the `Default` value for T.
+    #[inline]
+    fn default() -> BqlCell<T> {
+        BqlCell::new(Default::default())
+    }
+}
+
+impl<T: PartialEq + Copy> PartialEq for BqlCell<T> {
+    #[inline]
+    fn eq(&self, other: &BqlCell<T>) -> bool {
+        self.get() == other.get()
+    }
+}
+
+impl<T: Eq + Copy> Eq for BqlCell<T> {}
+
+impl<T: PartialOrd + Copy> PartialOrd for BqlCell<T> {
+    #[inline]
+    fn partial_cmp(&self, other: &BqlCell<T>) -> Option<Ordering> {
+        self.get().partial_cmp(&other.get())
+    }
+}
+
+impl<T: Ord + Copy> Ord for BqlCell<T> {
+    #[inline]
+    fn cmp(&self, other: &BqlCell<T>) -> Ordering {
+        self.get().cmp(&other.get())
+    }
+}
+
+impl<T> From<T> for BqlCell<T> {
+    /// Creates a new `BqlCell<T>` containing the given value.
+    fn from(t: T) -> BqlCell<T> {
+        BqlCell::new(t)
+    }
+}
+
+impl<T: fmt::Debug + Copy> fmt::Debug for BqlCell<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.get().fmt(f)
+    }
+}
+
+impl<T: fmt::Display + Copy> fmt::Display for BqlCell<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.get().fmt(f)
+    }
+}
+
+impl<T> BqlCell<T> {
+    /// Creates a new `BqlCell` containing the given value.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use qemu_api::cell::BqlCell;
+    ///
+    /// let c = BqlCell::new(5);
+    /// ```
+    #[inline]
+    pub const fn new(value: T) -> BqlCell<T> {
+        BqlCell {
+            value: UnsafeCell::new(value),
+        }
+    }
+
+    /// Sets the contained value.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use qemu_api::cell::BqlCell;
+    ///
+    /// let c = BqlCell::new(5);
+    ///
+    /// c.set(10);
+    /// ```
+    #[inline]
+    pub fn set(&self, val: T) {
+        self.replace(val);
+    }
+
+    /// Replaces the contained value with `val`, and returns the old contained
+    /// value.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use qemu_api::cell::BqlCell;
+    ///
+    /// let cell = BqlCell::new(5);
+    /// assert_eq!(cell.get(), 5);
+    /// assert_eq!(cell.replace(10), 5);
+    /// assert_eq!(cell.get(), 10);
+    /// ```
+    #[inline]
+    pub fn replace(&self, val: T) -> T {
+        debug_assert!(bql_locked());
+        // SAFETY: This can cause data races if called from a separate thread,
+        // but `BqlCell` is `!Sync` so this won't happen.
+        mem::replace(unsafe { &mut *self.value.get() }, val)
+    }
+
+    /// Unwraps the value, consuming the cell.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use qemu_api::cell::BqlCell;
+    ///
+    /// let c = BqlCell::new(5);
+    /// let five = c.into_inner();
+    ///
+    /// assert_eq!(five, 5);
+    /// ```
+    pub fn into_inner(self) -> T {
+        debug_assert!(bql_locked());
+        self.value.into_inner()
+    }
+}
+
+impl<T: Copy> BqlCell<T> {
+    /// Returns a copy of the contained value.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use qemu_api::cell::BqlCell;
+    ///
+    /// let c = BqlCell::new(5);
+    ///
+    /// let five = c.get();
+    /// ```
+    #[inline]
+    pub fn get(&self) -> T {
+        debug_assert!(bql_locked());
+        unsafe { *self.value.get() }
+    }
+}
+
+impl<T> BqlCell<T> {
+    /// Returns a raw pointer to the underlying data in this cell.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use qemu_api::cell::BqlCell;
+    ///
+    /// let c = BqlCell::new(5);
+    ///
+    /// let ptr = c.as_ptr();
+    /// ```
+    #[inline]
+    pub const fn as_ptr(&self) -> *mut T {
+        self.value.get()
+    }
+}
+
+impl<T: Default> BqlCell<T> {
+    /// Takes the value of the cell, leaving `Default::default()` in its place.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use qemu_api::cell::BqlCell;
+    ///
+    /// let c = BqlCell::new(5);
+    /// let five = c.take();
+    ///
+    /// assert_eq!(five, 5);
+    /// assert_eq!(c.into_inner(), 0);
+    /// ```
+    pub fn take(&self) -> T {
+        self.replace(Default::default())
+    }
+}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 440aff3817d..b04d110b3f5 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -8,6 +8,7 @@
 pub mod bindings;
 
 pub mod c_str;
+pub mod cell;
 pub mod definitions;
 pub mod device_class;
 pub mod offset_of;
-- 
2.47.0

Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Zhao Liu]

On Fri, Nov 22, 2024 at 08:47:55AM +0100, Paolo Bonzini wrote:
> Date: Fri, 22 Nov 2024 08:47:55 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/2] rust: add BQL-enforcing Cell variant
> X-Mailer: git-send-email 2.47.0
> 
> QEMU objects usually have their pointer shared with the "outside
> world" very early in their lifetime, for example when they create their
> MemoryRegions.  Because at this point it is not valid anymore to
> create a &mut reference to the device, individual parts of the
> device struct must be made mutable in a controlled manner.

I try to understand this description with the words in v1 :) :

>> But this actually applies to _all_ of the device struct!  Once a
>> pointer to the device has been handed out (for example via
>> memory_region_init_io or qdev_init_clock_in), accesses to the device
>> struct must not use &mut anymore.

is the final goal to wrap the entire DeviceState into the
BQLRefCell as well?

> QEMU's Big Lock (BQL) effectively turns multi-threaded code into
> single-threaded code while device code runs, as long as the BQL is not
> released while the device is borrowed (because C code could sneak in and
> mutate the device).  We can then introduce custom interior mutability primitives
> that are semantically similar to the standard library's (single-threaded)
> Cell and RefCell, but account for QEMU's threading model.  Accessing
> the "BqlCell" or borrowing the "BqlRefCell" requires proving that the
> BQL is held, and attempting to access without the BQL is a runtime panic,
> similar to RefCell's already-borrowed panic.

This design is very clever and clear!

But I'm a little fuzzy on when to use it. And could you educate me on
whether there are any guidelines for determining which bindings should
be placed in the BQLCell, such as anything that might be shared?

...

> diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
> index d719c13f46d..edc21e1a3f8 100644
> --- a/rust/qemu-api/meson.build
> +++ b/rust/qemu-api/meson.build
> @@ -13,6 +13,7 @@ _qemu_api_rs = static_library(
>      [
>        'src/lib.rs',
>        'src/bindings.rs',
         ^^^^^^^^^^^^^^^^^

need to rebase :-)

> +      'src/cell.rs',
>        'src/c_str.rs',
>        'src/definitions.rs',
>        'src/device_class.rs',
> +        self.get().fmt(f)
> +    }
> +}

...

> +    /// Replaces the contained value with `val`, and returns the old contained
> +    /// value.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use qemu_api::cell::BqlCell;
> +    ///
> +    /// let cell = BqlCell::new(5);
> +    /// assert_eq!(cell.get(), 5);
> +    /// assert_eq!(cell.replace(10), 5);
> +    /// assert_eq!(cell.get(), 10);
> +    /// ```
> +    #[inline]
> +    pub fn replace(&self, val: T) -> T {
> +        debug_assert!(bql_locked());

Could debug_assert() work? IIUC, it requires to pass `-C debug-assertions` to
compiler, but currently we don't support this flag in meson...

...so, should we add a debug option in meson configure?

> +        // SAFETY: This can cause data races if called from a separate thread,
> +        // but `BqlCell` is `!Sync` so this won't happen.
> +        mem::replace(unsafe { &mut *self.value.get() }, val)
> +    }
> +

Regards,
Zhao

Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Paolo Bonzini]

On 11/26/24 15:56, Zhao Liu wrote:
>>> But this actually applies to _all_ of the device struct!  Once a
>>> pointer to the device has been handed out (for example via
>>> memory_region_init_io or qdev_init_clock_in), accesses to the device
>>> struct must not use &mut anymore.
> 
> is the final goal to wrap the entire DeviceState into the
> BQLRefCell as well?

Not all of it, but certainly parts of it.  For example, the
properties are not mutable so they don't need to be in the BqlRefCell. 
The parents (SysBusDevice/DeviceState/Object) also manage their 
mutability on their own.

The registers and FIFO state would be in q BqlRefCell; as an 
approximation I expect that if you migrate a field, it will likely be in 
a BqlRefCell.

For PL011, that would be something like

struct PL011Registers {
     pub flags: registers::Flags,
     pub line_control: registers::LineControl,
     pub receive_status_error_clear: registers::ReceiveStatusErrorClear,
     pub control: registers::Control,
     pub dmacr: u32,
     pub int_enabled: u32,
     pub int_level: u32,
     pub read_fifo: [u32; PL011_FIFO_DEPTH],
     pub ilpr: u32,
     pub ibrd: u32,
     pub fbrd: u32,
     pub ifl: u32,
     pub read_pos: usize,
     pub read_count: usize,
     pub read_trigger: usize,
}

and a single "regs: BqlRefCell<PL011Registers>" in PL011State.

>> QEMU's Big Lock (BQL) effectively turns multi-threaded code into
>> single-threaded code while device code runs, as long as the BQL is not
>> released while the device is borrowed (because C code could sneak in and
>> mutate the device).  We can then introduce custom interior mutability primitives
>> that are semantically similar to the standard library's (single-threaded)
>> Cell and RefCell, but account for QEMU's threading model.  Accessing
>> the "BqlCell" or borrowing the "BqlRefCell" requires proving that the
>> BQL is held, and attempting to access without the BQL is a runtime panic,
>> similar to RefCell's already-borrowed panic.
> 
> This design is very clever and clear!
> 
> But I'm a little fuzzy on when to use it. And could you educate me on
> whether there are any guidelines for determining which bindings should
> be placed in the BQLCell, such as anything that might be shared?

It's the same as normal Rust code.  If in Rust you'd use a Cell or a 
RefCell, use a BqlCell or a BqlRefCell.

Right now it's hard to see it because there are a lot of "bad" casts 
from *mut to &mut.  But once the right bindings are in place, it will be 
a lot clearer.  For example the pl011 receive callback (currently an 
unsafe fn) might look like this:

     pub fn receive(&mut self, buf: [u8]) {
         if self.loopback_enabled() {
             return;
         }
         if !buf.is_empty() {
             debug_assert!(buf.len() == 1);
             self.put_fifo(buf[0].into());
         }
     }

except that it would not compile because the receiver must be &self. 
Hence the need for the BqlRefCell<PL011Registers>, which lets you change 
it to

     pub fn receive(&self, buf: [u8]) {
         let regs = self.regs.borrow_mut();
         if regs.loopback_enabled() {
             return;
         }
         if !buf.is_empty() {
             debug_assert!(buf.len() == 1);
             regs.put_fifo(buf[0].into());
         }
     }

Likewise for the MemoryRegionOps.  Right now you have

     pub fn write(&mut self, offset: hwaddr, value: u64) {
         ...
     }

but it will become

     pub fn write(&self, offset: hwaddr, value: u64) {
         let regs = self.regs.borrow_mut();
         ...
     }

Or who knows---perhaps the body of PL011State::write could become 
PL011Registers::write, and PL011State will do just

     pub fn write(&self, offset: hwaddr, value: u64) {
         self.regs.borrow_mut().write(offset, value);
         self.update()
     }

You can already apply this technique to your HPET port using a "normal" 
RefCell.  You'd lose the BQL assertion check and your object will not be 
Sync/Send (this is technically incorrect, because the code *does* run in 
multiple threads, but with the current state of Rust in QEMU it's not a 
bad option), but apart from this it will work.

However if you have already written a vmstate, you'll have to disable 
the vmstate temporarily because the vmstate macros cannot (yet) accept 
fields within a BqlRefCell.  Personally I believe that disabling vmstate 
and experimenting with interior mutability is a good compromise.

Plus, speaking in general, "it does something in a different way than 
the pl011 device model" is a good reason to merge the HPET model earlier 
too. :)

>> +    #[inline]
>> +    pub fn replace(&self, val: T) -> T {
>> +        debug_assert!(bql_locked());
> 
> Could debug_assert() work? IIUC, it requires to pass `-C debug-assertions` to
> compiler, but currently we don't support this flag in meson...

Meson automatically adds -C debug-assertions unless you configure with 
-Db_ndebug=off, which we never do.  So debug_assert!() is always on in 
QEMU; whether to use it or assert!() is more of a documentation choice.

Paolo

Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Zhao Liu]

On Tue, Nov 26, 2024 at 05:11:36PM +0100, Paolo Bonzini wrote:
> Date: Tue, 26 Nov 2024 17:11:36 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant
> 
> On 11/26/24 15:56, Zhao Liu wrote:
> > > > But this actually applies to _all_ of the device struct!  Once a
> > > > pointer to the device has been handed out (for example via
> > > > memory_region_init_io or qdev_init_clock_in), accesses to the device
> > > > struct must not use &mut anymore.
> > 
> > is the final goal to wrap the entire DeviceState into the
> > BQLRefCell as well?
> 
> Not all of it, but certainly parts of it.  For example, the
> properties are not mutable so they don't need to be in the BqlRefCell. The
> parents (SysBusDevice/DeviceState/Object) also manage their mutability on
> their own.
> 
> The registers and FIFO state would be in q BqlRefCell; as an approximation I
> expect that if you migrate a field, it will likely be in a BqlRefCell.
> 
> For PL011, that would be something like
> 
> struct PL011Registers {
>     pub flags: registers::Flags,
>     pub line_control: registers::LineControl,
>     pub receive_status_error_clear: registers::ReceiveStatusErrorClear,
>     pub control: registers::Control,
>     pub dmacr: u32,
>     pub int_enabled: u32,
>     pub int_level: u32,
>     pub read_fifo: [u32; PL011_FIFO_DEPTH],
>     pub ilpr: u32,
>     pub ibrd: u32,
>     pub fbrd: u32,
>     pub ifl: u32,
>     pub read_pos: usize,
>     pub read_count: usize,
>     pub read_trigger: usize,
> }
> 
> and a single "regs: BqlRefCell<PL011Registers>" in PL011State.

Here I have a possibly common question about the choice of BqlCell and
future BqlRefCell. Pls refer my comment below...

> > > QEMU's Big Lock (BQL) effectively turns multi-threaded code into
> > > single-threaded code while device code runs, as long as the BQL is not
> > > released while the device is borrowed (because C code could sneak in and
> > > mutate the device).  We can then introduce custom interior mutability primitives
> > > that are semantically similar to the standard library's (single-threaded)
> > > Cell and RefCell, but account for QEMU's threading model.  Accessing
> > > the "BqlCell" or borrowing the "BqlRefCell" requires proving that the
> > > BQL is held, and attempting to access without the BQL is a runtime panic,
> > > similar to RefCell's already-borrowed panic.
> > 
> > This design is very clever and clear!
> > 
> > But I'm a little fuzzy on when to use it. And could you educate me on
> > whether there are any guidelines for determining which bindings should
> > be placed in the BQLCell, such as anything that might be shared?
> 
> It's the same as normal Rust code.  If in Rust you'd use a Cell or a
> RefCell, use a BqlCell or a BqlRefCell.
> 
> Right now it's hard to see it because there are a lot of "bad" casts from
> *mut to &mut.  But once the right bindings are in place, it will be a lot
> clearer.  For example the pl011 receive callback (currently an unsafe fn)
> might look like this:
> 
>     pub fn receive(&mut self, buf: [u8]) {
>         if self.loopback_enabled() {
>             return;
>         }
>         if !buf.is_empty() {
>             debug_assert!(buf.len() == 1);
>             self.put_fifo(buf[0].into());
>         }
>     }
> 
> except that it would not compile because the receiver must be &self. Hence
> the need for the BqlRefCell<PL011Registers>, which lets you change it to
> 
>     pub fn receive(&self, buf: [u8]) {
>         let regs = self.regs.borrow_mut();
>         if regs.loopback_enabled() {
>             return;
>         }
>         if !buf.is_empty() {
>             debug_assert!(buf.len() == 1);
>             regs.put_fifo(buf[0].into());
>         }
>     }
> 
> Likewise for the MemoryRegionOps.  Right now you have
> 
>     pub fn write(&mut self, offset: hwaddr, value: u64) {
>         ...
>     }
> 
> but it will become
> 
>     pub fn write(&self, offset: hwaddr, value: u64) {
>         let regs = self.regs.borrow_mut();
>         ...
>     }

I understand we need BqlRefCell instead of BqlCell for registers since
there may be more than one reference, e.g., callbacks from CharBackend
and MemoryRegion may hold multiple references at the same time, right?

If right, like HPET, with only MemoryRegion callbacks, reads and writes
I guess which should not be able to happen at the same time, so BqlCell
is also feasible, as is Irq?

(This piece of the thread model is a bit more complex, and I'm fully
aware that the right TYPE relies a lot on understanding the underlying
mechanism, which I'm not yet familiar enough with :-) ).

However, all in all, using BqlRefCell for register fields looks to be
always better than BqlCell.

> Or who knows---perhaps the body of PL011State::write could become
> PL011Registers::write, and PL011State will do just
> 
>     pub fn write(&self, offset: hwaddr, value: u64) {
>         self.regs.borrow_mut().write(offset, value);
>         self.update()
>     }
> 
> You can already apply this technique to your HPET port using a "normal"
> RefCell.  You'd lose the BQL assertion check and your object will not be
> Sync/Send (this is technically incorrect, because the code *does* run in
> multiple threads, but with the current state of Rust in QEMU it's not a bad
> option), but apart from this it will work.

Thank you! I'll change the current code to support this. Instead of
implementing a register space like PL011, I continue to handle registers
with u64 variables and bit macro definitions like the C version. Also
related to the above question, I'm a bit hesitant to use BqlCell directly
or RefCell for such u64 fields...

> However if you have already written a vmstate, you'll have to disable the
> vmstate temporarily because the vmstate macros cannot (yet) accept fields
> within a BqlRefCell.  Personally I believe that disabling vmstate and
> experimenting with interior mutability is a good compromise.

Sure, I'll drop current VMState support.

> Plus, speaking in general, "it does something in a different way than the
> pl011 device model" is a good reason to merge the HPET model earlier too. :)

There must be a lot more opens :-(, such as the memattrs/timer binding,
which I hope to discuss with you at the later RFC!

> > > +    #[inline]
> > > +    pub fn replace(&self, val: T) -> T {
> > > +        debug_assert!(bql_locked());
> > 
> > Could debug_assert() work? IIUC, it requires to pass `-C debug-assertions` to
> > compiler, but currently we don't support this flag in meson...
> 
> Meson automatically adds -C debug-assertions unless you configure with
> -Db_ndebug=off, which we never do.  So debug_assert!() is always on in QEMU;
> whether to use it or assert!() is more of a documentation choice.

Thank you! I see.

Regards,
Zhao

Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]

Il mer 27 nov 2024, 07:17 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> > and a single "regs: BqlRefCell<PL011Registers>" in PL011State.
>
> Here I have a possibly common question about the choice of BqlCell and
> future BqlRefCell. Pls refer my comment below...
>
> I understand we need BqlRefCell instead of BqlCell for registers since
> there may be more than one reference, e.g., callbacks from CharBackend
> and MemoryRegion may hold multiple references at the same time, right?
>
> If right, like HPET, with only MemoryRegion callbacks, reads and writes
> I guess which should not be able to happen at the same time, so BqlCell
> is also feasible, as is Irq?
>

Choose between BqlCell/BqlRefCell like you would choose between
Cell/RefCell. They result in different code so pick what looks better.

That said for HPET you have the array of timers structs, and these structs
are not Copy, and therefore BqlRefCell is almost the only choice for that
array. With BqlRefCell used for the timers you might as well put the other
registers in the same BqlRefCell.

(This piece of the thread model is a bit more complex, and I'm fully
> aware that the right TYPE relies a lot on understanding the underlying
> mechanism, which I'm not yet familiar enough with :-) ).
>

It's not too complex—the point is to make it as similar as possible to
normal single threaded Rust.

However, all in all, using BqlRefCell for register fields looks to be
> always better than BqlCell.
>

Yep :)

Thank you! I'll change the current code to support this. Instead of
> implementing a register space like PL011, I continue to handle registers
> with u64 variables and bit macro definitions like the C version


That's fine. Experimenting in different directions makes it easier for
future developers to evaluate the tradeoffs.

Also related to the above question, I'm a bit hesitant to use BqlCell
> directly
> or RefCell for such u64 fields...
>

Do whatever looks best to you!

> Plus, speaking in general, "it does something in a different way than the
> > pl011 device model" is a good reason to merge the HPET model earlier
> too. :)
>
> There must be a lot more opens :-(, such as the memattrs/timer binding,
> which I hope to discuss with you at the later RFC!
>

Yes, but stuff like "correctly uses interior mutability" is a very good
reason to include the HPET early.

I will send the next version and include BqlRefCell once I incorporate
Junjie's feedback, in the meanwhile I will send you the BqlRefCell patch
off list.

Paolo

[-- Attachment #2: Type: text/html, Size: 4422 bytes --]

Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> QEMU objects usually have their pointer shared with the "outside
> world" very early in their lifetime, for example when they create their
> MemoryRegions.  Because at this point it is not valid anymore to
> create a &mut reference to the device, individual parts of the
> device struct must be made mutable in a controlled manner.
>
> QEMU's Big Lock (BQL) effectively turns multi-threaded code into
> single-threaded code while device code runs, as long as the BQL is not
> released while the device is borrowed (because C code could sneak in and
> mutate the device).  We can then introduce custom interior mutability primitives
> that are semantically similar to the standard library's (single-threaded)
> Cell and RefCell, but account for QEMU's threading model.  Accessing
> the "BqlCell" or borrowing the "BqlRefCell" requires proving that the
> BQL is held, and attempting to access without the BQL is a runtime panic,
> similar to RefCell's already-borrowed panic.

I like the idea!

>
> With respect to naming I also considered omitting the "Bql" prefix or
> moving it to the module, e.g.  qemu_api::bql::{Cell, RefCell}.  However,
> this could easily lead to mistakes and confusion; for example rustc could
> suggest the wrong import, leading to subtle bugs.
>
> As a start introduce the an equivalent of Cell.
> Almost all of the code was taken from Rust's standard library, while
> removing unstable features and probably-unnecessary functionality that
> amounts to 60% of the original code.  A lot of what's left is documentation,
> as well as unit tests in the form of doctests.  These are not yet integrated
> in "make check" but can be run with "cargo test --doc".
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/meson.build |   1 +
>  rust/qemu-api/src/cell.rs | 294 ++++++++++++++++++++++++++++++++++++++
>  rust/qemu-api/src/lib.rs  |   1 +
>  3 files changed, 296 insertions(+)
>  create mode 100644 rust/qemu-api/src/cell.rs
[snip]
> diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
> new file mode 100644
> index 00000000000..8842d43228b
> --- /dev/null
> +++ b/rust/qemu-api/src/cell.rs
> @@ -0,0 +1,294 @@
[snip]
> +//! BQL-protected mutable containers.
> +//!
> +//! Rust memory safety is based on this rule: Given an object `T`, it is only
> +//! possible to have one of the following:
> +//!
> +//! - Having several immutable references (`&T`) to the object (also known as
> +//!   **aliasing**).
> +//! - Having one mutable reference (`&mut T`) to the object (also known as
> +//!   **mutability**).
> +//!
> +//! This is enforced by the Rust compiler. However, there are situations where
> +//! this rule is not flexible enough. Sometimes it is required to have multiple
> +//! references to an object and yet mutate it. In particular, QEMU objects
> +//! usually have their pointer shared with the "outside world very early in
> +//! their lifetime", for example when they create their
> +//! [`MemoryRegion`s](crate::bindings::MemoryRegion).  Therefore, individual
> +//! parts of a  device must be made mutable in a controlled manner through the
> +//! use of cell types.
> +//!
> +//! This module provides a way to do so via the Big QEMU Lock.  While
> +//! [`BQLCell<T>`] is essentially the same single-threaded primitive that is
> +//! available in `std::cell`, the BQL allows it to be used from a multi-threaded
> +//! context and to share references across threads, while maintaining Rust's
> +//! safety guarantees.  For this reason, unlike its `std::cell` counterpart,
> +//! `BqlCell` implements the `Sync` trait.
> +//!
> +//! BQL checks are performed in debug builds but can be optimized away in
> +//! release builds, providing runtime safety during development with no overhead
> +//! in production.

BQL checks are essential for data race prevention. Skipping them in
release builds would require thorough testing for the same level of
confidence. That does not look to me an idiomatic approach in
Rust. Today we have "-C debug-assertions" which makes debug_assert!
effectively equivalent to assert!, but that may change in the future
when someone wants the rust build system to respect meson buildtype.

[snip]

> +    /// Replaces the contained value with `val`, and returns the old contained
> +    /// value.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use qemu_api::cell::BqlCell;
> +    ///
> +    /// let cell = BqlCell::new(5);
> +    /// assert_eq!(cell.get(), 5);
> +    /// assert_eq!(cell.replace(10), 5);
> +    /// assert_eq!(cell.get(), 10);
> +    /// ```
> +    #[inline]
> +    pub fn replace(&self, val: T) -> T {
> +        debug_assert!(bql_locked());
> +        // SAFETY: This can cause data races if called from a separate thread,
> +        // but `BqlCell` is `!Sync` so this won't happen.

This comment looks out-of-date. BqlCell<T> can be Sync but data race
won't happen because of the preceding BQL check.

--
Best Regards
Junjie Mao

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

[Paolo Bonzini]

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

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

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

[Paolo Bonzini]

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

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

[Philippe Mathieu-Daudé]

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
> 

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

[Paolo Bonzini]

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

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

[Philippe Mathieu-Daudé]

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

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

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