Paolo Bonzini <[email protected]> writes:
> The InterruptSource bindings let us call qemu_set_irq() and > sysbus_init_irq() as safe code. Apart from this, they are a good starting > point to think more about Rust shared/exclusive reference requirements > and what this means for QEMU. > > Interrupt sources, qemu_irq in C code, are pointers to IRQState objects. > They are QOM link properties and can be written to outside the control > of the device (i.e. from a shared reference); therefore their Rust > representation must be an interior-mutable field. I like the idea of modeling device struct fields as interior-mutable. That reflects how such fields are referenced and can help forbid invalid accesses early. To showcase the benefit of modeling interior-mutable fields, a more complex device example (probably involving coroutines or other async mechanisms) may be needed. > > 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. It does not matter if C code handed you a *mut, such as > in a MemoryRegion or CharBackend callback: Rust disallows altogether > creating mutable references to data that has an active shared reference. > Instead, individual regions of the device must use cell types to make > _parts_ of the device structs mutable. > > Back to interrupt sources, for now this patch uses a Cell, but this is > only an approximation of what is actually going on; a Cell can only > live within one thread, while here the semantics are "accessible by > multiple threads but only under the Big QEMU Lock". It seems to me that > the way to go is for QEMU to provide its own "cell" types that check > locking rules with respect to the "Big QEMU Lock" or to ``AioContext``s. > For example, qemu_api::cell::Cell, which is for Copy types like > std::cell:Cell, would only allow get()/set() under BQL protection and > therefore could be Send/Sync. Likewise, qemu_api::cell::RefCell would be > a RefCell that is Send/Sync, because it checks that borrow()/borrow_mut() > is only done under BQL. To me a container that check locking rules sound more like Lock, not Cell. Name it as a Cell can be misleading to those being used to Rust cells. That said, one of its primary differences from the standard locking types in Rust is that the lock involved is global instead of being bound to a specific object. There are two alternative APIs in my mind: 1. get(&self) -> T / set(&self, T) which internally checks if BQL is held by the current thread, and panics if it is not. This is more straightforward. 2. get(&self, _: &BqlLockGuard) -> T / set(&self, _: T, _: &BqlLockGuard) which takes an extra evidence of BQL being held. BqlLockGuard can only be got as an argument to BQL-protected callbacks (macro-generated glue code here) or Bql::lock() which calls bql_lock(). The second approach looks more idiomatic to me and may allow the compiler to error (via analyzing lifetimes) on derefs of borrow()/borrow_mut()-returned refs after BQL is unlocked (I need a double check on this). However, temporarily unlocking BQL is extremely rare in devices. I'm not sure if that's worthwhile at the cost of making the APIs more tedious to use. What do you think? -- Best Regards Junjie Mao > > Signed-off-by: Paolo Bonzini <[email protected]> > --- > Do people think this is the right way forward for interior > mutability? Should these bindings be committed already with > the FIXME comment, or should qemu_api::cell:Cell be written and > committed first? Should the implementation and use be split > in separate patches or is the diff small enough? > > rust/hw/char/pl011/src/device.rs | 22 +++++++----- > rust/qemu-api/meson.build | 2 ++ > rust/qemu-api/src/irq.rs | 61 ++++++++++++++++++++++++++++++++ > rust/qemu-api/src/lib.rs | 2 ++ > rust/qemu-api/src/sysbus.rs | 26 ++++++++++++++ > 5 files changed, 104 insertions(+), 9 deletions(-) > create mode 100644 rust/qemu-api/src/irq.rs > create mode 100644 rust/qemu-api/src/sysbus.rs >
