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

[Zhao Liu]

On Thu, Dec 05, 2024 at 05:28:45PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 17:28:45 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 00/13] rust: Reinvent the wheel for HPET timer in Rust
> 
> 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.

Great! Sorry for the slow response, I just saw this comment. I'm looking
forward to your work on this!

> > 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.

Thank you!

> > 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.

I like this idea. Clear splitting helps to standardize the usage of
BqlCell, preventing both underuse and overuse.

> > 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?

Yes, it was based on old code base...and it seems outdated.

> 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,

Thank you, it looks better and overcomes the drawback of the original
proposal.

> > 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?

Based your and Richard's suggestion, modifying memattrs avoids
introducing additional library. :-)

> 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.

I think it could work, and this time it won't be needed, based on your
newer suggestion. :-)

> > 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.

Sure. I see.

> > 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.

Thank you!

Regards,
Zhao