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

[Zhao Liu]

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

Thank you for explanation. It makes a lot of sense.

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

Yes, I feel that this approach more clearly explains the purpose of QOM
init.

And since we are talking about the safety of INSTANCE_INIT, I think we
should add some safety guidelines here, like:
 * Proper Initialization of pointers and references
 * Explicit initialization of Non-Zero Fields
 * In placed memory region is correctly initialized.

(Or do you have any additional or clearer guidelines?)

This could be the reference when adding SAFETY comment for the device's
own `unsafe` init(). :-)

And this is also a good explanation to distinguish between initialization
in init() and realize(). For example, HPET attempts to initialize the
timer array in realize().

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

Then do we need to place OBJECT in some suitable memory location (Box or
something) for Rust implementation?

> and then instance_post_init can run with a &self.  Until then, however,
> instance_init has to remain unsafe.

Thanks for sharing! It's a very reasonable direction.

Regards,
Zhao