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);
 }
 
 #[repr(C)]
@@ -615,19 +615,6 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
     }
 }
 
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011State`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-pub unsafe extern "C" fn pl011_init(obj: *mut Object) {
-    unsafe {
-        debug_assert!(!obj.is_null());
-        let mut state = NonNull::new_unchecked(obj.cast::<PL011State>());
-        state.as_mut().init();
-    }
-}
-
 #[repr(C)]
 #[derive(Debug, qemu_api_macros::Object)]
 /// PL011 Luminary device model.
@@ -640,19 +627,16 @@ pub struct PL011LuminaryClass {
     _inner: [u8; 0],
 }
 
-/// Initializes a pre-allocated, unitialized instance of `PL011Luminary`.
-///
-/// # Safety
-///
-/// We expect the FFI user of this function to pass a valid pointer, that has
-/// the same size as [`PL011Luminary`]. We also expect the device is
-/// readable/writeable from one thread at any time.
-pub unsafe extern "C" fn pl011_luminary_init(obj: *mut Object) {
-    unsafe {
-        debug_assert!(!obj.is_null());
-        let mut state = NonNull::new_unchecked(obj.cast::<PL011Luminary>());
-        let state = state.as_mut();
-        state.parent_obj.device_id = DeviceId::Luminary;
+impl PL011Luminary {
+    /// Initializes a pre-allocated, unitialized instance of `PL011Luminary`.
+    ///
+    /// # Safety
+    ///
+    /// We expect the FFI user of this function to pass a valid pointer, that
+    /// has the same size as [`PL011Luminary`]. We also expect the device is
+    /// readable/writeable from one thread at any time.
+    unsafe fn init(&mut self) {
+        self.parent_obj.device_id = DeviceId::Luminary;
     }
 }
 
@@ -660,7 +644,7 @@ impl ObjectImpl for PL011Luminary {
     type Class = PL011LuminaryClass;
     const TYPE_NAME: &'static CStr = crate::TYPE_PL011_LUMINARY;
     const PARENT_TYPE_NAME: Option<&'static CStr> = Some(crate::TYPE_PL011);
-    const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = Some(pl011_luminary_init);
+    const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
 }
 
 impl DeviceImpl for PL011Luminary {}
diff --git a/rust/qemu-api/src/definitions.rs b/rust/qemu-api/src/definitions.rs
index d64a581a5cc..078ba30d61a 100644
--- a/rust/qemu-api/src/definitions.rs
+++ b/rust/qemu-api/src/definitions.rs
@@ -8,6 +8,24 @@
 
 use crate::bindings::{Object, ObjectClass, TypeInfo};
 
+unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) {
+    // SAFETY: obj is an instance of T, since rust_instance_init<T>
+    // is called from QOM core as the instance_init function
+    // for class T
+    unsafe { T::INSTANCE_INIT.unwrap()(&mut *obj.cast::<T>()) }
+}
+
+unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut Object) {
+    // SAFETY: obj is an instance of T, since rust_instance_post_init<T>
+    // is called from QOM core as the instance_post_init function
+    // for class T
+    //
+    // FIXME: it's not really guaranteed that there are no backpointers to
+    // obj; it's quite possible that they have been created by instance_init().
+    // The receiver should be &self, not &mut self.
+    T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::<T>() })
+}
+
 unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) {
     // SAFETY: obj is an instance of T, since drop_object<T>
     // is called from QOM core as the instance_finalize function
@@ -16,13 +34,42 @@
 }
 
 /// Trait a type must implement to be registered with QEMU.
+///
+/// # Safety
+///
+/// - the struct must be `#[repr(C)]`
+///
+/// - `Class` and `TYPE` must match the data in the `TypeInfo` (this is
+///   automatic if the class is defined via `ObjectImpl`).
+///
+/// - the first field of the struct must be of the instance struct corresponding
+///   to the superclass declared as `PARENT_TYPE_NAME`
 pub trait ObjectImpl: ClassInitImpl + Sized {
+    /// The QOM class object corresponding to this struct.  Not used yet.
     type Class;
+
+    /// The name of the type, which can be passed to `object_new()` to
+    /// generate an instance of this type.
     const TYPE_NAME: &'static CStr;
+
+    /// The parent of the type.  This should match the first field of
+    /// the struct that implements `ObjectImpl`:
     const PARENT_TYPE_NAME: Option<&'static CStr>;
+
+    /// Whether the object can be instantiated
     const ABSTRACT: bool = false;
-    const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
-    const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
+
+    /// Function that is called to initialize an object.  The parent class will
+    /// have already been initialized so the type is only responsible for
+    /// initializing its own members.
+    ///
+    /// FIXME: The argument is not really a valid reference. `&mut
+    /// MaybeUninit<Self>` would be a better description.
+    const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = None;
+
+    /// Function that is called to finish initialization of an object, once
+    /// `INSTANCE_INIT` functions have been called.
+    const INSTANCE_POST_INIT: Option<fn(&mut Self)> = None;
 
     const TYPE_INFO: TypeInfo = TypeInfo {
         name: Self::TYPE_NAME.as_ptr(),
@@ -33,8 +80,14 @@ pub trait ObjectImpl: ClassInitImpl + Sized {
         },
         instance_size: core::mem::size_of::<Self>(),
         instance_align: core::mem::align_of::<Self>(),
-        instance_init: Self::INSTANCE_INIT,
-        instance_post_init: Self::INSTANCE_POST_INIT,
+        instance_init: match Self::INSTANCE_INIT {
+            None => None,
+            Some(_) => Some(rust_instance_init::<Self>),
+        },
+        instance_post_init: match Self::INSTANCE_POST_INIT {
+            None => None,
+            Some(_) => Some(rust_instance_post_init::<Self>),
+        },
         instance_finalize: Some(drop_object::<Self>),
         abstract_: Self::ABSTRACT,
         class_size: core::mem::size_of::<Self::Class>(),
-- 
2.47.1

> +unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) {
> +    // SAFETY: obj is an instance of T, since rust_instance_init<T>
> +    // is called from QOM core as the instance_init function
> +    // for class T
> +    unsafe { T::INSTANCE_INIT.unwrap()(&mut *obj.cast::<T>()) }
> +}

Here's the difference, why doesn't init() narrow the unsafe scope like
post_init() does?

> +unsafe extern "C" fn rust_instance_post_init<T: ObjectImpl>(obj: *mut Object) {
> +    // SAFETY: obj is an instance of T, since rust_instance_post_init<T>
> +    // is called from QOM core as the instance_post_init function
> +    // for class T
> +    //
> +    // FIXME: it's not really guaranteed that there are no backpointers to
> +    // obj; it's quite possible that they have been created by instance_init().
> +    // The receiver should be &self, not &mut self.
> +    T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::<T>() })
> +}
> +

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?

> +///
> +/// - the struct must be `#[repr(C)]`
> +///
> +/// - `Class` and `TYPE` must match the data in the `TypeInfo` (this is
> +///   automatic if the class is defined via `ObjectImpl`).
> +///
> +/// - the first field of the struct must be of the instance struct corresponding
> +///   to the superclass declared as `PARENT_TYPE_NAME`
>  pub trait ObjectImpl: ClassInitImpl + Sized {
> +    /// The QOM class object corresponding to this struct.  Not used yet.
>      type Class;
> +
> +    /// The name of the type, which can be passed to `object_new()` to
> +    /// generate an instance of this type.
>      const TYPE_NAME: &'static CStr;
> +
> +    /// The parent of the type.  This should match the first field of
> +    /// the struct that implements `ObjectImpl`:
>      const PARENT_TYPE_NAME: Option<&'static CStr>;
> +
> +    /// Whether the object can be instantiated
>      const ABSTRACT: bool = false;
> -    const INSTANCE_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
> -    const INSTANCE_POST_INIT: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
> +
> +    /// Function that is called to initialize an object.  The parent class will
> +    /// have already been initialized so the type is only responsible for
> +    /// initializing its own members.
> +    ///
> +    /// FIXME: The argument is not really a valid reference. `&mut
> +    /// MaybeUninit<Self>` would be a better description.
> +    const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = None;

And here.

> +    /// Function that is called to finish initialization of an object, once
> +    /// `INSTANCE_INIT` functions have been called.
> +    const INSTANCE_POST_INIT: Option<fn(&mut Self)> = None;
>  

Otherwise,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

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

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

Il mer 11 dic 2024, 08:41 Zhao Liu <zhao1.liu@intel.com> ha scritto:

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

Yes that's pretty much it.

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

Generally:

- embedded objects will have to be initialized in instance_init unless they
are Options

- sysbus_init_irq and sysbus_init_mmio can be called in both instance_init
and instance_post_init for now, but they will have to be in post_init once
the signature of init changes to return impl PinInit

- if you don't need properties you can choose between post_init and
realize, if you need properties you need to initialize in realize (and
then, unlike C, you might need to explicitly allow the pre-realize state;
for example using Option<&...> instead of just a reference; or Vec<>
instead of an array).

>
> > 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. I discussed it with Manos some time ago and in principle you
could use a Box (QOM supports custom freeing functions) but it's a bit
complex because the freeing function would have to free the memory without
dropping the contents of the Box (the drop is done by QOM via
instance_finalize).

If you want to allocate the HPETTimers at realize time, I think you can
place them in a Vec. I think you won't need NonNull for this, but I am not
100% sure. Alternatively if you want to always prepare all MAX_TIMERS of
them and then only use a subset, you can use an array.

Either way, probably it makes sense for you to have an "fn
timers_iter(&self) -> impl Iter<Item = &BqlRefCell<HPETTimer>>" in
HPETState, or something like that. Then you can easily use for loops to
walk all timers between 0 and self.num_timers-1.

Paolo

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

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

> - if you don't need properties you can choose between post_init and
> realize, if you need properties you need to initialize in realize (and
> then, unlike C, you might need to explicitly allow the pre-realize state;
> for example using Option<&...> instead of just a reference; or Vec<>
> instead of an array).

...in realize(), also need to handle the "timers" property to enable
the timers that property requires.

> - sysbus_init_irq and sysbus_init_mmio can be called in both instance_init
> and instance_post_init for now, but they will have to be in post_init once
> the signature of init changes to return impl PinInit

make sense, thanks!

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

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?

> I discussed it with Manos some time ago and in principle you
> could use a Box (QOM supports custom freeing functions) but it's a bit
> complex because the freeing function would have to free the memory without
> dropping the contents of the Box (the drop is done by QOM via
> instance_finalize).
> 
> If you want to allocate the HPETTimers at realize time, I think you can
> place them in a Vec. I think you won't need NonNull for this, but I am not
> 100% sure. Alternatively if you want to always prepare all MAX_TIMERS of
> them and then only use a subset, you can use an array.

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

> Either way, probably it makes sense for you to have an "fn
> timers_iter(&self) -> impl Iter<Item = &BqlRefCell<HPETTimer>>" in
> HPETState, or something like that. Then you can easily use for loops to
> walk all timers between 0 and self.num_timers-1.

Good idea, yes, will do.

Thanks,
Zhao

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

> 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