[PATCH 0/5] rust: make instance_init implementations use safe Rust

[PATCH 0/5] rust: make instance_init implementations use safe Rust

[Paolo Bonzini]

This is a step towards safe bindings to instance_init: making the
implementation safe, though ensuring that *all* fields as initialized
is still up to the implementor.  This helps making it clear what we
want from crates like pinned-init (the Linux one) or its inspiration
pin-init.

This series has two concoctions that are a bit more advanced.

The one in the first patch is probably going to be temporary
once instance_init takes its final form, but it's very useful while
we're stuck with piece-by-piece initialization.

The fourth patch instead is an adaptation of the technique in
GhostCell (https://plv.mpi-sws.org/rustbelt/ghostcell/), which allows
to isolate an object within the invocation of a function.  This one
probably will stay, together with the ParentInit struct that (in one
shape or another) acts as the "proof" that instance_init has been
called on the parent classes.

Paolo

Paolo Bonzini (5):
  rust: qemu_api: introduce MaybeUninit field projection
  rust: hpet: fully initialize object after instance_init
  rust: qom: introduce ParentInit
  rust: qom: make ParentInit lifetime-invariant
  rust: qom: change instance_init to take a ParentInit<>

 rust/hw/char/pl011/src/device.rs |  34 +++---
 rust/hw/timer/hpet/src/device.rs |  56 +++++-----
 rust/qemu-api/meson.build        |   1 +
 rust/qemu-api/src/lib.rs         |   1 +
 rust/qemu-api/src/memory.rs      |  12 +--
 rust/qemu-api/src/qdev.rs        |  51 +++++----
 rust/qemu-api/src/qom.rs         | 175 ++++++++++++++++++++++++++++++-
 rust/qemu-api/src/uninit.rs      |  85 +++++++++++++++
 8 files changed, 341 insertions(+), 74 deletions(-)
 create mode 100644 rust/qemu-api/src/uninit.rs

-- 
2.49.0

[PATCH 1/5] rust: qemu_api: introduce MaybeUninit field projection

[Paolo Bonzini]

Add a macro that makes it possible to convert a MaybeUninit<> into
another MaybeUninit<> for a single field within it.  Furthermore, it is
possible to use the resulting MaybeUninitField<> in APIs that take the
parent object, such as memory_region_init_io().

This allows removing some of the undefined behavior from instance_init()
functions, though this may not be the definitive implementation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/meson.build   |  1 +
 rust/qemu-api/src/lib.rs    |  1 +
 rust/qemu-api/src/uninit.rs | 85 +++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 rust/qemu-api/src/uninit.rs

diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index cac8595a148..33653b4a28e 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -28,6 +28,7 @@ _qemu_api_rs = static_library(
       'src/qom.rs',
       'src/sysbus.rs',
       'src/timer.rs',
+      'src/uninit.rs',
       'src/vmstate.rs',
       'src/zeroable.rs',
     ],
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 93902fc94bc..c78198f0f41 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -27,6 +27,7 @@
 pub mod qom;
 pub mod sysbus;
 pub mod timer;
+pub mod uninit;
 pub mod vmstate;
 pub mod zeroable;
 
diff --git a/rust/qemu-api/src/uninit.rs b/rust/qemu-api/src/uninit.rs
new file mode 100644
index 00000000000..a7bed6935b1
--- /dev/null
+++ b/rust/qemu-api/src/uninit.rs
@@ -0,0 +1,85 @@
+//! Access fields of a [`MaybeUninit`](std::mem::MaybeUninit)
+
+use std::{
+    mem::MaybeUninit,
+    ops::{Deref, DerefMut},
+};
+
+pub struct MaybeUninitField<'a, T, U> {
+    parent: &'a mut MaybeUninit<T>,
+    child: *mut U,
+}
+
+impl<'a, T, U> MaybeUninitField<'a, T, U> {
+    #[doc(hidden)]
+    pub fn new(parent: &'a mut MaybeUninit<T>, child: *mut U) -> Self {
+        MaybeUninitField { parent, child }
+    }
+
+    /// Return a constant pointer to the containing object of the field.
+    ///
+    /// Because the `MaybeUninitField` remembers the containing object,
+    /// it is possible to use it in foreign APIs that initialize the
+    /// child.
+    pub fn parent(f: &Self) -> *const T {
+        f.parent.as_ptr()
+    }
+
+    /// Return a mutable pointer to the containing object.
+    ///
+    /// Because the `MaybeUninitField` remembers the containing object,
+    /// it is possible to use it in foreign APIs that initialize the
+    /// child.
+    pub fn parent_mut(f: &mut Self) -> *mut T {
+        f.parent.as_mut_ptr()
+    }
+}
+
+impl<'a, T, U> Deref for MaybeUninitField<'a, T, U> {
+    type Target = MaybeUninit<U>;
+
+    fn deref(&self) -> &MaybeUninit<U> {
+        // SAFETY: self.child was obtained by dereferencing a valid mutable
+        // reference; the content of the memory may be invalid or uninitialized
+        // but MaybeUninit<_> makes no assumption on it
+        unsafe { &*(self.child.cast()) }
+    }
+}
+
+impl<'a, T, U> DerefMut for MaybeUninitField<'a, T, U> {
+    fn deref_mut(&mut self) -> &mut MaybeUninit<U> {
+        // SAFETY: self.child was obtained by dereferencing a valid mutable
+        // reference; the content of the memory may be invalid or uninitialized
+        // but MaybeUninit<_> makes no assumption on it
+        unsafe { &mut *(self.child.cast()) }
+    }
+}
+
+/// ```
+/// #[derive(Debug)]
+/// struct S {
+///     x: u32,
+///     y: u32,
+/// }
+///
+/// # use std::mem::MaybeUninit;
+/// # use qemu_api::{assert_match, uninit_field_mut};
+///
+/// let mut s: MaybeUninit<S> = MaybeUninit::zeroed();
+/// uninit_field_mut!(s, x).write(5);
+/// let s = unsafe { s.assume_init() };
+/// assert_match!(s, S { x: 5, y: 0 });
+/// ```
+#[macro_export]
+macro_rules! uninit_field_mut {
+    ($container:expr, $($field:tt)+) => {{
+        let container__: &mut ::std::mem::MaybeUninit<_> = &mut $container;
+        let container_ptr__ = container__.as_mut_ptr();
+
+        // SAFETY: the container is not used directly, only through a MaybeUninit<>,
+        // so the safety is delegated to the caller and to final invocation of
+        // assume_init()
+        let target__ = unsafe { std::ptr::addr_of_mut!((*container_ptr__).$($field)+) };
+        $crate::uninit::MaybeUninitField::new(container__, target__)
+    }};
+}
-- 
2.49.0

[PATCH 2/5] rust: hpet: fully initialize object after instance_init

[Paolo Bonzini]

The array of BqlRefCell<HPETTimer> is not initialized yet at the
end of instance_init.  In particular, the "state" field is NonNull
and therefore it is invalid to have it as zero bytes.

Note that MaybeUninit is necessary because assigning to self.timers[index]
would trigger Drop of the old value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/timer/hpet/src/device.rs | 42 +++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 735b2fbef79..340ca1d355d 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -4,6 +4,7 @@
 
 use std::{
     ffi::{c_int, c_void, CStr},
+    mem::MaybeUninit,
     pin::Pin,
     ptr::{addr_of_mut, null_mut, NonNull},
     slice::from_ref,
@@ -25,6 +26,7 @@
     qom_isa,
     sysbus::{SysBusDevice, SysBusDeviceImpl},
     timer::{Timer, CLOCK_VIRTUAL, NANOSECONDS_PER_SECOND},
+    uninit_field_mut,
     vmstate::VMStateDescription,
     vmstate_fields, vmstate_of, vmstate_struct, vmstate_subsections, vmstate_validate,
     zeroable::Zeroable,
@@ -212,13 +214,13 @@ pub struct HPETTimer {
 }
 
 impl HPETTimer {
-    fn init(&mut self, index: u8, state: &HPETState) {
-        *self = HPETTimer {
+    fn new(index: u8, state: *const HPETState) -> HPETTimer {
+        HPETTimer {
             index,
             // SAFETY: the HPETTimer will only be used after the timer
             // is initialized below.
             qemu_timer: unsafe { Timer::new() },
-            state: NonNull::new((state as *const HPETState).cast_mut()).unwrap(),
+            state: NonNull::new(state.cast_mut()).unwrap(),
             config: 0,
             cmp: 0,
             fsb: 0,
@@ -226,19 +228,15 @@ fn init(&mut self, index: u8, state: &HPETState) {
             period: 0,
             wrap_flag: 0,
             last: 0,
-        };
+        }
+    }
 
+    fn init_timer_with_cell(cell: &BqlRefCell<Self>) {
+        let mut timer = cell.borrow_mut();
         // SAFETY: HPETTimer is only used as part of HPETState, which is
         // always pinned.
-        let qemu_timer = unsafe { Pin::new_unchecked(&mut self.qemu_timer) };
-        qemu_timer.init_full(
-            None,
-            CLOCK_VIRTUAL,
-            Timer::NS,
-            0,
-            timer_handler,
-            &state.timers[self.index as usize],
-        )
+        let qemu_timer = unsafe { Pin::new_unchecked(&mut timer.qemu_timer) };
+        qemu_timer.init_full(None, CLOCK_VIRTUAL, Timer::NS, 0, timer_handler, cell);
     }
 
     fn get_state(&self) -> &HPETState {
@@ -607,9 +605,18 @@ fn handle_legacy_irq(&self, irq: u32, level: u32) {
         }
     }
 
-    fn init_timer(&self) {
-        for (index, timer) in self.timers.iter().enumerate() {
-            timer.borrow_mut().init(index.try_into().unwrap(), self);
+    fn init_timers(this: &mut MaybeUninit<Self>) {
+        let state = this.as_ptr();
+        for index in 0..HPET_MAX_TIMERS {
+            let mut timer = uninit_field_mut!(*this, timers[index]);
+
+            // Initialize in two steps, to avoid calling Timer::init_full on a
+            // temporary that can be moved.
+            let timer = timer.write(BqlRefCell::new(HPETTimer::new(
+                index.try_into().unwrap(),
+                state,
+            )));
+            HPETTimer::init_timer_with_cell(timer);
         }
     }
 
@@ -710,6 +717,8 @@ unsafe fn init(&mut self) {
             "hpet",
             HPET_REG_SPACE_LEN,
         );
+
+        Self::init_timers(unsafe { &mut *((self as *mut Self).cast::<MaybeUninit<Self>>()) });
     }
 
     fn post_init(&self) {
@@ -731,7 +740,6 @@ fn realize(&self) -> qemu_api::Result<()> {
 
         self.hpet_id.set(HPETFwConfig::assign_hpet_id()?);
 
-        self.init_timer();
         // 64-bit General Capabilities and ID Register; LegacyReplacementRoute.
         self.capability.set(
             HPET_CAP_REV_ID_VALUE << HPET_CAP_REV_ID_SHIFT |
-- 
2.49.0

[PATCH 3/5] rust: qom: introduce ParentInit

[Paolo Bonzini]

This is a smart pointer for MaybeUninit; it can be upcasted to the
already-initialized parent classes, or dereferenced to a MaybeUninit
for the class that is being initialized.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/qom.rs | 86 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 14f98fee60a..21c271cd2f9 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -95,7 +95,7 @@
 use std::{
     ffi::{c_void, CStr},
     fmt,
-    mem::ManuallyDrop,
+    mem::{ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
@@ -206,6 +206,90 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
     }
 }
 
+/// This struct knows that the superclasses of the object have already been
+/// initialized.
+pub struct ParentInit<'a, T>(&'a mut MaybeUninit<T>);
+
+impl<'a, T> ParentInit<'a, T> {
+    #[inline]
+    pub fn with(obj: &'a mut MaybeUninit<T>, f: impl FnOnce(ParentInit<'a, T>)) {
+        let parent_init = ParentInit(obj);
+        f(parent_init)
+    }
+}
+
+impl<T: ObjectType> ParentInit<'_, T> {
+    /// Return the receiver as a mutable raw pointer to Object.
+    pub fn as_object_mut_ptr(&self) -> *mut bindings::Object {
+        self.as_object_ptr().cast_mut()
+    }
+
+    /// Return the receiver as a const raw pointer to Object.
+    /// This is preferrable to `as_object_mut_ptr()` if a C
+    /// function only needs a `const Object *`.
+    pub fn as_object_ptr(&self) -> *const bindings::Object {
+        self.0.as_ptr().cast()
+    }
+}
+
+impl<'a, T: ObjectImpl> ParentInit<'a, T> {
+    /// Convert from a derived type to one of its parent types, which
+    /// have already been initialized.
+    ///
+    /// # Safety
+    ///
+    /// Structurally this is always a safe operation; the [`IsA`] trait
+    /// provides static verification trait that `Self` dereferences to `U` or
+    /// a child of `U`, and only parent types of `T` are allowed.
+    ///
+    /// However, while the fields of the resulting reference are initialized,
+    /// calls might use uninitialized fields of the subclass.  It is your
+    /// responsibility to avoid this.
+    pub unsafe fn upcast<U: ObjectType>(&self) -> &'a U
+    where
+        T::ParentType: IsA<U>,
+    {
+        // SAFETY: soundness is declared via IsA<U>, which is an unsafe trait;
+        // the parent has been initialized before `instance_init `is called
+        unsafe { &*(self.0.as_ptr().cast::<U>()) }
+    }
+
+    /// Convert from a derived type to one of its parent types, which
+    /// have already been initialized.
+    ///
+    /// # Safety
+    ///
+    /// Structurally this is always a safe operation; the [`IsA`] trait
+    /// provides static verification trait that `Self` dereferences to `U` or
+    /// a child of `U`, and only parent types of `T` are allowed.
+    ///
+    /// However, while the fields of the resulting reference are initialized,
+    /// calls might use uninitialized fields of the subclass.  It is your
+    /// responsibility to avoid this.
+    pub unsafe fn upcast_mut<U: ObjectType>(&mut self) -> &'a mut U
+    where
+        T::ParentType: IsA<U>,
+    {
+        // SAFETY: soundness is declared via IsA<U>, which is an unsafe trait;
+        // the parent has been initialized before `instance_init `is called
+        unsafe { &mut *(self.0.as_mut_ptr().cast::<U>()) }
+    }
+}
+
+impl<T> Deref for ParentInit<'_, T> {
+    type Target = MaybeUninit<T>;
+
+    fn deref(&self) -> &Self::Target {
+        self.0
+    }
+}
+
+impl<T> DerefMut for ParentInit<'_, T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        self.0
+    }
+}
+
 unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut bindings::Object) {
     let mut state = NonNull::new(obj).unwrap().cast::<T>();
     // SAFETY: obj is an instance of T, since rust_instance_init<T>
-- 
2.49.0

[PATCH 4/5] rust: qom: make ParentInit lifetime-invariant

[Paolo Bonzini]

This is the trick that allows the parent-field initializer to be used
only for the object that it's meant to be initialized.  This way,
the owner of a MemoryRegion must be the object that embeds it.

More information is in the comments; it's best explained with a simplified
example.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/qom.rs | 88 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 4 deletions(-)

diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 21c271cd2f9..1481ef20f0c 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -95,6 +95,7 @@
 use std::{
     ffi::{c_void, CStr},
     fmt,
+    marker::PhantomData,
     mem::{ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
     ptr::NonNull,
@@ -208,12 +209,91 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
 
 /// This struct knows that the superclasses of the object have already been
 /// initialized.
-pub struct ParentInit<'a, T>(&'a mut MaybeUninit<T>);
+///
+/// The declaration of `ParentInit` is.. *"a kind of magic"*.  It uses a
+/// technique that is found in several crates, the main ones probably being
+/// `ghost-cell` (in fact it was introduced by the [`GhostCell` paper](https://plv.mpi-sws.org/rustbelt/ghostcell/))
+/// and `generativity`.
+///
+/// The `PhantomData` makes the `ParentInit` type *invariant* with respect to
+/// the lifetime argument `'init`.  This, together with the `for<'...>` in
+/// `[ParentInit::with]`, block any attempt of the compiler to be creative when
+/// operating on types of type `ParentInit` and to extend their lifetimes.  In
+/// particular, it ensures that the `ParentInit` cannot be made to outlive the
+/// `rust_instance_init()` function that creates it, and therefore that the
+/// `&'init T` reference is valid.
+///
+/// This implementation of the same concept, without the QOM baggage, can help
+/// understanding the effect:
+///
+/// ```
+/// use std::marker::PhantomData;
+///
+/// #[derive(PartialEq, Eq)]
+/// pub struct Jail<'closure, T: Copy>(&'closure T, PhantomData<fn(&'closure ()) -> &'closure ()>);
+///
+/// impl<'closure, T: Copy> Jail<'closure, T> {
+///     fn get(&self) -> T {
+///         *self.0
+///     }
+///
+///     #[inline]
+///     fn with<U>(v: T, f: impl for<'id> FnOnce(Jail<'id, T>) -> U) -> U {
+///         let parent_init = Jail(&v, PhantomData);
+///         f(parent_init)
+///     }
+/// }
+/// ```
+///
+/// It's impossible to escape the `Jail`; `token1` cannot be moved out of the
+/// closure:
+///
+/// ```ignore
+/// let x = 42;
+/// let escape = Jail::with(&x, |token1| {
+///     println!("{}", token1.get());
+///     token1
+/// });
+/// // fails to compile:
+/// println!("{}", escape.get());
+/// ```
+///
+/// Likewise, in the QOM case the `ParentInit` cannot be moved out of
+/// `instance_init()`. Without this trick it would be possible to stash a
+/// `ParentInit` and use it later to access uninitialized memory.
+///
+/// Here is another example, showing how separately-created "identities" stay
+/// isolated:
+///
+/// ```ignore
+/// impl<'closure, T: Copy> Clone for Jail<'closure, T> {
+///     fn clone(&self) -> Jail<'closure, T> {
+///         Jail(self.0, PhantomData)
+///     }
+/// }
+///
+/// fn main() {
+///     Jail::with(42, |token1| {
+///         // this works and returns true: the clone has the same "identity"
+///         println!("{}", token1 == token1.clone());
+///         Jail::with(42, |token2| {
+///             // here the outer token remains accessible...
+///             println!("{}", token1.get());
+///             // ... but the two are separate: this fails to compile:
+///             println!("{}", token1 == token2);
+///         });
+///     });
+/// }
+/// ```
+pub struct ParentInit<'init, T>(
+    &'init mut MaybeUninit<T>,
+    PhantomData<fn(&'init ()) -> &'init ()>,
+);
 
-impl<'a, T> ParentInit<'a, T> {
+impl<'init, T> ParentInit<'init, T> {
     #[inline]
-    pub fn with(obj: &'a mut MaybeUninit<T>, f: impl FnOnce(ParentInit<'a, T>)) {
-        let parent_init = ParentInit(obj);
+    pub fn with(obj: &'init mut MaybeUninit<T>, f: impl for<'id> FnOnce(ParentInit<'id, T>)) {
+        let parent_init = ParentInit(obj, PhantomData);
         f(parent_init)
     }
 }
-- 
2.49.0

[PATCH 5/5] rust: qom: change instance_init to take a ParentInit<>

[Paolo Bonzini]

This removes undefined behavior associated to writing to uninitialized
fields, and makes it possible to remove "unsafe" from the instance_init
implementation.

However, the init function itself is still unsafe, because it must promise
(as a sort as MaybeUninit::assume_init) that all fields have been
initialized.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 34 ++++++++++-----------
 rust/hw/timer/hpet/src/device.rs | 16 ++++------
 rust/qemu-api/src/memory.rs      | 12 ++++----
 rust/qemu-api/src/qdev.rs        | 51 ++++++++++++++++++++------------
 rust/qemu-api/src/qom.rs         |  9 ++++--
 5 files changed, 65 insertions(+), 57 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index be8387f6f2d..2d416cd9a3c 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -2,7 +2,7 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-use std::{ffi::CStr, mem::size_of, ptr::addr_of_mut};
+use std::{ffi::CStr, mem::size_of};
 
 use qemu_api::{
     chardev::{CharBackend, Chardev, Event},
@@ -11,9 +11,10 @@
     memory::{hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder},
     prelude::*,
     qdev::{Clock, ClockEvent, DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
-    qom::{ObjectImpl, Owned, ParentField},
+    qom::{ObjectImpl, Owned, ParentField, ParentInit},
     static_assert,
     sysbus::{SysBusDevice, SysBusDeviceImpl},
+    uninit_field_mut,
     vmstate::VMStateDescription,
 };
 
@@ -163,7 +164,7 @@ impl PL011Impl for PL011State {
 impl ObjectImpl for PL011State {
     type ParentType = SysBusDevice;
 
-    const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+    const INSTANCE_INIT: Option<unsafe fn(ParentInit<Self>)> = Some(Self::init);
     const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
     const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
 }
@@ -488,7 +489,7 @@ impl PL011State {
     /// `PL011State` type. It must not be called more than once on the same
     /// location/instance. All its fields are expected to hold uninitialized
     /// values with the sole exception of `parent_obj`.
-    unsafe fn init(&mut self) {
+    unsafe fn init(mut this: ParentInit<Self>) {
         static PL011_OPS: MemoryRegionOps<PL011State> = MemoryRegionOpsBuilder::<PL011State>::new()
             .read(&PL011State::read)
             .write(&PL011State::write)
@@ -496,28 +497,23 @@ unsafe fn init(&mut self) {
             .impl_sizes(4, 4)
             .build();
 
-        // SAFETY:
-        //
-        // self and self.iomem are guaranteed to be valid at this point since callers
-        // must make sure the `self` reference is valid.
+        // SAFETY: this and this.iomem are guaranteed to be valid at this point
         MemoryRegion::init_io(
-            unsafe { &mut *addr_of_mut!(self.iomem) },
-            addr_of_mut!(*self),
+            &mut uninit_field_mut!(*this, iomem),
             &PL011_OPS,
             "pl011",
             0x1000,
         );
 
-        self.regs = Default::default();
+        uninit_field_mut!(*this, regs).write(Default::default());
 
-        // SAFETY:
-        //
-        // self.clock is not initialized at this point; but since `Owned<_>` is
-        // not Drop, we can overwrite the undefined value without side effects;
-        // it's not sound but, because for all PL011State instances are created
-        // by QOM code which calls this function to initialize the fields, at
-        // leastno code is able to access an invalid self.clock value.
-        self.clock = self.init_clock_in("clk", &Self::clock_update, ClockEvent::ClockUpdate);
+        let clock = DeviceState::init_clock_in(
+            &mut this,
+            "clk",
+            &Self::clock_update,
+            ClockEvent::ClockUpdate,
+        );
+        uninit_field_mut!(*this, clock).write(clock);
     }
 
     const fn clock_update(&self, _event: ClockEvent) {
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 340ca1d355d..a281927781e 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -21,8 +21,8 @@
         hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder, MEMTXATTRS_UNSPECIFIED,
     },
     prelude::*,
-    qdev::{DeviceImpl, DeviceMethods, DeviceState, Property, ResetType, ResettablePhasesImpl},
-    qom::{ObjectImpl, ObjectType, ParentField},
+    qdev::{DeviceImpl, DeviceState, Property, ResetType, ResettablePhasesImpl},
+    qom::{ObjectImpl, ObjectType, ParentField, ParentInit},
     qom_isa,
     sysbus::{SysBusDevice, SysBusDeviceImpl},
     timer::{Timer, CLOCK_VIRTUAL, NANOSECONDS_PER_SECOND},
@@ -697,7 +697,7 @@ fn set_counter_reg(&self, shift: u32, len: u32, val: u64) {
             .set(self.counter.get().deposit(shift, len, val));
     }
 
-    unsafe fn init(&mut self) {
+    unsafe fn init(mut this: ParentInit<Self>) {
         static HPET_RAM_OPS: MemoryRegionOps<HPETState> =
             MemoryRegionOpsBuilder::<HPETState>::new()
                 .read(&HPETState::read)
@@ -707,18 +707,14 @@ unsafe fn init(&mut self) {
                 .impl_sizes(4, 8)
                 .build();
 
-        // SAFETY:
-        // self and self.iomem are guaranteed to be valid at this point since callers
-        // must make sure the `self` reference is valid.
         MemoryRegion::init_io(
-            unsafe { &mut *addr_of_mut!(self.iomem) },
-            addr_of_mut!(*self),
+            &mut uninit_field_mut!(*this, iomem),
             &HPET_RAM_OPS,
             "hpet",
             HPET_REG_SPACE_LEN,
         );
 
-        Self::init_timers(unsafe { &mut *((self as *mut Self).cast::<MaybeUninit<Self>>()) });
+        Self::init_timers(&mut this);
     }
 
     fn post_init(&self) {
@@ -900,7 +896,7 @@ unsafe impl ObjectType for HPETState {
 impl ObjectImpl for HPETState {
     type ParentType = SysBusDevice;
 
-    const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+    const INSTANCE_INIT: Option<unsafe fn(ParentInit<Self>)> = Some(Self::init);
     const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
     const CLASS_INIT: fn(&mut Self::Class) = Self::Class::class_init::<Self>;
 }
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index 9ef2694bd62..e40fad6cf19 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -16,6 +16,7 @@
     callbacks::FnCall,
     cell::Opaque,
     prelude::*,
+    uninit::MaybeUninitField,
     zeroable::Zeroable,
 };
 
@@ -147,7 +148,7 @@ impl MemoryRegion {
     #[inline(always)]
     unsafe fn do_init_io(
         slot: *mut bindings::MemoryRegion,
-        owner: *mut Object,
+        owner: *mut bindings::Object,
         ops: &'static bindings::MemoryRegionOps,
         name: &'static str,
         size: u64,
@@ -156,7 +157,7 @@ unsafe fn do_init_io(
             let cstr = CString::new(name).unwrap();
             memory_region_init_io(
                 slot,
-                owner.cast::<bindings::Object>(),
+                owner,
                 ops,
                 owner.cast::<c_void>(),
                 cstr.as_ptr(),
@@ -166,16 +167,15 @@ unsafe fn do_init_io(
     }
 
     pub fn init_io<T: IsA<Object>>(
-        &mut self,
-        owner: *mut T,
+        this: &mut MaybeUninitField<'_, T, Self>,
         ops: &'static MemoryRegionOps<T>,
         name: &'static str,
         size: u64,
     ) {
         unsafe {
             Self::do_init_io(
-                self.0.as_mut_ptr(),
-                owner.cast::<Object>(),
+                this.as_mut_ptr().cast(),
+                MaybeUninitField::parent_mut(this).cast(),
                 &ops.0,
                 name,
                 size,
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 0610959f467..a2a95a4938f 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -19,7 +19,7 @@
     error::{Error, Result},
     irq::InterruptSource,
     prelude::*,
-    qom::{ObjectClass, ObjectImpl, Owned},
+    qom::{ObjectClass, ObjectImpl, Owned, ParentInit},
     vmstate::VMStateDescription,
 };
 
@@ -247,15 +247,9 @@ unsafe impl ObjectType for DeviceState {
 }
 qom_isa!(DeviceState: Object);
 
-/// Trait for methods exposed by the [`DeviceState`] class.  The methods can be
-/// called on all objects that have the trait `IsA<DeviceState>`.
-///
-/// The trait should only be used through the blanket implementation,
-/// which guarantees safety via `IsA`.
-pub trait DeviceMethods: ObjectDeref
-where
-    Self::Target: IsA<DeviceState>,
-{
+/// Initialization methods take a [`ParentInit`] and can be called as
+/// associated functions.
+impl DeviceState {
     /// Add an input clock named `name`.  Invoke the callback with
     /// `self` as the first parameter for the events that are requested.
     ///
@@ -266,12 +260,15 @@ pub trait DeviceMethods: ObjectDeref
     /// which Rust code has a reference to a child object) it would be
     /// possible for this function to return a `&Clock` too.
     #[inline]
-    fn init_clock_in<F: for<'a> FnCall<(&'a Self::Target, ClockEvent)>>(
-        &self,
+    pub fn init_clock_in<T: DeviceImpl, F: for<'a> FnCall<(&'a T, ClockEvent)>>(
+        this: &mut ParentInit<T>,
         name: &str,
         _cb: &F,
         events: ClockEvent,
-    ) -> Owned<Clock> {
+    ) -> Owned<Clock>
+    where
+        T::ParentType: IsA<DeviceState>,
+    {
         fn do_init_clock_in(
             dev: &DeviceState,
             name: &str,
@@ -287,10 +284,10 @@ fn do_init_clock_in(
             unsafe {
                 let cstr = CString::new(name).unwrap();
                 let clk = bindings::qdev_init_clock_in(
-                    dev.as_mut_ptr(),
+                    dev.0.as_mut_ptr(),
                     cstr.as_ptr(),
                     cb,
-                    dev.as_void_ptr(),
+                    dev.0.as_void_ptr(),
                     events.0,
                 );
 
@@ -307,12 +304,12 @@ fn do_init_clock_in(
                 // SAFETY: the opaque is "this", which is indeed a pointer to T
                 F::call((unsafe { &*(opaque.cast::<T>()) }, event))
             }
-            Some(rust_clock_cb::<Self::Target, F>)
+            Some(rust_clock_cb::<T, F>)
         } else {
             None
         };
 
-        do_init_clock_in(self.upcast(), name, cb, events)
+        do_init_clock_in(unsafe { this.upcast_mut() }, name, cb, events)
     }
 
     /// Add an output clock named `name`.
@@ -324,16 +321,32 @@ fn do_init_clock_in(
     /// which Rust code has a reference to a child object) it would be
     /// possible for this function to return a `&Clock` too.
     #[inline]
-    fn init_clock_out(&self, name: &str) -> Owned<Clock> {
+    pub fn init_clock_out<T: DeviceImpl>(this: &mut ParentInit<T>, name: &str) -> Owned<Clock>
+    where
+        T::ParentType: IsA<DeviceState>,
+    {
         unsafe {
             let cstr = CString::new(name).unwrap();
-            let clk = bindings::qdev_init_clock_out(self.upcast().as_mut_ptr(), cstr.as_ptr());
+            // TODO: introduce a type guaranteeing that the DeviceState part has been
+            // initialized
+            let dev: &mut DeviceState = this.upcast_mut();
+            let clk = bindings::qdev_init_clock_out(dev.0.as_mut_ptr(), cstr.as_ptr());
 
             let clk: &Clock = Clock::from_raw(clk);
             Owned::from(clk)
         }
     }
+}
 
+/// Trait for methods exposed by the [`DeviceState`] class.  The methods can be
+/// called on all objects that have the trait `IsA<DeviceState>`.
+///
+/// The trait should only be used through the blanket implementation,
+/// which guarantees safety via `IsA`.
+pub trait DeviceMethods: ObjectDeref
+where
+    Self::Target: IsA<DeviceState>,
+{
     fn prop_set_chr(&self, propname: &str, chr: &Owned<Chardev>) {
         assert!(bql_locked());
         let c_propname = CString::new(propname).unwrap();
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 1481ef20f0c..80a50108e65 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -371,12 +371,15 @@ fn deref_mut(&mut self) -> &mut Self::Target {
 }
 
 unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut bindings::Object) {
-    let mut state = NonNull::new(obj).unwrap().cast::<T>();
+    let mut state = NonNull::new(obj).unwrap().cast::<MaybeUninit<T>>();
+
     // 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()(state.as_mut());
+        ParentInit::with(state.as_mut(), |parent_init| {
+            T::INSTANCE_INIT.unwrap()(parent_init);
+        });
     }
 }
 
@@ -643,7 +646,7 @@ pub trait ObjectImpl: ObjectType + IsA<Object> {
     ///
     /// 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;
+    const INSTANCE_INIT: Option<unsafe fn(ParentInit<Self>)> = None;
 
     /// Function that is called to finish initialization of an object, once
     /// `INSTANCE_INIT` functions have been called.
-- 
2.49.0

Re: [PATCH 2/5] rust: hpet: fully initialize object after instance_init

[Zhao Liu]

On Mon, Jun 09, 2025 at 05:44:20PM +0200, Paolo Bonzini wrote:
> Date: Mon,  9 Jun 2025 17:44:20 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 2/5] rust: hpet: fully initialize object after instance_init
> X-Mailer: git-send-email 2.49.0
> 
> The array of BqlRefCell<HPETTimer> is not initialized yet at the
> end of instance_init.  In particular, the "state" field is NonNull
> and therefore it is invalid to have it as zero bytes.
> 
> Note that MaybeUninit is necessary because assigning to self.timers[index]
> would trigger Drop of the old value.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/timer/hpet/src/device.rs | 42 +++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 17 deletions(-)

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

Re: [PATCH 1/5] rust: qemu_api: introduce MaybeUninit field projection

[Zhao Liu]

On Mon, Jun 09, 2025 at 05:44:19PM +0200, Paolo Bonzini wrote:
> Date: Mon,  9 Jun 2025 17:44:19 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/5] rust: qemu_api: introduce MaybeUninit field projection
> X-Mailer: git-send-email 2.49.0
> 
> Add a macro that makes it possible to convert a MaybeUninit<> into
> another MaybeUninit<> for a single field within it.  Furthermore, it is
> possible to use the resulting MaybeUninitField<> in APIs that take the
> parent object, such as memory_region_init_io().
> 
> This allows removing some of the undefined behavior from instance_init()
> functions, though this may not be the definitive implementation.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/meson.build   |  1 +
>  rust/qemu-api/src/lib.rs    |  1 +
>  rust/qemu-api/src/uninit.rs | 85 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 rust/qemu-api/src/uninit.rs

...

> +impl<'a, T, U> Deref for MaybeUninitField<'a, T, U> {
> +    type Target = MaybeUninit<U>;
> +
> +    fn deref(&self) -> &MaybeUninit<U> {
> +        // SAFETY: self.child was obtained by dereferencing a valid mutable
> +        // reference; the content of the memory may be invalid or uninitialized
> +        // but MaybeUninit<_> makes no assumption on it
> +        unsafe { &*(self.child.cast()) }
> +    }
> +}
> +
> +impl<'a, T, U> DerefMut for MaybeUninitField<'a, T, U> {
> +    fn deref_mut(&mut self) -> &mut MaybeUninit<U> {
> +        // SAFETY: self.child was obtained by dereferencing a valid mutable
> +        // reference; the content of the memory may be invalid or uninitialized
> +        // but MaybeUninit<_> makes no assumption on it
> +        unsafe { &mut *(self.child.cast()) }
> +    }
> +}

Nice trick.

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

Re: [PATCH 3/5] rust: qom: introduce ParentInit

[Zhao Liu]

On Mon, Jun 09, 2025 at 05:44:21PM +0200, Paolo Bonzini wrote:
> Date: Mon,  9 Jun 2025 17:44:21 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 3/5] rust: qom: introduce ParentInit
> X-Mailer: git-send-email 2.49.0
> 
> This is a smart pointer for MaybeUninit; it can be upcasted to the
> already-initialized parent classes, or dereferenced to a MaybeUninit
> for the class that is being initialized.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/qom.rs | 86 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 1 deletion(-)

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

Re: [PATCH 4/5] rust: qom: make ParentInit lifetime-invariant

[Paolo Bonzini]

On Thu, Jun 12, 2025 at 11:00 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > +/// It's impossible to escape the `Jail`; `token1` cannot be moved out of the
> > +/// closure:
> > +///
> > +/// ```ignore
> > +/// let x = 42;
> > +/// let escape = Jail::with(&x, |token1| {
> > +///     println!("{}", token1.get());
> > +///     token1
>
> This line will fail to compile (the below comment "// fails to compile" seems
> to indicate that println! will fail):
>
> error: lifetime may not live long enough
>   --> src/main.rs:22:9
>    |
> 20 |     let escape = Jail::with(x, |token1| {
>    |                                 ------- return type of closure is Jail<'2, i32>
>    |                                 |
>    |                                 has type `Jail<'1, i32>`
> 21 |         println!("{}", token1.get());
> 22 |         token1
>    |         ^^^^^^ returning this value requires that `'1` must outlive `'2`

Right, I put it there because '2 lives until the second println!. The
problem is not so much that it's returning token1, it's that the
println uses it.

I can see that it's confusing, maybe:

    // Because "escape" is used after the closure has returned, the
    // compiler cannot find a type for the "let escape" assignment.

Paolo

Re: [PATCH 4/5] rust: qom: make ParentInit lifetime-invariant

[Zhao Liu]

On Mon, Jun 09, 2025 at 05:44:22PM +0200, Paolo Bonzini wrote:
> Date: Mon,  9 Jun 2025 17:44:22 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 4/5] rust: qom: make ParentInit lifetime-invariant
> X-Mailer: git-send-email 2.49.0
> 
> This is the trick that allows the parent-field initializer to be used
> only for the object that it's meant to be initialized.  This way,
> the owner of a MemoryRegion must be the object that embeds it.
> 
> More information is in the comments; it's best explained with a simplified
> example.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/src/qom.rs | 88 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
> index 21c271cd2f9..1481ef20f0c 100644
> --- a/rust/qemu-api/src/qom.rs
> +++ b/rust/qemu-api/src/qom.rs
> @@ -95,6 +95,7 @@
>  use std::{
>      ffi::{c_void, CStr},
>      fmt,
> +    marker::PhantomData,
>      mem::{ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
>      ptr::NonNull,
> @@ -208,12 +209,91 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
>  
>  /// This struct knows that the superclasses of the object have already been
>  /// initialized.
> -pub struct ParentInit<'a, T>(&'a mut MaybeUninit<T>);
> +///
> +/// The declaration of `ParentInit` is.. *"a kind of magic"*.  It uses a
> +/// technique that is found in several crates, the main ones probably being
> +/// `ghost-cell` (in fact it was introduced by the [`GhostCell` paper](https://plv.mpi-sws.org/rustbelt/ghostcell/))
> +/// and `generativity`.

From the paper, I understand this technique should be "branded type". :-)

> +/// The `PhantomData` makes the `ParentInit` type *invariant* with respect to
> +/// the lifetime argument `'init`.  This, together with the `for<'...>` in
> +/// `[ParentInit::with]`, block any attempt of the compiler to be creative when
> +/// operating on types of type `ParentInit` and to extend their lifetimes.  In
> +/// particular, it ensures that the `ParentInit` cannot be made to outlive the
> +/// `rust_instance_init()` function that creates it, and therefore that the
> +/// `&'init T` reference is valid.
> +///
> +/// This implementation of the same concept, without the QOM baggage, can help
> +/// understanding the effect:
> +///
> +/// ```
> +/// use std::marker::PhantomData;
> +///
> +/// #[derive(PartialEq, Eq)]
> +/// pub struct Jail<'closure, T: Copy>(&'closure T, PhantomData<fn(&'closure ()) -> &'closure ()>);
> +///
> +/// impl<'closure, T: Copy> Jail<'closure, T> {
> +///     fn get(&self) -> T {
> +///         *self.0
> +///     }
> +///
> +///     #[inline]
> +///     fn with<U>(v: T, f: impl for<'id> FnOnce(Jail<'id, T>) -> U) -> U {
> +///         let parent_init = Jail(&v, PhantomData);
> +///         f(parent_init)
> +///     }
> +/// }
> +/// ```
> +///
> +/// It's impossible to escape the `Jail`; `token1` cannot be moved out of the
> +/// closure:
> +///
> +/// ```ignore
> +/// let x = 42;
> +/// let escape = Jail::with(&x, |token1| {
> +///     println!("{}", token1.get());
> +///     token1

This line will fail to compile (the below comment "// fails to compile" seems
to indicate that println! will fail):

error: lifetime may not live long enough
  --> src/main.rs:22:9
   |
20 |     let escape = Jail::with(x, |token1| {
   |                                 ------- return type of closure is Jail<'2, i32>
   |                                 |
   |                                 has type `Jail<'1, i32>`
21 |         println!("{}", token1.get());
22 |         token1
   |         ^^^^^^ returning this value requires that `'1` must outlive `'2`


Referring to GhostToken::new() [*], it said:

        // Return the result of running `f`.  Note that the `GhostToken` itself
        // cannot be returned, because `R` cannot mention the lifetime `'id`, so
        // the `GhostToken` only exists within its scope.

So this example is good, I think just need to optimize the location of the error hint.

[*]: https://gitlab.mpi-sws.org/FP/ghostcell/-/blob/master/ghostcell/src/lib.rs#L128

> +/// });
> +/// // fails to compile:
> +/// println!("{}", escape.get());
> +/// ```
> +///
> +/// Likewise, in the QOM case the `ParentInit` cannot be moved out of
> +/// `instance_init()`. Without this trick it would be possible to stash a
> +/// `ParentInit` and use it later to access uninitialized memory.
> +///
> +/// Here is another example, showing how separately-created "identities" stay
> +/// isolated:
> +///
> +/// ```ignore
> +/// impl<'closure, T: Copy> Clone for Jail<'closure, T> {
> +///     fn clone(&self) -> Jail<'closure, T> {
> +///         Jail(self.0, PhantomData)
> +///     }
> +/// }
> +///
> +/// fn main() {
> +///     Jail::with(42, |token1| {
> +///         // this works and returns true: the clone has the same "identity"
> +///         println!("{}", token1 == token1.clone());
> +///         Jail::with(42, |token2| {
> +///             // here the outer token remains accessible...
> +///             println!("{}", token1.get());
> +///             // ... but the two are separate: this fails to compile:
> +///             println!("{}", token1 == token2);
> +///         });
> +///     });
> +/// }
> +/// ```
> +pub struct ParentInit<'init, T>(
> +    &'init mut MaybeUninit<T>,
> +    PhantomData<fn(&'init ()) -> &'init ()>,
> +);
>  
> -impl<'a, T> ParentInit<'a, T> {
> +impl<'init, T> ParentInit<'init, T> {
>      #[inline]
> -    pub fn with(obj: &'a mut MaybeUninit<T>, f: impl FnOnce(ParentInit<'a, T>)) {
> -        let parent_init = ParentInit(obj);
> +    pub fn with(obj: &'init mut MaybeUninit<T>, f: impl for<'id> FnOnce(ParentInit<'id, T>)) {
> +        let parent_init = ParentInit(obj, PhantomData);

I think it's also valuable to add the similar comment as GhostToken did,
mentioning this `f` can't reture ParentInit itself.

>          f(parent_init)
>      }
>  }
> -- 
> 2.49.0
> 

Nice comment and nice reference (learned a lot).

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

Re: [PATCH 4/5] rust: qom: make ParentInit lifetime-invariant

[Paolo Bonzini]

On Thu, Jun 12, 2025 at 11:20 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > Right, I put it there because '2 lives until the second println!. The
> > problem is not so much that it's returning token1, it's that the
> > println uses it.
>
> Even after I comment out the last intln line, the compiler still
> complains about returning token1. It seems the compiler's checking is
> stricter.

Oh, you're right. Anything that the closure returns must have a longer
lifetime than the closure itself!

Paolo

Re: [PATCH 4/5] rust: qom: make ParentInit lifetime-invariant

[Zhao Liu]

On Thu, Jun 12, 2025 at 11:07:51AM +0200, Paolo Bonzini wrote:
> Date: Thu, 12 Jun 2025 11:07:51 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 4/5] rust: qom: make ParentInit lifetime-invariant
> 
> On Thu, Jun 12, 2025 at 11:00 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > > +/// It's impossible to escape the `Jail`; `token1` cannot be moved out of the
> > > +/// closure:
> > > +///
> > > +/// ```ignore
> > > +/// let x = 42;
> > > +/// let escape = Jail::with(&x, |token1| {
> > > +///     println!("{}", token1.get());
> > > +///     token1
> >
> > This line will fail to compile (the below comment "// fails to compile" seems
> > to indicate that println! will fail):
> >
> > error: lifetime may not live long enough
> >   --> src/main.rs:22:9
> >    |
> > 20 |     let escape = Jail::with(x, |token1| {
> >    |                                 ------- return type of closure is Jail<'2, i32>
> >    |                                 |
> >    |                                 has type `Jail<'1, i32>`
> > 21 |         println!("{}", token1.get());
> > 22 |         token1
> >    |         ^^^^^^ returning this value requires that `'1` must outlive `'2`
> 
> Right, I put it there because '2 lives until the second println!. The
> problem is not so much that it's returning token1, it's that the
> println uses it.

Even after I comment out the last intln line, the compiler still
complains about returning token1. It seems the compiler's checking is
stricter.

I tried at there:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1a9232532250b4a275638b926d7e65e5

Thanks,
Zhao

> I can see that it's confusing, maybe:
> 
>     // Because "escape" is used after the closure has returned, the
>     // compiler cannot find a type for the "let escape" assignment.
> 
> Paolo
> 

Re: [PATCH 4/5] rust: qom: make ParentInit lifetime-invariant

[Zhao Liu]

> > -impl<'a, T> ParentInit<'a, T> {
> > +impl<'init, T> ParentInit<'init, T> {
> >      #[inline]
> > -    pub fn with(obj: &'a mut MaybeUninit<T>, f: impl FnOnce(ParentInit<'a, T>)) {
> > -        let parent_init = ParentInit(obj);
> > +    pub fn with(obj: &'init mut MaybeUninit<T>, f: impl for<'id> FnOnce(ParentInit<'id, T>)) {
> > +        let parent_init = ParentInit(obj, PhantomData);
> 
> I think it's also valuable to add the similar comment as GhostToken did,
> mentioning this `f` can't reture ParentInit itself.

My bad, I forgot there's no returned value...

> >          f(parent_init)
> >      }
> >  }

Re: [PATCH 5/5] rust: qom: change instance_init to take a ParentInit<>

[Zhao Liu]

On Mon, Jun 09, 2025 at 05:44:23PM +0200, Paolo Bonzini wrote:
> Date: Mon,  9 Jun 2025 17:44:23 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 5/5] rust: qom: change instance_init to take a ParentInit<>
> X-Mailer: git-send-email 2.49.0
> 
> This removes undefined behavior associated to writing to uninitialized
> fields, and makes it possible to remove "unsafe" from the instance_init
> implementation.
> 
> However, the init function itself is still unsafe, because it must promise
> (as a sort as MaybeUninit::assume_init) that all fields have been
> initialized.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 34 ++++++++++-----------
>  rust/hw/timer/hpet/src/device.rs | 16 ++++------
>  rust/qemu-api/src/memory.rs      | 12 ++++----
>  rust/qemu-api/src/qdev.rs        | 51 ++++++++++++++++++++------------
>  rust/qemu-api/src/qom.rs         |  9 ++++--
>  5 files changed, 65 insertions(+), 57 deletions(-)

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