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