Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Zhao Liu]

On Tue, Nov 26, 2024 at 05:11:36PM +0100, Paolo Bonzini wrote:
> Date: Tue, 26 Nov 2024 17:11:36 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant
> 
> On 11/26/24 15:56, Zhao Liu wrote:
> > > > But this actually applies to _all_ of the device struct!  Once a
> > > > pointer to the device has been handed out (for example via
> > > > memory_region_init_io or qdev_init_clock_in), accesses to the device
> > > > struct must not use &mut anymore.
> > 
> > is the final goal to wrap the entire DeviceState into the
> > BQLRefCell as well?
> 
> Not all of it, but certainly parts of it.  For example, the
> properties are not mutable so they don't need to be in the BqlRefCell. The
> parents (SysBusDevice/DeviceState/Object) also manage their mutability on
> their own.
> 
> The registers and FIFO state would be in q BqlRefCell; as an approximation I
> expect that if you migrate a field, it will likely be in a BqlRefCell.
> 
> For PL011, that would be something like
> 
> struct PL011Registers {
>     pub flags: registers::Flags,
>     pub line_control: registers::LineControl,
>     pub receive_status_error_clear: registers::ReceiveStatusErrorClear,
>     pub control: registers::Control,
>     pub dmacr: u32,
>     pub int_enabled: u32,
>     pub int_level: u32,
>     pub read_fifo: [u32; PL011_FIFO_DEPTH],
>     pub ilpr: u32,
>     pub ibrd: u32,
>     pub fbrd: u32,
>     pub ifl: u32,
>     pub read_pos: usize,
>     pub read_count: usize,
>     pub read_trigger: usize,
> }
> 
> and a single "regs: BqlRefCell<PL011Registers>" in PL011State.

Here I have a possibly common question about the choice of BqlCell and
future BqlRefCell. Pls refer my comment below...

> > > QEMU's Big Lock (BQL) effectively turns multi-threaded code into
> > > single-threaded code while device code runs, as long as the BQL is not
> > > released while the device is borrowed (because C code could sneak in and
> > > mutate the device).  We can then introduce custom interior mutability 
> > > primitives
> > > that are semantically similar to the standard library's (single-threaded)
> > > Cell and RefCell, but account for QEMU's threading model.  Accessing
> > > the "BqlCell" or borrowing the "BqlRefCell" requires proving that the
> > > BQL is held, and attempting to access without the BQL is a runtime panic,
> > > similar to RefCell's already-borrowed panic.
> > 
> > This design is very clever and clear!
> > 
> > But I'm a little fuzzy on when to use it. And could you educate me on
> > whether there are any guidelines for determining which bindings should
> > be placed in the BQLCell, such as anything that might be shared?
> 
> It's the same as normal Rust code.  If in Rust you'd use a Cell or a
> RefCell, use a BqlCell or a BqlRefCell.
> 
> Right now it's hard to see it because there are a lot of "bad" casts from
> *mut to &mut.  But once the right bindings are in place, it will be a lot
> clearer.  For example the pl011 receive callback (currently an unsafe fn)
> might look like this:
> 
>     pub fn receive(&mut self, buf: [u8]) {
>         if self.loopback_enabled() {
>             return;
>         }
>         if !buf.is_empty() {
>             debug_assert!(buf.len() == 1);
>             self.put_fifo(buf[0].into());
>         }
>     }
> 
> except that it would not compile because the receiver must be &self. Hence
> the need for the BqlRefCell<PL011Registers>, which lets you change it to
> 
>     pub fn receive(&self, buf: [u8]) {
>         let regs = self.regs.borrow_mut();
>         if regs.loopback_enabled() {
>             return;
>         }
>         if !buf.is_empty() {
>             debug_assert!(buf.len() == 1);
>             regs.put_fifo(buf[0].into());
>         }
>     }
> 
> Likewise for the MemoryRegionOps.  Right now you have
> 
>     pub fn write(&mut self, offset: hwaddr, value: u64) {
>         ...
>     }
> 
> but it will become
> 
>     pub fn write(&self, offset: hwaddr, value: u64) {
>         let regs = self.regs.borrow_mut();
>         ...
>     }

I understand we need BqlRefCell instead of BqlCell for registers since
there may be more than one reference, e.g., callbacks from CharBackend
and MemoryRegion may hold multiple references at the same time, right?

If right, like HPET, with only MemoryRegion callbacks, reads and writes
I guess which should not be able to happen at the same time, so BqlCell
is also feasible, as is Irq?

(This piece of the thread model is a bit more complex, and I'm fully
aware that the right TYPE relies a lot on understanding the underlying
mechanism, which I'm not yet familiar enough with :-) ).

However, all in all, using BqlRefCell for register fields looks to be
always better than BqlCell.

> Or who knows---perhaps the body of PL011State::write could become
> PL011Registers::write, and PL011State will do just
> 
>     pub fn write(&self, offset: hwaddr, value: u64) {
>         self.regs.borrow_mut().write(offset, value);
>         self.update()
>     }
> 
> You can already apply this technique to your HPET port using a "normal"
> RefCell.  You'd lose the BQL assertion check and your object will not be
> Sync/Send (this is technically incorrect, because the code *does* run in
> multiple threads, but with the current state of Rust in QEMU it's not a bad
> option), but apart from this it will work.

Thank you! I'll change the current code to support this. Instead of
implementing a register space like PL011, I continue to handle registers
with u64 variables and bit macro definitions like the C version. Also
related to the above question, I'm a bit hesitant to use BqlCell directly
or RefCell for such u64 fields...

> However if you have already written a vmstate, you'll have to disable the
> vmstate temporarily because the vmstate macros cannot (yet) accept fields
> within a BqlRefCell.  Personally I believe that disabling vmstate and
> experimenting with interior mutability is a good compromise.

Sure, I'll drop current VMState support.

> Plus, speaking in general, "it does something in a different way than the
> pl011 device model" is a good reason to merge the HPET model earlier too. :)

There must be a lot more opens :-(, such as the memattrs/timer binding,
which I hope to discuss with you at the later RFC!

> > > +    #[inline]
> > > +    pub fn replace(&self, val: T) -> T {
> > > +        debug_assert!(bql_locked());
> > 
> > Could debug_assert() work? IIUC, it requires to pass `-C debug-assertions` 
> > to
> > compiler, but currently we don't support this flag in meson...
> 
> Meson automatically adds -C debug-assertions unless you configure with
> -Db_ndebug=off, which we never do.  So debug_assert!() is always on in QEMU;
> whether to use it or assert!() is more of a documentation choice.

Thank you! I see.

Regards,
Zhao