Re: [PATCH 07/10] rust: pl011: wrap registers with BqlRefCell

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

> > >  #[repr(C)]
> > > -#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
> >
> > This is the issue I also met, so why not drive "Debug" for BqlRefCell?
> >
> 
> Because it is not entirely possible to do it safely--there could be
> outstanding borrows that break invariants and cause debug() to fail. Maybe
> we could implement it on BqlRefCell<PL011Registers> with a custom derive
> macro...
> 
> RefCell doesn't implement Debug either for the same reason.

Thank you for the clarification, I understand now (I was indeed puzzled
as to why RefCell didn't do this).

> I tried to do this in [*]. Do we need to reconsider this?
> >
> > [*]:
> > https://lore.kernel.org/qemu-devel/20241205060714.256270-3-zhao1.liu@intel.com/
> >
> > > +#[derive(qemu_api_macros::Object, qemu_api_macros::offsets)]
> > >  /// PL011 Device Model in QEMU
> > >  pub struct PL011State {
> > >      pub parent_obj: ParentField<SysBusDevice>,
> > >      pub iomem: MemoryRegion,
> > >      #[doc(alias = "chr")]
> > >      pub char_backend: CharBackend,
> > > -    pub regs: PL011Registers,
> > > +    pub regs: BqlRefCell<PL011Registers>,
> >
> > This is a good example on the usage of BqlRefCell!
> >
> > //! `BqlRefCell` is best suited for data that is primarily accessed by the
> > //! device's own methods, where multiple reads and writes can be grouped
> > within
> > //! a single borrow and a mutable reference can be passed around. "
> >
> 
> Yeah, the comment was inspired by this usage and not vice versa. :D
> 
> >      /// QEMU interrupts
> > >      ///
> > >      /// ```text
> > > @@ -530,8 +530,8 @@ fn post_init(&self) {
> > >          }
> > >      }
> > >
> > > +    #[allow(clippy::needless_pass_by_ref_mut)]
> >
> > How did you trigger this lint error? I switched to 1.84 and didn't get
> > any errors (I noticed that 1.84 fixed the issue of ignoring `self` [*],
> > but it still doesn't seem to work on my side).
> >
> 
> I will double check. But I do see that there is no mut access inside, at
> least not until the qemu_chr_fe_accept_input() is moved here. Unfortunately
> until all MemoryRegion and CharBackend bindings are available the uses of
> &mut and the casts to *mut are really really wonky.

yes, I agree here we should remove mut :-). (if needless_pass_by_ref_mut
doesn't work on this place, I think we can drop it.)

> (On the other hand it wouldn't be possible to have a grip on the qemu_api
> code without users).
> 
> Paolo
> 
> > @@ -603,19 +603,19 @@ pub fn realize(&mut self) {
> > >      }
> > >
> > >      pub fn reset(&mut self) {
> >
> > In principle, this place should also trigger `needless_pass_by_ref_mut`.
> >
> 
> Yes but clippy hides it because this function is assigned to a function
> pointer const. At least I think so---the point is more generally that you
> can't change &mut to & without breaking compilation.

Make sense!

> > > -        self.regs.reset();
> > > +        self.regs.borrow_mut().reset();
> > >      }
> >
> > [snip]
> >
> > > @@ -657,10 +657,10 @@ pub fn post_load(&mut self, _version_id: u32) ->
> > Result<(), ()> {
> > >  pub unsafe extern "C" fn pl011_receive(opaque: *mut c_void, buf: *const
> > u8, size: c_int) {
> > >      unsafe {
> > >          debug_assert!(!opaque.is_null());
> > > -        let mut state =
> > NonNull::new_unchecked(opaque.cast::<PL011State>());
> > > +        let state = NonNull::new_unchecked(opaque.cast::<PL011State>());
> >
> > Perhaps we can use NonNull::new and unwrap()? Then debug_assert! is
> > unnecessary.
> >
> > let state = unsafe {
> > NonNull::new(opaque.cast::<PL011State>()).unwrap().as_ref() };
> >
> 
> Yeah, though that's preexisting and it's code that will go away relatively
> soon. I tried to minimize unrelated changes and changes to these temporary
> unsafe functions, but in some cases there were some that sneaked in.
> 
> Let me know what you prefer.
>

I prefer to use NonNull::new and unwrap(). Too much assert() pattern is
not user-friendly. I also think it's unnecessary to change NonNull
interface in this patch, we can see what's left when you're done with
the most QAPI work.

Thanks,
Zhao