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

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 3891 bytes --]