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

[Zhao Liu]

> I see -- though, thinking more about it, since you have
> 
>     fn init_timer(&mut self) {
>         let raw_ptr: *mut HPETState = self;
> 
>         for i in 0..HPET_MAX_TIMERS {
>             let mut timer = self.get_timer(i).borrow_mut();
>             timer.init(i, raw_ptr).init_timer_with_state();
>         }
>     }
> 
> It seems to me that you can do everything in instance_init. Later on a
> function like the above
> will become something like
> 
> impl HPETTimer {
>     fn init_timer(hpet: NonNull<HPETState>, n: usize) -> impl PinInit<Self> {

Thank you! I should pass NonNull type other than `*mut HPETState` for now :-)

>         pin_init!(&this in HPETTimer {
>             index: n,
>             qemu_timer <- Timer::init_ns(...),
>             state: hpet,
>             config: 0,
>             cmp: 0,
>             fsb: 0,
>             cmp64: 0,
>             period: 0,
>             wrap_flag: false,
>             last: 0,
>         }
>     }
> }

That's the right and ideal way, and I like it.

> But even now you can write something that takes a &mut self as the
> first argument. It's undefined behavior but it's okay as long as we
> have a path forward.

Yes, I agree. In the next version, I can follow your suggestion and put
these embedded items into instance_init(), to be better prepared for the
next step.

> > > > > 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?
> > > >
> > >
> > > Allocation is still done by the C code, so I am not sure I understand the
> > > question. Rust code allocates QOM objects with object_new() so they are
> > > malloc-ed.
> >
> > Sorry, I'm not familiar enough with this piece...I have the question
> > because PinInit doc said "you will need a suitable memory location that
> > can hold a T. This can be KBox<T>, Arc<T>, UniqueArc<T> or even the
> > stack (see stack_pin_init!)."
> 
> Ah, I see. You can use __pinned_init directly on the memory that is
> passed to rust_instance_init. See for example the implementation of
> InPlaceWrite for Box
> (https://docs.rs/pinned-init/latest/src/pinned_init/lib.rs.html#1307).

Thank you! I understand your intention. A similar implementation would
also be quite natural in rust_instance_init.

> > I see that the core point is ensuring that structures cannot be moved.
> > Given that object_new() on the C side ensures that the allocated object
> > will not be moved, Rust side does not need to worry about pinning, correct?
> 
> Sort of... You still need to worry about it for two reasons:
> 
> 1) if you have &mut Self you can move values out of the object using
> e.g. mem::replace or mem::swap. Those would move the value in memory
> and cause trouble (think of moving a QEMUTimer while it is pointed to
> by the QEMUTimerList). This is solved by 1) using &Self all the time +
> interior mutability

With your help and through our discussions, I have gained a clearer
understanding of this intention.

> 2) using pinned_init's "PinnedDrop" functionality,
> because &Self can be used in QEMU-specific APIs but (obviously) not in
> the built-in Drop trait.
> 
> 2) right now marking something as pinned is an indirect way to tell
> the compiler and miri that there are external references to it. For a
> longer discussion you can read
> https://crates.io/crates/pinned-aliasable or
> https://gist.github.com/Darksonn/1567538f56af1a8038ecc3c664a42462.

Thanks for sharing!

> Linux does this with a wrapper type similar to the one in pinned-aliasable:
> 
> /// Stores an opaque value.
> ///
> /// This is meant to be used with FFI objects that are never
> interpreted by Rust code.
> #[repr(transparent)]
> pub struct Opaque<T> {
>     value: UnsafeCell<MaybeUninit<T>>,
>     _pin: PhantomPinned,
> }
> 
> It's on my todo list to introduce it in qemu_api::cell and (for
> example) change qom::Object from
> 
>     pub use bindings::Object
> 
> to
> 
>     pub type Object = Opaque<bindings::Object>;
> 
> Or something like that.

Yes, I agree with this idea. It's what we need.

Regards,
Zhao