Add a smart pointer that allows to add and remove references from
QOM objects.  It's important to note that while all QOM objects have a
reference count, in practice not all of them have their lifetime guarded
by it.  Embedded objects, specifically, are confined to the lifetime of
the owner.

When writing Rust bindings this is important, because embedded objects are
*never* used through the "Owned<>" smart pointer that is introduced here.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/qemu-api/src/qom.rs     | 121 ++++++++++++++++++++++++++++++++++-
 rust/qemu-api/src/vmstate.rs |   6 +-
 2 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 97901fb9084..c404f8a1aa7 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -56,13 +56,21 @@
 use std::{
     ffi::CStr,
     fmt,
+    mem::ManuallyDrop,
     ops::{Deref, DerefMut},
     os::raw::c_void,
+    ptr::NonNull,
 };
 
 pub use bindings::{Object, ObjectClass};
 
-use crate::bindings::{self, object_dynamic_cast, object_get_class, object_get_typename, TypeInfo};
+use crate::{
+    bindings::{
+        self, object_dynamic_cast, object_get_class, object_get_typename, object_ref, object_unref,
+        TypeInfo,
+    },
+    cell::bql_locked,
+};
 
 /// Marker trait: `Self` can be statically upcasted to `P` (i.e. `P` is a direct
 /// or indirect parent of `Self`).
@@ -605,6 +613,110 @@ unsafe impl ObjectType for Object {
         unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_OBJECT) };
 }
 
+/// A reference-counted pointer to a QOM object.
+///
+/// `Owned<T>` wraps `T` with automatic reference counting.  It increases the
+/// reference count when created via [`Owned::from`] and decreases it when
+/// dropped. This ensures that the reference count remains elevated as long as
+/// any `Owned<T>` references to it exist. Note that however the object may
+/// disappear if it is *embedded* within a larger object. For more information
+/// see the [`Owned::from`].
+///
+/// It is *not* safe to send `Owned<T>` to another thread, because currently
+/// `object_unref` requires the Big QEMU Lock.
+#[repr(transparent)]
+#[derive(PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub struct Owned<T: ObjectType>(NonNull<T>);
+
+// SAFETY: the implementation asserts via `bql_locked` that the BQL is taken
+unsafe impl<T: ObjectType + Sync> Sync for Owned<T> {}
+
+impl<T: ObjectType> Owned<T> {
+    /// Convert a raw C pointer into an owned reference to the QOM
+    /// object it points to.  The object's reference count will be
+    /// decreased when the `Owned` is dropped.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `ptr` is NULL.
+    ///
+    /// # Safety
+    ///
+    /// The caller must indeed own a reference to the QOM object.
+    /// The object must not be embedded in another unless the outer
+    /// object is guaranteed to have a longer lifetime.
+    ///
+    /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
+    /// back to `from_raw()` (assuming the original `Owned` was valid!),
+    /// since the owned reference remains there between the calls to
+    /// `into_raw()` and `from_raw()`.
+    #[allow(clippy::missing_const_for_fn)]
+    pub unsafe fn from_raw(ptr: *const T) -> Self {
+        // SAFETY NOTE: while NonNull requires a mutable pointer, only
+        // Deref is implemented so the pointer passed to from_raw
+        // remains const
+        Owned(NonNull::new(ptr as *mut T).unwrap())
+    }
+
+    /// Obtain a raw C pointer from a reference.  `src` is consumed
+    /// and the reference is leaked.
+    #[allow(clippy::missing_const_for_fn)]
+    pub fn into_raw(src: Owned<T>) -> *mut T {
+        let src = ManuallyDrop::new(src);
+        src.0.as_ptr()
+    }
+
+    /// Increase the reference count of a QOM object and return
+    /// a new owned reference to it.
+    ///
+    /// # Safety
+    ///
+    /// The object must not be embedded in another, unless the outer
+    /// object is guaranteed to have a longer lifetime.
+    pub unsafe fn from(obj: &T) -> Self {
+        unsafe {
+            object_ref(obj.as_object_mut_ptr().cast::<c_void>());
+
+            // SAFETY NOTE: while NonNull requires a mutable pointer, only
+            // Deref is implemented so the reference passed to from_raw
+            // remains shared
+            Owned(NonNull::new_unchecked(obj.as_mut_ptr()))
+        }
+    }
+}
+
+impl<T: ObjectType> Deref for Owned<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: creation method is unsafe; whoever calls it has
+        // responsibility that the pointer is valid, and remains valid
+        // throughout the lifetime of the `Owned<T>`.
+        // With that guarantee, reference counting ensures that
+        // the object remains alive.
+        unsafe { &*self.0.as_ptr() }
+    }
+}
+impl<T: ObjectType> ObjectDeref for Owned<T> {}
+
+impl<T: ObjectType> Drop for Owned<T> {
+    fn drop(&mut self) {
+        assert!(bql_locked());
+        // SAFETY: creation method is unsafe, and whoever calls it has
+        // responsibility that the pointer is valid, and remains valid
+        // throughout the lifetime of the `Owned<T>`.
+        unsafe {
+            object_unref(self.as_object_mut_ptr().cast::<c_void>());
+        }
+    }
+}
+
+impl<T: IsA<Object>> fmt::Debug for Owned<T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        self.deref().debug_fmt(f)
+    }
+}
+
 /// Trait for methods exposed by the Object class.  The methods can be
 /// called on all objects that have the trait `IsA<Object>`.
 ///
@@ -636,6 +748,13 @@ fn get_class(&self) -> &'static <Self::Target as ObjectType>::Class {
 
         klass
     }
+
+    /// Convenience function for implementing the Debug trait
+    fn debug_fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.debug_tuple(&self.typename())
+            .field(&(self as *const Self))
+            .finish()
+    }
 }
 
 impl<R: ObjectDeref> ObjectMethods for R where R::Target: IsA<Object> {}
diff --git a/rust/qemu-api/src/vmstate.rs b/rust/qemu-api/src/vmstate.rs
index 6ac432cf52f..11d21b8791c 100644
--- a/rust/qemu-api/src/vmstate.rs
+++ b/rust/qemu-api/src/vmstate.rs
@@ -29,6 +29,8 @@
 pub use crate::bindings::{VMStateDescription, VMStateField};
 use crate::{
     bindings::{self, VMStateFlags},
+    prelude::*,
+    qom::Owned,
     zeroable::Zeroable,
 };
 
@@ -191,7 +193,8 @@ pub const fn vmstate_varray_flag<T: VMState>(_: PhantomData<T>) -> VMStateFlags
 /// * a transparent wrapper for any of the above (`Cell`, `UnsafeCell`,
 ///   [`BqlCell`](crate::cell::BqlCell), [`BqlRefCell`](crate::cell::BqlRefCell)
 /// * a raw pointer to any of the above
-/// * a `NonNull` pointer or a `Box` for any of the above
+/// * a `NonNull` pointer, a `Box` or an [`Owned`](crate::qom::Owned) for any of
+///   the above
 /// * an array of any of the above
 ///
 /// In order to support other types, the trait `VMState` must be implemented
@@ -398,6 +401,7 @@ unsafe impl<$base> VMState for $type where $base: VMState $($where)* {
 // Unlike C pointers, Box is always non-null therefore there is no need
 // to specify VMS_ALLOC.
 impl_vmstate_pointer!(Box<T> where T: VMState);
+impl_vmstate_pointer!(Owned<T> where T: VMState + ObjectType);
 
 // Arrays using the underlying type's VMState plus
 // VMS_ARRAY/VMS_ARRAY_OF_POINTER
-- 
2.47.1

> +impl<T: ObjectType> Owned<T> {
> +    /// Convert a raw C pointer into an owned reference to the QOM
> +    /// object it points to.  The object's reference count will be
> +    /// decreased when the `Owned` is dropped.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if `ptr` is NULL.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must indeed own a reference to the QOM object.
> +    /// The object must not be embedded in another unless the outer
> +    /// object is guaranteed to have a longer lifetime.
> +    ///
> +    /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
> +    /// back to `from_raw()` (assuming the original `Owned` was valid!),
> +    /// since the owned reference remains there between the calls to
> +    /// `into_raw()` and `from_raw()`.
> +    #[allow(clippy::missing_const_for_fn)]
> +    pub unsafe fn from_raw(ptr: *const T) -> Self {
> +        // SAFETY NOTE: while NonNull requires a mutable pointer, only
> +        // Deref is implemented so the pointer passed to from_raw
> +        // remains const
> +        Owned(NonNull::new(ptr as *mut T).unwrap())
> +    }

...

> +    /// Increase the reference count of a QOM object and return
> +    /// a new owned reference to it.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The object must not be embedded in another, unless the outer
> +    /// object is guaranteed to have a longer lifetime.
> +    pub unsafe fn from(obj: &T) -> Self {
> +        unsafe {
> +            object_ref(obj.as_object_mut_ptr().cast::<c_void>());
> +
> +            // SAFETY NOTE: while NonNull requires a mutable pointer, only
> +            // Deref is implemented so the reference passed to from_raw
> +            // remains shared
> +            Owned(NonNull::new_unchecked(obj.as_mut_ptr()))
> +        }
> +    }
> +}
> +

About the difference between from_raw() and from(), I understand if the
C side also holds a pointer, the Rust side must increase the reference
count (using Owned::from), and If the C side does not have any other
pointers, Rust can directly use Owned::from_raw. Am I right?

* The use of from():

        fn do_init_clock_in(
            dev: *mut DeviceState,
            name: &str,
            cb: Option<unsafe extern "C" fn(*mut c_void, ClockEvent)>,
            events: ClockEvent,
        ) -> Owned<Clock> {
            assert!(bql_locked());

            // SAFETY: the clock is heap allocated and does not have a reference, so
            // Owned::from adds one.  the callback is disabled automatically
            // when the clock is unparented, which happens before the device is
            // finalized.
            unsafe {
                let cstr = CString::new(name).unwrap();
                let clk = bindings::qdev_init_clock_in(
                    dev,
                    cstr.as_ptr(),
                    cb,
                    dev.cast::<c_void>(),
                    events.0,
                );

                Owned::from(&*clk)
            }
        }

* The use of from_raw():

    fn new() -> Owned<Self> {
        assert!(bql_locked());
        // SAFETY: the object created by object_new is allocated on
        // the heap and has a reference count of 1
        unsafe {
            let obj = &*object_new(Self::TYPE_NAME.as_ptr());
            Owned::from_raw(obj.unsafe_cast::<Self>())
        }
    }


Comparing with these 2 use cases, I find the difference is
qdev_init_clock_in() creates a pointer in qdev_init_clocklist().

Then the comment "the clock is heap allocated and does not have
a reference" sounds like a conflict. I'm sure I'm missing something. :-(

Thanks,
Zhao

On Mon, Jan 27, 2025 at 8:38 AM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> > +impl<T: ObjectType> Owned<T> {
> > +    /// Convert a raw C pointer into an owned reference to the QOM
> > +    /// object it points to.  The object's reference count will be
> > +    /// decreased when the `Owned` is dropped.
> > +    ///
> > +    /// # Panics
> > +    ///
> > +    /// Panics if `ptr` is NULL.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must indeed own a reference to the QOM object.
> > +    /// The object must not be embedded in another unless the outer
> > +    /// object is guaranteed to have a longer lifetime.
> > +    ///
> > +    /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
> > +    /// back to `from_raw()` (assuming the original `Owned` was valid!),
> > +    /// since the owned reference remains there between the calls to
> > +    /// `into_raw()` and `from_raw()`.
> > +    #[allow(clippy::missing_const_for_fn)]
> > +    pub unsafe fn from_raw(ptr: *const T) -> Self {
> > +        // SAFETY NOTE: while NonNull requires a mutable pointer, only
> > +        // Deref is implemented so the pointer passed to from_raw
> > +        // remains const
> > +        Owned(NonNull::new(ptr as *mut T).unwrap())
> > +    }
>
> ...
>
> > +    /// Increase the reference count of a QOM object and return
> > +    /// a new owned reference to it.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The object must not be embedded in another, unless the outer
> > +    /// object is guaranteed to have a longer lifetime.
> > +    pub unsafe fn from(obj: &T) -> Self {
> > +        unsafe {
> > +            object_ref(obj.as_object_mut_ptr().cast::<c_void>());
> > +
> > +            // SAFETY NOTE: while NonNull requires a mutable pointer, only
> > +            // Deref is implemented so the reference passed to from_raw
> > +            // remains shared
> > +            Owned(NonNull::new_unchecked(obj.as_mut_ptr()))
> > +        }
> > +    }
> > +}
> > +
>
> About the difference between from_raw() and from(), I understand if the
> C side also holds a pointer, the Rust side must increase the reference
> count (using Owned::from), and If the C side does not have any other
> pointers, Rust can directly use Owned::from_raw. Am I right?

Pretty much - more precisely you use Object::from_raw 1) if the C side
gifts a reference 2) if you got the pointer from Owned::into_raw. The
second case is similar to Arc::from_raw, which expects that you got a
reference from Arc::into_raw. The first is the more common case.

>
> * The use of from():
>
>                 let clk = bindings::qdev_init_clock_in(...)
>                 Owned::from(&*clk)

In this case the C side wants to manage the reference that
qdev_init_clock_in() returns; it is dropped in
qdev_finalize_clocklist(). So Rust code needs to increase the
refcount.

> * The use of from_raw():
>
>     fn new() -> Owned<Self> {
>         assert!(bql_locked());
>         // SAFETY: the object created by object_new is allocated on
>         // the heap and has a reference count of 1
>         unsafe {
>             let obj = &*object_new(Self::TYPE_NAME.as_ptr());
>             Owned::from_raw(obj.unsafe_cast::<Self>())
>         }
>     }

In this case the C side lets the caller manage the (only) reference
when object_new returns, so you must not increase the refcount.

Owned::from() is slightly less efficient, though that almost never
matters. If it does you can use ManuallyDrop::new(Owned::from_raw(p)).

> Comparing with these 2 use cases, I find the difference is
> qdev_init_clock_in() creates a pointer in qdev_init_clocklist().

That is related, but more precisely the difference is that
qdev_init_clock_in() wants to unref that pointer later.

> Then the comment "the clock is heap allocated and does not have
> a reference" sounds like a conflict. I'm sure I'm missing something. :-(

Changed:

      // SAFETY: the clock is heap allocated, but qdev_init_clock_in()
      // does not gift the reference to its caller; so use Owned::from to
      // add one.  the callback is disabled automatically when the clock
      // is unparented, which happens before the device is finalized.


Thanks for the review!

Paolo

> > * The use of from():
> >
> >                 let clk = bindings::qdev_init_clock_in(...)
> >                 Owned::from(&*clk)
> 
> In this case the C side wants to manage the reference that
> qdev_init_clock_in() returns; it is dropped in
> qdev_finalize_clocklist(). So Rust code needs to increase the
> refcount.

Pls forgive me for one more question about qdev_init_clock_in() on the C
side. :-)

qdev_init_clock_in() didn't unref `clk` after object_property_add_child(),
so it is intentional, to make the ref count of `clk` be 2:
 * 1 count is held by clocklist until qdev_finalize_clocklist().
 * another 1 is held by its parent via QOM Child<>.

Am I understanding it correctly?

> > Then the comment "the clock is heap allocated and does not have
> > a reference" sounds like a conflict. I'm sure I'm missing something. :-(
> 
> Changed:
> 
>       // SAFETY: the clock is heap allocated, but qdev_init_clock_in()
>       // does not gift the reference to its caller; so use Owned::from to
>       // add one.  the callback is disabled automatically when the clock
>       // is unparented, which happens before the device is finalized.

LGTM.

Thank you very much for your patience. I think I understand ref count
now.

Regards,
Zhao

On 2/5/25 10:13, Zhao Liu wrote:
>>> * The use of from():
>>>
>>>                  let clk = bindings::qdev_init_clock_in(...)
>>>                  Owned::from(&*clk)
>>
>> In this case the C side wants to manage the reference that
>> qdev_init_clock_in() returns; it is dropped in
>> qdev_finalize_clocklist(). So Rust code needs to increase the
>> refcount.
> 
> Pls forgive me for one more question about qdev_init_clock_in() on the C
> side. :-)
> 
> qdev_init_clock_in() didn't unref `clk` after object_property_add_child(),
> so it is intentional, to make the ref count of `clk` be 2:
>   * 1 count is held by clocklist until qdev_finalize_clocklist().
>   * another 1 is held by its parent via QOM Child<>.
> 
> Am I understanding it correctly?

Yes, that's more precise.  In Rust it will be 3, the two above plus the 
Owned<Clock>.

Ah wait... qdev_finalize_clocklist() is only called _after_ the Rust 
struct is Drop::drop-ped, because device_finalize() is called after the 
subclass's instance_finalize.

So the result of qdev_init_clock_in(), strictly speaking, does not have 
to be an Owned<Clock>.  It can also be a &'device Clock; either is 
possible.  Would you prefer that, or do you think it's enough to add a 
comment?

Paolo

>>> Then the comment "the clock is heap allocated and does not have
>>> a reference" sounds like a conflict. I'm sure I'm missing something. :-(
>>
>> Changed:
>>
>>        // SAFETY: the clock is heap allocated, but qdev_init_clock_in()
>>        // does not gift the reference to its caller; so use Owned::from to
>>        // add one.  the callback is disabled automatically when the clock
>>        // is unparented, which happens before the device is finalized.
> 
> LGTM.
> 
> Thank you very much for your patience. I think I understand ref count
> now.
> 
> Regards,
> Zhao
> 
> 
> 
>

On Wed, Feb 05, 2025 at 10:10:01AM +0100, Paolo Bonzini wrote:
> Date: Wed, 5 Feb 2025 10:10:01 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 02/10] rust: qom: add reference counting functionality
> 
> On 2/5/25 10:13, Zhao Liu wrote:
> > > > * The use of from():
> > > > 
> > > >                  let clk = bindings::qdev_init_clock_in(...)
> > > >                  Owned::from(&*clk)
> > > 
> > > In this case the C side wants to manage the reference that
> > > qdev_init_clock_in() returns; it is dropped in
> > > qdev_finalize_clocklist(). So Rust code needs to increase the
> > > refcount.
> > 
> > Pls forgive me for one more question about qdev_init_clock_in() on the C
> > side. :-)
> > 
> > qdev_init_clock_in() didn't unref `clk` after object_property_add_child(),
> > so it is intentional, to make the ref count of `clk` be 2:
> >   * 1 count is held by clocklist until qdev_finalize_clocklist().
> >   * another 1 is held by its parent via QOM Child<>.
> > 
> > Am I understanding it correctly?
> 
> Yes, that's more precise.  In Rust it will be 3, the two above plus the
> Owned<Clock>.

Thanks!

> Ah wait... qdev_finalize_clocklist() is only called _after_ the Rust struct
> is Drop::drop-ped, because device_finalize() is called after the subclass's
> instance_finalize.
> 
> So the result of qdev_init_clock_in(), strictly speaking, does not have to
> be an Owned<Clock>.  It can also be a &'device Clock; either is possible.
> Would you prefer that, or do you think it's enough to add a comment?
> 

I prefer the latter, i.e., keep current Owned<Clock> and add a comment
to mention something like "Although strictly speaking, it does not have
to be an Owned<Clock>. Owned<> is still worth favor to use to protect
Rust code from FFI. When unsure whether to use Owned<>, then use Owned<>."

-Zhao

Hi Paolo,

On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 20:39:55 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> X-Mailer: git-send-email 2.47.1
> 
> Add a smart pointer that allows to add and remove references from
> QOM objects.  It's important to note that while all QOM objects have a
> reference count, in practice not all of them have their lifetime guarded
> by it.

About the background, I have a maybe common question...why Rust needs
extra reference count guarding?

For C side, I notice for child objects, which may be totally embedded in
parent object, or may be pointed to by a pointer member in parent object
(like pl011's clock), they usually become the Child<> property of their
parents by object_initialize_child() (for embedded child) or
object_property_add_child() (for child pointer).

And both these 2 interfaces will increase the ref count in
object_property_try_add_child(). With ref count increasing, it seems
that the Child<> property also express the meaning like "the child
object is 'owned' by its parent".

So, what are the benefits of `Owned` when we also creates Child<>
relationship?

Additionally, I felt that the ref count may be a bit confusing. After
creating Child<> property, the child object's ref count is sometimes 1,
and other times it's 2:

 * With object_initialize_child(), child's ref count is 1.

 * With object_property_add_child() (usually after a object_new() to
   create child first):

   - sometimes user will call object_unref(), and then the ref count is 1.
     E.g., x86_cpu_apic_create() in target/i386/cpu-apic.c.

   - sometimes no object_unref(), then ref count is 2.
     E.g., exynos4210_realize() in hw/arm/exynos4210.c, creats "cortex-a9".

> Embedded objects, specifically, are confined to the lifetime of
> the owner.
> 
> When writing Rust bindings this is important, because embedded objects are
> *never* used through the "Owned<>" smart pointer that is introduced here.

From this description, I understand your goal is:

 * For embedded child object, its lifetimer is managed by its parent
   object, through Child<> for the most cases.

 * For non-embedded child - a pointer/reference in parent object, its
   lifetimer is managed by `Owned<>` (and with Child<>).

Am I right?

Thanks,
Zhao

On Sun, Jan 26, 2025 at 3:56 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> Hi Paolo,
>
> On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> > Date: Fri, 17 Jan 2025 20:39:55 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> > X-Mailer: git-send-email 2.47.1
> >
> > Add a smart pointer that allows to add and remove references from
> > QOM objects.  It's important to note that while all QOM objects have a
> > reference count, in practice not all of them have their lifetime guarded
> > by it.
>
> About the background, I have a maybe common question...why Rust needs
> extra reference count guarding?

Children properties are removed, and thus their reference is dropped,
before instance_finalize() is called (see object_finalize() in
qom/object.c). This is not valid in Rust, you need to keep the object
alive until the last line of Rust code has run - which is after
Drop::drop() has run.

> Additionally, I felt that the ref count may be a bit confusing. After
> creating Child<> property, the child object's ref count is sometimes 1,
> and other times it's 2:
>
>  * With object_initialize_child(), child's ref count is 1.
>
>  * With object_property_add_child() (usually after a object_new() to
>    create child first):
>
>    - sometimes user will call object_unref(), and then the ref count is 1.
>      E.g., x86_cpu_apic_create() in target/i386/cpu-apic.c.
>
>    - sometimes no object_unref(), then ref count is 2.
>      E.g., exynos4210_realize() in hw/arm/exynos4210.c, creats "cortex-a9".

In C, having a ref count of 2 is usually a latent memory leak (because
most of the time there's not going to be an object_unref() in the
instance_finalize() method). In this case the leak is latent, because
TYPE_EXYNOS4210_SOC is not hot-unpluggable and thus will never really
go away once realized.

In Rust, this class of leaks simply does not exist with the right API.
ObjectMethods::property_add_child() could either:

- take an Owned<T> and consume it, thus always giving a ref count of 1
on exit. If you want to keep the object you would have to clone it.

- take "&'owner self, &'child T where 'owner: 'child", then you can
pass an embedded object like object_initialize_child().

In the latter case however you *still* need to keep the reference
count elevated until Drop runs. That is, unlike C, Rust code will
always have a ref count of 2 for children. For this reason, instead of
having a "T" in the struct you would have another wrapper---something
like Child<'owner, T>. This wrapper cannot be cloned but it does an
unref when dropped.

My expectation is that property_add_child() will be used exclusivel
for the first case, i.e. it will take an Owned<T>. If you want to
create a child property from an embedded object, something like
object_initialize_child() can be used once pinned-init is used to
rewrite how instance_init is used. It will look something like

pin_init! {
  &this in MyClass {
    ...,
    iomem <- MemoryRegion::init_io(
            this,
            &MY_MR_OPS,
            "memory-region-name",
            0x1000,
    ),
    child_obj <- ChildClass::init().to_child(this, "prop-name")
  }
}

where to_child() wraps an "impl PinInit<T>" and turns it into an "impl
PinInit<Child<'a, T>>". Or something like that. :)

> From this description, I understand your goal is:
>
>  * For embedded child object, its lifetimer is managed by its parent
>    object, through Child<> for the most cases.
>
>  * For non-embedded child - a pointer/reference in parent object, its
>    lifetimer is managed by `Owned<>` (and with Child<>).
>
> Am I right?

Yes, you're right.

I am not sure if you meant Child<> as the QOM concept, or as a Rust
struct. If the latter, you're really really right.

Paolo

On Wed, Jan 29, 2025 at 11:03:31AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:03:31 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 02/10] rust: qom: add reference counting functionality
> 
> On Sun, Jan 26, 2025 at 3:56 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > Hi Paolo,
> >
> > On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> > > Date: Fri, 17 Jan 2025 20:39:55 +0100
> > > From: Paolo Bonzini <pbonzini@redhat.com>
> > > Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> > > X-Mailer: git-send-email 2.47.1
> > >
> > > Add a smart pointer that allows to add and remove references from
> > > QOM objects.  It's important to note that while all QOM objects have a
> > > reference count, in practice not all of them have their lifetime guarded
> > > by it.
> >
> > About the background, I have a maybe common question...why Rust needs
> > extra reference count guarding?
> 
> Children properties are removed, and thus their reference is dropped,
> before instance_finalize() is called (see object_finalize() in
> qom/object.c). This is not valid in Rust, you need to keep the object
> alive until the last line of Rust code has run - which is after
> Drop::drop() has run.

I see, this is also a typical effort to eliminate unsafe crossing of FFI
boundaries.

> > Additionally, I felt that the ref count may be a bit confusing. After
> > creating Child<> property, the child object's ref count is sometimes 1,
> > and other times it's 2:
> >
> >  * With object_initialize_child(), child's ref count is 1.
> >
> >  * With object_property_add_child() (usually after a object_new() to
> >    create child first):
> >
> >    - sometimes user will call object_unref(), and then the ref count is 1.
> >      E.g., x86_cpu_apic_create() in target/i386/cpu-apic.c.
> >
> >    - sometimes no object_unref(), then ref count is 2.
> >      E.g., exynos4210_realize() in hw/arm/exynos4210.c, creats "cortex-a9".
> 
> In C, having a ref count of 2 is usually a latent memory leak (because
> most of the time there's not going to be an object_unref() in the
> instance_finalize() method). In this case the leak is latent, because
> TYPE_EXYNOS4210_SOC is not hot-unpluggable and thus will never really
> go away once realized.

Further, what about doing object_unref() in object_property_add_child()
or object_property_try_add_child()?

Then we can ensure the object will have ref count of 1 on the exit of
object_property_add_child() and people will no longer miss
object_unref().

Although, there are a few more devices involved to fix similar issues.

> In Rust, this class of leaks simply does not exist with the right API.
> ObjectMethods::property_add_child() could either:
> 
> - take an Owned<T> and consume it, thus always giving a ref count of 1
> on exit. If you want to keep the object you would have to clone it.
>
> - take "&'owner self, &'child T where 'owner: 'child", then you can
> pass an embedded object like object_initialize_child().
> 
> In the latter case however you *still* need to keep the reference
> count elevated until Drop runs. That is, unlike C, Rust code will
> always have a ref count of 2 for children. For this reason, instead of
> having a "T" in the struct you would have another wrapper---something
> like Child<'owner, T>. This wrapper cannot be cloned but it does an
> unref when dropped.

Thanks, the whole picture is nice.

> My expectation is that property_add_child() will be used exclusivel
> for the first case, i.e. it will take an Owned<T>. If you want to
> create a child property from an embedded object, something like
> object_initialize_child() can be used once pinned-init is used to
> rewrite how instance_init is used. It will look something like
> 
> pin_init! {
>   &this in MyClass {
>     ...,
>     iomem <- MemoryRegion::init_io(
>             this,
>             &MY_MR_OPS,
>             "memory-region-name",
>             0x1000,
>     ),
>     child_obj <- ChildClass::init().to_child(this, "prop-name")
>   }
> }
> 
> where to_child() wraps an "impl PinInit<T>" and turns it into an "impl
> PinInit<Child<'a, T>>". Or something like that. :)

Elegant code design, looking forward to pin_init.

> > From this description, I understand your goal is:
> >
> >  * For embedded child object, its lifetimer is managed by its parent
> >    object, through Child<> for the most cases.
> >
> >  * For non-embedded child - a pointer/reference in parent object, its
> >    lifetimer is managed by `Owned<>` (and with Child<>).
> >
> > Am I right?
> 
> Yes, you're right.
> 
> I am not sure if you meant Child<> as the QOM concept, or as a Rust
> struct. If the latter, you're really really right.
> 

Thank you :-) It seems virtio device will have an embedded case to adopt
Child<> struct (virtio_instance_init_common()).

Regards,
Zhao