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

[Paolo Bonzini]

On Wed, Dec 11, 2024 at 5:38 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> > Generally:
> >
> > - embedded objects will have to be initialized in instance_init unless they
> > are Options
>
> I see, at least for HPETTimer array, I need to prepare all of them in
> instance_init()...
>
> ...in realize(), also need to handle the "timers" property to enable
> the timers that property requires.

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> {
        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,
        }
    }
}

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.

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

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

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.

> Vec seems to lack proper vmstate support. I understand that we need to
> modify VMSTATE_STRUCT_VARRAY_POINTER_* to introduce a variant for Vec.
>
> Since the array support is already available, I chose to use an array
> instead (although vmstate is disabled for now).

Yes, you're right.

Palo