(Resend as the previous email may have failed.) > Remaining unsafe code > ''''''''''''''''''''' > > qdev bindings cover basic classes and interfaces, including > GPIO pins, timers, clocks and MemoryRegionOps. VMState > still needs unsafe callbacks for pre_save/post_load, with > the final version waiting for a bump of the minimum supported > Rust version to 1.83.0. > > Apart from VMState, the remaining instances of `unsafe` blocks in the > pl011 and HPET code can all be removed without bumping the language > version. > > HPET does some very simple memory accesses; a good safe solution > for this may be the ``vm-memory`` crate. While I have not looked into > using it, ``vm-memory`` and ``vm-virtio`` were written with QEMU's > use cases in mind.
I'm working on this and trying to wrap simple memory access by vm-memory. > A coding style for devices > '''''''''''''''''''''''''' > > pl011 and HPET were developed independently and sometimes have different > idioms that could be unified. Peter Maydell made several observations: > > Something I do notice is that there's some inconsistency in > how we've structured things between the two devices, e.g.: ... > * pl011 has defined named fields for its registers, but hpet does > things like:: > > self.config.get() & (1 << HPET_CFG_LEG_RT_SHIFT) != 0 On the one hand, this way is more friendly and easy to review for C developers (comparing with the C version), and on the other hand, there are similar writeups in other projects (crosvm/firecracker). So, HPET also shows a clumsy and conservative approach :-). > * pl011 has a split between PL011State and PL011Registers, > but HPET does not. As I mentioned in an email thread a > little while back, I feel like the State/Registers split > is something we should either make a more clear deliberate > formalised separation that's part of how we recommend > device models should be designed Sorry, I just noticed this point. I tried to abstract a HPETRegisters but found out it didn't work well in HPET case...So I didn't insist. The following is the lesson I learned when I wrote HPET in Rust in the first time (copied from my HPET v2 cover letter [*]): Additional Experience ===================== PL011 provides a pattern to group all registers in one BqlRefCell instead of multiple BqlCells. I also tried to leverage this design to HPET, but it didn't fit very well with this issue: * HPETState abstracts many helpers to check register bit and tell caller about the state, e.g., is_legacy_mode(), is_timer_int_active(), etc. But this also means these helpers can't be used in BqlRefCell:: borrow_mut() context again, otherwise, they would cause the runtime bql check panic. - Some cases are easy to fix, i.e, use borrow_mut to modify the registers after the helpers' call. - Some cases would by tricky, like memory write callback, since it has complex logic and it's hard to decouple register changes from the reset logic as clearly as PL011 does. The fix for this case is either to avoid using register helpers again in the borrow_mut context of write(), or to use BqlCell instead of BqlRefCell to get finer-grained access control to avoid refactoring code logic. I chose the latter. So I think this might be a practical lesson that the choice of BqlCell and BqlRefCell is also indeed related to code logic: If the code logic is too complex to decouple borrow() and borrow_mut() (and the compiler doesn't check this, only the runtime's panic_already_borrowed() will complains!) , then BqlCell should be chosen. Because fine-grained access is easier to control and avoid errors. :-) [*]: https://lore.kernel.org/qemu-devel/20250125125137.1223277-1-zhao1....@intel.com/ Thanks, Zhao
