Re: Rust in QEMU update, April 2025

[Zhao Liu <zhao1.liu@intel.com>]

(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.liu@intel.com/

Thanks,
Zhao