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