Re: [RFC 03/13] rust/cell: add get_mut() method for BqlCell

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/5/24 07:07, Zhao Liu wrote:
> The get_mut() is useful when doing compound assignment operations, e.g.,
> *c.get_mut() += 1.
> 
> Implement get_mut() for BqlCell by referring to Cell.

I think you can't do this because the BQL might be released while the owner has a &mut.  Like:

    let mtx = Mutex<BqlCell<u32>>::new();
    let guard = mtx.lock();
    let cell = &mut *guard;
    let inner = cell.get_mut(cell);
    // anything that releases bql_lock
    *inner += 1;

On the other hand I don't think you need it.  You have just two uses.

First, this one:

+        if set && self.is_int_level_triggered() {
+            // If Timer N Interrupt Enable bit is 0, "the timer will
+            // still operate and generate appropriate status bits, but
+            // will not cause an interrupt"
+            *self.get_state_mut().int_status.get_mut() |= mask;
+        } else {
+            *self.get_state_mut().int_status.get_mut() &= !mask;
+        }

Where you can just write

     self.get_state_ref().update_int_status(self.index,
         set && self.is_int_level_triggered())

and the HPETState can do something like

     fn update_int_status(&self, index: u32, level: bool) {
         self.int_status.set(deposit64(self.int_status.get(), bit, 1, level as u64));
     }

For hpet_fw_cfg you have unsafe in the device and it's better if you do:

-        self.hpet_id.set(unsafe { hpet_fw_cfg.assign_hpet_id() });
+        self.hpet_id.set(fw_cfg_config::assign_hpet_id());

with methods like this that do the unsafe access:

impl fw_cfg_config {
     pub(crate) fn assign_hpet_id() -> usize {
         assert!(bql_locked());
         // SAFETY: all accesses go through these methods, which guarantee
         // that the accesses are protected by the BQL.
         let fw_cfg = unsafe { &mut *hpet_fw_cfg };

         if self.count == u8::MAX {
             // first instance
             fw_cfg.count = 0;
         }

         if fw_cfg.count == 8 {
             // TODO: Add error binding: error_setg()
             panic!("Only 8 instances of HPET is allowed");
         }

         let id: usize = fw_cfg.count.into();
         fw_cfg.count += 1;
         id
     }
}

and you can assert bql_locked by hand instead of using the BqlCell.

Paolo

> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   rust/qemu-api/src/cell.rs | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
> index 07b636f26266..95f1cc0b3eb5 100644
> --- a/rust/qemu-api/src/cell.rs
> +++ b/rust/qemu-api/src/cell.rs
> @@ -324,6 +324,31 @@ impl<T> BqlCell<T> {
>       pub const fn as_ptr(&self) -> *mut T {
>           self.value.get()
>       }
> +
> +    /// Returns a mutable reference to the underlying data.
> +    ///
> +    /// This call borrows `BqlCell` mutably (at compile-time) which guarantees
> +    /// that we possess the only reference.
> +    ///
> +    /// However be cautious: this method expects `self` to be mutable, which is
> +    /// generally not the case when using a `BqlCell`. If you require interior
> +    /// mutability by reference, consider using `BqlRefCell` which provides
> +    /// run-time checked mutable borrows through its [`borrow_mut`] method.
> +    ///
> +    /// [`borrow_mut`]: BqlRefCell::borrow_mut()
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use qemu_api::cell::BqlCell;;
> +    ///
> +    /// let mut c = BqlCell::new(5);
> +    /// *c.get_mut() += 1;
> +    ///
> +    /// assert_eq!(c.get(), 6);
> +    pub fn get_mut(&mut self) -> &mut T {
> +        self.value.get_mut()
> +    }
>   }
>   
>   impl<T: Default> BqlCell<T> {