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

[Paolo Bonzini]

Il gio 23 gen 2025, 10:05 Zhao Liu <zhao1.liu@intel.com> ha scritto:

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

&mut is not needed here, but it is needed in write(). After accept_input() is moved to out of memory_ops.rs, the #[allow] can be removed.

I will do that in v2.

Paolo

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