Re: [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/5/24 07:07, Zhao Liu wrote:
> * Proper bindings for MemoryRegionOps, which needs to wrap the ram
>    read/write callbacks.
>    - I think it shouldn't be complicated because qom/qdev already
>      provides good examples.

Sounds good, I have some initial work on PL011.

> * vmstate support.
>    - vmstate code for HPET is actually ready, but it will be pending (and
>      maybe re-writing) until the vmstate infra gets cleaned up.

Yeah, mostly due to the cells.

> * Error handling.
>    - Now HPET just use panic and println to replace error_setg and
>      warn_report.
> 
> * Trace support.
>    - No trace for now.

> Anything that needs to be modified within a callback should be protected
> by BqlCell/BqlRefCell.
> 
> Based on this, I am also considering how the opaque parameter in certain
> callbacks should interact with BQL cells. In the timer binding (patch
> 7), I think the opaque parameter accepted by the timer callback should
> be placed in a BQL cell. However, considering generality, I did not make
> further changes and only passed BqlRefCell<HPETTimer> as the opaque
> parameter in the HPET code.

That sounds good.

I'll review your timer bindings and post the infrastructure that I have 
for callbacks.

> Furthermore, is it possible in the future to wrap the entire state
> within a BQL cell? This could save the effort of wrapping many state
> members individually when state becomes very huge and complex.

I think it's better if we keep a split between the mutable and immutable 
parts.  For example properties are immutable.

> QDEV Property
> -------------
> 
> To support bit type property, I added another macro variant (in patch 8)
> to allow bitnr parameter. However, I think this lacks scalability.
> 
> In qdev-properties.h, it is clear that the PropertyInfo of a property is
> bound to its type. Previously, Junjie and I attempted to do the same in
> Rust by binding PropertyInfo to the type, thereby avoiding the need to
> specify too many parameters in the macro definitions:
> 
> https://lore.kernel.org/qemu-devel/20241017143245.1248589-1-zhao1.liu@intel.com/

Oops, sorry.  I have applied patch 1, but patch 2 is a bit problematic 
because I think it's not const-friendly.  Maybe it was applied to older 
code?

I haven't looked closely at whether it's possible to use trait { ... 
const TYPE: &Property = ... }, maybe only in newer Rust?  But as you 
point out there is an issue with multiple PropertyInfos that can apply 
to the same type, therefore I think the way to go here is to use a 
procedural macro and a #[property] attribute.  That also makes bit 
properties easy:

     #[property(bit=0, name="foo")
     #[property(bit=1, name="bar")
     features: u32,

> MEMTXATTRS_UNSPECIFIED
> ----------------------
> 
> MEMTXATTRS_UNSPECIFIED is another global variable. Since it is
> immutable, BQL cell is not needed.
> 
> But MemTxAttrs is a structure with bitfields, and the bindings generated
> by bindgen can only be modified through methods. Therefore, it is
> necessary to introduce lazy to initialize MEMTXATTRS_UNSPECIFIED in a
> const context (patch 6).

Ugh, that's a pity.  Maybe instead of introducing a new dependency we 
can use an always-inlined function?

In an ideal world, bindgen would produce const functions.  A hackish but 
easy possibility is to modify the generated bindgen code (with sed? :)) 
to apply const to the bitfield functions.

> Cycle Reference
> ---------------
> 
> A common pattern in QEMU devices is that a sub-member of the state
> contains a pointer to the state itself.
> 
> For HPETState, it maintains a HPETTimer array, and each HPETTimer also
> has the pointer to parent HPETState. So there's the cycle reference
> issue.
> 
> The common way to address this is to use RefCell<Weak<>> [2], but in
> practice, I found it's difficult to be applied in device code. The
> device instance is not created in Rust side, and there's only init()
> method to initialize created device instance. This way, it's hard to be
> compatible with the pattern of creating weak references (at least I
> failed).
> 
> Then, I chose NonNull to address this issue, as recommended by the
> author of NonNull and the standard collections [3].

NonNull is okay for now.  Later on we will handle it with pinned_init.

However, do not get a &mut HPETState, everything should go through 
&HPETState and cells.

> Therefore, I believe that in Rust devices, QOM members should also be
> kept private or at most `pub(crate)`. This is also why I tried to avoid
> using direct `pub` in HPET.

Yes, I agree.  I should look at unnecessarily pub things in pl011.

Paolo