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

[Zhao Liu]

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