| Message ID | 20241205060714.256270-4-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Reinvent the wheel for HPET timer in Rust | expand |
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 > > >
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> {
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(+)