Re: [PATCH 14/26] rust: qom: move bridge for TypeInfo functions out of pl011

[Paolo Bonzini]

On 12/10/24 16:50, Zhao Liu wrote:
> On Mon, Dec 09, 2024 at 01:37:05PM +0100, Paolo Bonzini wrote:
> > Date: Mon,  9 Dec 2024 13:37:05 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: [PATCH 14/26] rust: qom: move bridge for TypeInfo functions out of
> >   pl011
> > X-Mailer: git-send-email 2.47.1
> >
> > Allow the ObjectImpl trait to expose Rust functions that avoid raw
> > pointers (though INSTANCE_INIT for example is still unsafe).
> > ObjectImpl::TYPE_INFO adds thunks around the functions in
> > ObjectImpl.
> >
> > While at it, document `TypeInfo`.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   rust/hw/char/pl011/src/device.rs | 40 +++++++--------------
> >   rust/qemu-api/src/definitions.rs | 61 +++++++++++++++++++++++++++++---
> >   2 files changed, 69 insertions(+), 32 deletions(-)
> >
> > diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> > index 56403c36609..b9f8fb134b5 100644
> > --- a/rust/hw/char/pl011/src/device.rs
> > +++ b/rust/hw/char/pl011/src/device.rs
> > @@ -110,7 +110,7 @@ impl ObjectImpl for PL011State {
> >       type Class = PL011Class;
> >       const TYPE_NAME: &'static CStr = crate::TYPE_PL011;
> >       const PARENT_TYPE_NAME: Option<&'static CStr> = Some(TYPE_SYS_BUS_DEVICE);
> > -    const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = 
> > Some(pl011_init);
> > +    const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
>
> No need to keep `unsafe` here?

Right now instance_init is called with only the parent initialized, and 
the remaining memory zeroed; its purpose is to prepare things for 
instance_post_init which can then be safe (it's also kind of wrong for 
instance_post_init to receive a &mut Self, because instance_init will 
create other pointers to the object, for example in a MemoryRegion's 
"parent" field).

The right thing to do would be to have an argument of type &mut 
MaybeUninit<Self>.  Then the caller would do something like

    let maybe_uninit = obj as *mut MaybeUninit<Self>;
    unsafe {
        Self::INSTANCE_INIT(&mut *maybe_uninit);
        maybe_uninit.assume_init_mut();
    }

Note however that INSTANCE_INIT would still be unsafe, because its 
safety promise is that it prepares things for the caller's 
assume_init_mut().

The way that this will become safe is to use the pinned_init crate from 
Linux: instance_init returns the initialization as an "impl 
PinInit<Self>", and then instance_post_init can run with a &self.  Until 
then, however, instance_init has to remain unsafe.

Paolo