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

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

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