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.

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

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

On Thu, Dec 05, 2024 at 04:55:47PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 16:55:47 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 03/13] rust/cell: add get_mut() method for BqlCell
> 
> 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:

Thanks for pointing that out, I really didn't think of that, I
understand how that would break the atomicity of the BQL lock, right?

>    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));
>     }

Yes, it's clearer!

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

Nice idea!

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

Will something like “anything that releases bql_lock” happen here?
There seems to be no atomicity guarantee here.

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

Thanks! I can also add a line of doc for bql_locked that it can be used
directly without BqlCell if necessary.

And if you also agree the Phillipe's idea, I also need to add BqlCell
for fw_cfg field in HPETClass :-).

Regards,
Zhao

Il sab 7 dic 2024, 16:38 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> Thanks for pointing that out, I really didn't think of that, I
> understand how that would break the atomicity of the BQL lock, right?
>

Yes, but also the function seems unnecessary.

> 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 };
>
> Nice idea!
>
> >         if self.count == u8::MAX {
> >             // first instance
> >             fw_cfg.count = 0;
> >         }
>
> Will something like “anything that releases bql_lock” happen here?


No, there are no function calls even.

There seems to be no atomicity guarantee here.
>

It's not beautiful but it's guaranteed to be atomic. For the rare case of
static mut, which is unsafe anyway, it makes sense.

>
> >         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.
>
> Thanks! I can also add a line of doc for bql_locked that it can be used
> directly without BqlCell if necessary.
>

Good idea!

And if you also agree the Phillipe's idea, I also need to add BqlCell
> for fw_cfg field in HPETClass :-).
>

No, that also breaks compilation with CONFIG_HPET=n. The idea is nice but
it doesn't work. ¯⁠\⁠_⁠(⁠ツ⁠)⁠_⁠/⁠¯

Paolo


> Regards,
> Zhao
>
>
>