[PATCH 0/2] qom: Tighten object lifetime handling

[PATCH 0/2] qom: Tighten object lifetime handling

[Akihiko Odaki]

QOM has extensive assertions and prevents most cases that would trigger
undefined behavior, but two corner cases still need explicit handling:
temporary object resurrection during finalization, and references to
embedded objects. This series covers both cases.

The temporary object resurrection patch originated from:
https://lore.kernel.org/qemu-devel/20250906-mr-v2-1-2820f5a3d282@rsg.ci.i.u-tokyo.ac.jp/
("[PATCH v2 1/3] qom: Do not finalize twice")

I also plan to make the embedded object reference handling a
prerequisite for the next version of the following series:
https://lore.kernel.org/qemu-devel/20250917-qom-v1-0-7262db7b0a84@rsg.ci.i.u-tokyo.ac.jp/
("[PATCH 00/35] memory: QOM-ify AddressSpace")

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Akihiko Odaki (2):
      qom: Reject temporary object resurrection
      qom: Manage references to embedded child objects

 include/qom/object.h            | 39 ++++++++++++++++-----
 block/throttle-groups.c         |  2 +-
 qom/object.c                    | 75 +++++++++++++++++++++++++++++++----------
 tests/unit/check-qom-proplist.c |  8 ++---
 rust/qom/src/qom.rs             |  8 ++---
 5 files changed, 98 insertions(+), 34 deletions(-)
---
base-commit: 2db91528542672cf0db78b3f2cc0e22b36302b38
change-id: 20260614-embedded-00ca98fcdb5c

Best regards,
--  
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

[PATCH 1/2] qom: Reject temporary object resurrection

[Akihiko Odaki]

If object_ref() is called during finalization, it will temporarily
"resurrect" the object. Although object_finalize() asserts that no
resurrecting reference remains before freeing the object, the assertion
cannot catch the case where the resurrecting reference is dropped before
freeing the object.

One way to catch this would be to add a check in object_ref() to reject
resurrecting references before they are created. However, object_ref()
is frequently called so it is better to minimize the overhead.

To avoid adding the overhead, change how the reference count is
represented and let an existing assertion detect resurrection. More
concretely, obj->ref now stores the current reference count minus 1. The
stored count is therefore 0 after object_initialize(), and the final
object_unref() decrements it from 0 to UINT32_MAX and starts
finalization. A resurrecting object_ref() will then trip the existing
obj->ref < INT_MAX assertion.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 include/qom/object.h            |  4 ++--
 block/throttle-groups.c         |  2 +-
 qom/object.c                    | 15 ++++++---------
 tests/unit/check-qom-proplist.c |  8 ++++----
 4 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 11f55613fcd0..c4dcf65dcf7f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1121,7 +1121,7 @@ GSList *object_class_get_list_sorted(const char *implements_type,
  * @obj: the object
  *
  * Increase the reference count of a object.  A object cannot be freed as long
- * as its reference count is greater than zero.
+ * as a reference remains.
  * Returns: @obj
  */
 Object *object_ref(void *obj);
@@ -1131,7 +1131,7 @@ Object *object_ref(void *obj);
  * @obj: the object
  *
  * Decrease the reference count of a object.  A object cannot be freed as long
- * as its reference count is greater than zero.
+ * as a reference remains.
  */
 void object_unref(void *obj);
 
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 4b1b1944c20a..3b2a89f5849d 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -962,7 +962,7 @@ static void throttle_group_get_limits(Object *obj, Visitor *v,
 
 static bool throttle_group_can_be_deleted(UserCreatable *uc)
 {
-    return OBJECT(uc)->ref == 1;
+    return OBJECT(uc)->ref == 0;
 }
 
 static void throttle_group_obj_class_init(ObjectClass *klass,
diff --git a/qom/object.c b/qom/object.c
index 0ac201de4c15..761eff1337e6 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -497,7 +497,6 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
 
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
-    object_ref(obj);
     object_class_property_init_all(obj);
     obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
                                             NULL, object_property_free);
@@ -560,11 +559,10 @@ bool object_initialize_child_with_propsv(Object *parentobj,
 
 out:
     /*
-     * We want @obj's reference to be 1 on success, 0 on failure.
-     * On success, it's 2: one taken by object_initialize(), and one
-     * by object_property_add_child().
-     * On failure in object_initialize() or earlier, it's 1.
-     * On failure afterwards, it's also 1: object_unparent() releases
+     * We want @obj's reference to be 0 on success, UINT32_MAX on failure.
+     * On success, it's 1: one taken by object_property_add_child().
+     * On failure in object_initialize() or earlier, it's 0.
+     * On failure afterwards, it's also 0: object_unparent() releases
      * the reference taken by object_property_add_child().
      */
     object_unref(obj);
@@ -668,7 +666,6 @@ static void object_finalize(void *data)
     object_property_del_all(obj);
     object_deinit(obj, ti);
 
-    g_assert(obj->ref == 0);
     g_assert(obj->parent == NULL);
     if (obj->free) {
         obj->free(obj);
@@ -1329,10 +1326,10 @@ void object_unref(void *objptr)
     if (!obj) {
         return;
     }
-    g_assert(obj->ref > 0);
+    g_assert(obj->ref < INT_MAX);
 
     /* parent always holds a reference to its children */
-    if (qatomic_fetch_dec(&obj->ref) == 1) {
+    if (qatomic_fetch_dec(&obj->ref) == 0) {
         object_finalize(obj);
     }
 }
diff --git a/tests/unit/check-qom-proplist.c b/tests/unit/check-qom-proplist.c
index 89de92b7d91f..b5527dd52afd 100644
--- a/tests/unit/check-qom-proplist.c
+++ b/tests/unit/check-qom-proplist.c
@@ -351,7 +351,7 @@ static void test_dummy_createv_tree(void)
                               NULL));
 
     g_assert(err == NULL);
-    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
+    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
     g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
     g_assert(dobj->bv == true);
     g_assert(dobj->av == DUMMY_PLATYPUS);
@@ -375,7 +375,7 @@ static void test_dummy_createv_parentless(void)
                                          NULL));
 
     g_assert(err == NULL);
-    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
+    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
     g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
     g_assert(dobj->bv == true);
     g_assert(dobj->av == DUMMY_PLATYPUS);
@@ -414,7 +414,7 @@ static void test_dummy_createlist_tree(void)
                         NULL));
 
     g_assert(err == NULL);
-    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
+    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
     g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
     g_assert(dobj->bv == true);
     g_assert(dobj->av == DUMMY_PLATYPUS);
@@ -450,7 +450,7 @@ static void test_dummy_createlist_parentless(void)
                               NULL));
 
     g_assert(err == NULL);
-    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
+    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
     g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
     g_assert(dobj->bv == true);
     g_assert(dobj->av == DUMMY_PLATYPUS);

-- 
2.54.0

[PATCH 2/2] qom: Manage references to embedded child objects

[Akihiko Odaki]

Problem
=======

The Rust wrapper for object_ref() is marked unsafe and has:
> The object must not be embedded in another unless the outer
> object is guaranteed to have a longer lifetime.

In other words, object_ref() does not work for embedded objects and
does not keep embedded objects alive. MemoryRegion has its own
memory_region_ref() helper to call object_ref() on its owner for this
reason.

However, this is insufficient to avoid calling object_ref() for all
embedded objects. For example, consider an embedded Device that has a
MemoryRegion. When referencing a MemoryRegion for guest memory access,
QEMU automatically references the owning Device to keep the MemoryRegion
alive. However, that reference is ineffective if the Device itself is
embedded, because object_ref() does not keep the containing storage
alive.

One concrete case is qemu-xhci: XHCIPciState embeds XHCIState as a
child object, and the embedded XHCIState owns the MemoryRegion used for
PCI BAR 0. When memory core references that region, memory_region_ref()
references the embedded XHCIState owner, but that does not keep the
containing XHCIPciState storage alive across runtime unplug.

To avoid such a use-after-free hazard, memory_region_init*() would also
need a SAFETY comment saying "the owner must not be embedded in another
unless its outer object is guaranteed to have a longer lifetime." The
unsafe nature of object_ref() propagates across the codebase and
spreads SAFETY comments across the codebase, and any failure to fulfill
the requirement can cause use-after-free.

Solution
========

Eliminate this whole class of use-after-free hazard by properly managing
references to the "storage" of embedded child objects. Objects now have
a separate storage reference counter for their storage. If an object is
embedded, the reference counter is set to UINT32_MAX. Otherwise, it
represents the number of references to the object's storage - 1.

Object::free is now a union with Object::storage, which holds a
reference to the storage for an embedded object. This forms the
following reference graph:

Parent -> Storage
Parent -> Embedded Child -> Storage

Functions for taking and dropping references to object storage are also
exposed as APIs. This is particularly useful when the storage needs to
be freed asynchronously due to RCU, for example.

A possible concern is that this adds a uint32_t to Object. This is not a
problem in most situations where the host uses 64-bit addressing because
the member is added to a gap needed for alignment. It does increase
memory usage for 32-bit hosts, but proliferation of SAFETY comments to
avoid the overhead is unlikely to be worthwhile.

Note that the Rust wrapper for object_ref() is still marked as unsafe.
This is because an object implemented in C may have pointers whose
lifetimes are not properly extended according to the reference counter.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 include/qom/object.h | 39 +++++++++++++++++++++++++++-------
 qom/object.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++-------
 rust/qom/src/qom.rs  |  8 +++----
 3 files changed, 87 insertions(+), 20 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index c4dcf65dcf7f..b5d656e3758c 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -155,8 +155,12 @@ struct Object
 {
     /* private: */
     ObjectClass *class;
-    ObjectFree *free;
+    union {
+        ObjectFree *free;
+        Object *storage;
+    };
     GHashTable *properties;
+    uint32_t storage_ref;
     uint32_t ref;
     Object *parent;
 };
@@ -613,7 +617,7 @@ struct InterfaceClass
  * @klass: The class to instantiate.
  *
  * This function will initialize a new object using heap allocated memory.
- * The returned object has a reference count of 1, and will be freed when
+ * The returned object has a reference count of 1, and will be finalized when
  * the last reference is dropped.
  *
  * Returns: The newly allocated and instantiated object.
@@ -625,7 +629,7 @@ Object *object_new_with_class(ObjectClass *klass);
  * @typename: The name of the type of the object to instantiate.
  *
  * This function will initialize a new object using heap allocated memory.
- * The returned object has a reference count of 1, and will be freed when
+ * The returned object has a reference count of 1, and will be finalized when
  * the last reference is dropped.
  *
  * Returns: The newly allocated and instantiated object.
@@ -641,7 +645,7 @@ Object *object_new(const char *typename);
  * @...: list of property names and values
  *
  * This function will initialize a new object using heap allocated memory.
- * The returned object has a reference count of 1, and will be freed when
+ * The returned object has a reference count of 1, and will be finalized when
  * the last reference is dropped.
  *
  * The @id parameter will be used when registering the object as a
@@ -1120,8 +1124,8 @@ GSList *object_class_get_list_sorted(const char *implements_type,
  * object_ref:
  * @obj: the object
  *
- * Increase the reference count of a object.  A object cannot be freed as long
- * as a reference remains.
+ * Increase the reference count of a object.  A object cannot be finalized as
+ * long as a reference remains.
  * Returns: @obj
  */
 Object *object_ref(void *obj);
@@ -1130,11 +1134,30 @@ Object *object_ref(void *obj);
  * object_unref:
  * @obj: the object
  *
- * Decrease the reference count of a object.  A object cannot be freed as long
- * as a reference remains.
+ * Decrease the reference count of a object.  A object cannot be finalized as
+ * long as a reference remains.
  */
 void object_unref(void *obj);
 
+/**
+ * object_storage_ref:
+ * @obj: the object
+ *
+ * Increase the reference count of a object's storage.  A object cannot be
+ * freed as long as its storage is referenced.
+ * Returns: the object that provides the storage
+ */
+Object *object_storage_ref(void *obj);
+
+/**
+ * object_storage_unref:
+ * @obj: the object
+ *
+ * Decrease the reference count of a object's storage.  A object cannot be
+ * freed as long as its storage is referenced.
+ */
+void object_storage_unref(void *obj);
+
 /**
  * object_property_try_add:
  * @obj: the object to add a property to
diff --git a/qom/object.c b/qom/object.c
index 761eff1337e6..0a2c7ce9710d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -487,7 +487,8 @@ static void object_class_property_init_all(Object *obj)
     }
 }
 
-static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type)
+static void object_initialize_with_type(Object *obj, size_t size,
+                                        TypeImpl *type, Object *storage)
 {
     type_initialize(type);
 
@@ -496,6 +497,10 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
     g_assert(size >= type->instance_size);
 
     memset(obj, 0, type->instance_size);
+    if (storage) {
+        obj->storage_ref = UINT32_MAX;
+        obj->storage = object_storage_ref(storage);
+    }
     obj->class = type->class;
     object_class_property_init_all(obj);
     obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
@@ -508,7 +513,7 @@ void object_initialize(void *data, size_t size, const char *typename)
 {
     TypeImpl *type = type_get_or_load_by_name(typename, &error_fatal);
 
-    object_initialize_with_type(data, size, type);
+    object_initialize_with_type(data, size, type, NULL);
 }
 
 bool object_initialize_child_with_props(Object *parentobj,
@@ -531,14 +536,15 @@ bool object_initialize_child_with_props(Object *parentobj,
 bool object_initialize_child_with_propsv(Object *parentobj,
                                          const char *propname,
                                          void *childobj, size_t size,
-                                         const char *type,
+                                         const char *typename,
                                          Error **errp, va_list vargs)
 {
+    TypeImpl *type = type_get_or_load_by_name(typename, &error_fatal);
     bool ok = false;
     Object *obj;
     UserCreatable *uc;
 
-    object_initialize(childobj, size, type);
+    object_initialize_with_type(childobj, size, type, parentobj);
     obj = OBJECT(childobj);
 
     if (!object_set_propv(obj, vargs, errp)) {
@@ -667,9 +673,7 @@ static void object_finalize(void *data)
     object_deinit(obj, ti);
 
     g_assert(obj->parent == NULL);
-    if (obj->free) {
-        obj->free(obj);
-    }
+    object_storage_unref(obj);
 }
 
 /* Find the minimum alignment guaranteed by the system malloc. */
@@ -708,7 +712,7 @@ static Object *object_new_with_type(Type type)
         obj_free = qemu_vfree;
     }
 
-    object_initialize_with_type(obj, size, type);
+    object_initialize_with_type(obj, size, type, NULL);
     obj->free = obj_free;
 
     trace_object_new(obj, obj->class->type->name);
@@ -1334,6 +1338,46 @@ void object_unref(void *objptr)
     }
 }
 
+Object *object_storage_ref(void *objptr)
+{
+    Object *obj = OBJECT(objptr);
+    uint32_t ref;
+
+    if (!obj) {
+        return NULL;
+    }
+
+    while (qatomic_read(&obj->storage_ref) == UINT32_MAX) {
+        obj = obj->storage;
+    }
+
+    ref = qatomic_fetch_inc(&obj->storage_ref);
+    /* Assert waaay before the integer overflows */
+    g_assert(ref < INT_MAX);
+    return obj;
+}
+
+void object_storage_unref(void *objptr)
+{
+    Object *obj = OBJECT(objptr);
+    uint32_t ref;
+
+    if (!obj) {
+        return;
+    }
+
+    while (qatomic_read(&obj->storage_ref) == UINT32_MAX) {
+        obj = obj->storage;
+    }
+
+    ref = qatomic_fetch_dec(&obj->storage_ref);
+    g_assert(ref < INT_MAX);
+
+    if (ref == 0 && obj->free) {
+        obj->free(obj);
+    }
+}
+
 ObjectProperty *
 object_property_try_add(Object *obj, const char *name, const char *type,
                         ObjectPropertyAccessor *get,
diff --git a/rust/qom/src/qom.rs b/rust/qom/src/qom.rs
index cc00ddcfc988..34f7fff64083 100644
--- a/rust/qom/src/qom.rs
+++ b/rust/qom/src/qom.rs
@@ -809,8 +809,8 @@ impl<T: ObjectType> Owned<T> {
     /// # 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.
+    /// The object's implementation must guarantee functionality as long as it
+    /// is referenced.
     ///
     /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
     /// back to `from_raw()` (assuming the original `Owned` was valid!),
@@ -836,8 +836,8 @@ pub fn into_raw(src: Owned<T>) -> *mut T {
     ///
     /// # Safety
     ///
-    /// The object must not be embedded in another, unless the outer
-    /// object is guaranteed to have a longer lifetime.
+    /// The object's implementation must guarantee functionality as long as it
+    /// is referenced.
     pub unsafe fn from(obj: &T) -> Self {
         unsafe {
             object_ref(obj.as_object_mut_ptr().cast::<c_void>());

-- 
2.54.0

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Daniel P. Berrangé]

On Mon, Jun 15, 2026 at 01:11:07PM +0900, Akihiko Odaki wrote:
> Problem
> =======
> 
> The Rust wrapper for object_ref() is marked unsafe and has:
> > The object must not be embedded in another unless the outer
> > object is guaranteed to have a longer lifetime.
> 
> In other words, object_ref() does not work for embedded objects and
> does not keep embedded objects alive. MemoryRegion has its own
> memory_region_ref() helper to call object_ref() on its owner for this
> reason.
> 
> However, this is insufficient to avoid calling object_ref() for all
> embedded objects. For example, consider an embedded Device that has a
> MemoryRegion. When referencing a MemoryRegion for guest memory access,
> QEMU automatically references the owning Device to keep the MemoryRegion
> alive. However, that reference is ineffective if the Device itself is
> embedded, because object_ref() does not keep the containing storage
> alive.

Do we know how many examples we have of embedding objects inside
another ?

I would much prefer if we forbid the embedding of objects. It is
horrible design practice to have some QOM objects which can be
freed via reference count and some which cannot.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 1/2] qom: Reject temporary object resurrection

[Daniel P. Berrangé]

On Mon, Jun 15, 2026 at 01:11:06PM +0900, Akihiko Odaki wrote:
> If object_ref() is called during finalization, it will temporarily
> "resurrect" the object. Although object_finalize() asserts that no
> resurrecting reference remains before freeing the object, the assertion
> cannot catch the case where the resurrecting reference is dropped before
> freeing the object.

What's the real world problem here ? 

> 
> One way to catch this would be to add a check in object_ref() to reject
> resurrecting references before they are created. However, object_ref()
> is frequently called so it is better to minimize the overhead.
> 
> To avoid adding the overhead, change how the reference count is
> represented and let an existing assertion detect resurrection. More
> concretely, obj->ref now stores the current reference count minus 1. The
> stored count is therefore 0 after object_initialize(), and the final
> object_unref() decrements it from 0 to UINT32_MAX and starts
> finalization. A resurrecting object_ref() will then trip the existing
> obj->ref < INT_MAX assertion.

Please no, this is horribly surprising semantics.

> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  include/qom/object.h            |  4 ++--
>  block/throttle-groups.c         |  2 +-
>  qom/object.c                    | 15 ++++++---------
>  tests/unit/check-qom-proplist.c |  8 ++++----
>  4 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 11f55613fcd0..c4dcf65dcf7f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1121,7 +1121,7 @@ GSList *object_class_get_list_sorted(const char *implements_type,
>   * @obj: the object
>   *
>   * Increase the reference count of a object.  A object cannot be freed as long
> - * as its reference count is greater than zero.
> + * as a reference remains.
>   * Returns: @obj
>   */
>  Object *object_ref(void *obj);
> @@ -1131,7 +1131,7 @@ Object *object_ref(void *obj);
>   * @obj: the object
>   *
>   * Decrease the reference count of a object.  A object cannot be freed as long
> - * as its reference count is greater than zero.
> + * as a reference remains.
>   */
>  void object_unref(void *obj);
>  
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 4b1b1944c20a..3b2a89f5849d 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -962,7 +962,7 @@ static void throttle_group_get_limits(Object *obj, Visitor *v,
>  
>  static bool throttle_group_can_be_deleted(UserCreatable *uc)
>  {
> -    return OBJECT(uc)->ref == 1;
> +    return OBJECT(uc)->ref == 0;
>  }
>  
>  static void throttle_group_obj_class_init(ObjectClass *klass,
> diff --git a/qom/object.c b/qom/object.c
> index 0ac201de4c15..761eff1337e6 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -497,7 +497,6 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
>  
>      memset(obj, 0, type->instance_size);
>      obj->class = type->class;
> -    object_ref(obj);
>      object_class_property_init_all(obj);
>      obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
>                                              NULL, object_property_free);
> @@ -560,11 +559,10 @@ bool object_initialize_child_with_propsv(Object *parentobj,
>  
>  out:
>      /*
> -     * We want @obj's reference to be 1 on success, 0 on failure.
> -     * On success, it's 2: one taken by object_initialize(), and one
> -     * by object_property_add_child().
> -     * On failure in object_initialize() or earlier, it's 1.
> -     * On failure afterwards, it's also 1: object_unparent() releases
> +     * We want @obj's reference to be 0 on success, UINT32_MAX on failure.
> +     * On success, it's 1: one taken by object_property_add_child().
> +     * On failure in object_initialize() or earlier, it's 0.
> +     * On failure afterwards, it's also 0: object_unparent() releases
>       * the reference taken by object_property_add_child().
>       */
>      object_unref(obj);
> @@ -668,7 +666,6 @@ static void object_finalize(void *data)
>      object_property_del_all(obj);
>      object_deinit(obj, ti);
>  
> -    g_assert(obj->ref == 0);
>      g_assert(obj->parent == NULL);
>      if (obj->free) {
>          obj->free(obj);
> @@ -1329,10 +1326,10 @@ void object_unref(void *objptr)
>      if (!obj) {
>          return;
>      }
> -    g_assert(obj->ref > 0);
> +    g_assert(obj->ref < INT_MAX);
>  
>      /* parent always holds a reference to its children */
> -    if (qatomic_fetch_dec(&obj->ref) == 1) {
> +    if (qatomic_fetch_dec(&obj->ref) == 0) {
>          object_finalize(obj);
>      }
>  }
> diff --git a/tests/unit/check-qom-proplist.c b/tests/unit/check-qom-proplist.c
> index 89de92b7d91f..b5527dd52afd 100644
> --- a/tests/unit/check-qom-proplist.c
> +++ b/tests/unit/check-qom-proplist.c
> @@ -351,7 +351,7 @@ static void test_dummy_createv_tree(void)
>                                NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> @@ -375,7 +375,7 @@ static void test_dummy_createv_parentless(void)
>                                           NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> @@ -414,7 +414,7 @@ static void test_dummy_createlist_tree(void)
>                          NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> @@ -450,7 +450,7 @@ static void test_dummy_createlist_parentless(void)
>                                NULL));
>  
>      g_assert(err == NULL);
> -    g_assert_cmpint(dobj->parent_obj.ref, ==, 1);
> +    g_assert_cmpint(dobj->parent_obj.ref, ==, 0);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
> 
> -- 
> 2.54.0
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Peter Maydell]

On Mon, 15 Jun 2026 at 09:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
> Do we know how many examples we have of embedding objects inside
> another ?

It is an extremely common pattern for device and SoC model
implementations; we've been recommending it for years.

> I would much prefer if we forbid the embedding of objects. It is
> horrible design practice to have some QOM objects which can be
> freed via reference count and some which cannot.

That would be a very large amount of code to rewrite to the
new paradigm. I don't object inherently ("you have pointers to
your child objects" works better when they might be implemented
in Rust and might play better with being able to create
machines and wire them up on the command line); I'm just
noting how much work it would be if you wanted to make
embedding forbidden.

thanks
-- PMM

Re: [PATCH 1/2] qom: Reject temporary object resurrection

[Kevin Wolf]

Am 15.06.2026 um 06:11 hat Akihiko Odaki geschrieben:
> If object_ref() is called during finalization, it will temporarily
> "resurrect" the object. Although object_finalize() asserts that no
> resurrecting reference remains before freeing the object, the assertion
> cannot catch the case where the resurrecting reference is dropped before
> freeing the object.

Ok, and why is that a problem?

In the cover letter, you referred to an earlier patch you had posted:

    The temporary object resurrection patch originated from:
    https://lore.kernel.org/qemu-devel/20250906-mr-v2-1-2820f5a3d282@rsg.ci.i.u-tokyo.ac.jp/
    ("[PATCH v2 1/3] qom: Do not finalize twice")

From that patch, it's clear that one problem is that .finalize() can be
called multiple times, which you probably don't want. But what is the
reason for switching from just ignoring the second .finalize() like in
the old patch to completely forbidding a temporary refcount increase?

> One way to catch this would be to add a check in object_ref() to reject
> resurrecting references before they are created. However, object_ref()
> is frequently called so it is better to minimize the overhead.

We're talking about a single comparison with zero here. So if that's the
worse alternative, the other one can't have any drawbacks.

> To avoid adding the overhead, change how the reference count is
> represented and let an existing assertion detect resurrection. More
> concretely, obj->ref now stores the current reference count minus 1. The
> stored count is therefore 0 after object_initialize(), and the final
> object_unref() decrements it from 0 to UINT32_MAX and starts
> finalization. A resurrecting object_ref() will then trip the existing
> obj->ref < INT_MAX assertion.

A refcount field where 0 means that a last reference is still remaining
is highly confusing. If we do want to forbid taking a temporary new
reference in .finalize() for some reason, the explicit check in
object_ref() sounds much better to me.

> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  include/qom/object.h            |  4 ++--
>  block/throttle-groups.c         |  2 +-
>  qom/object.c                    | 15 ++++++---------
>  tests/unit/check-qom-proplist.c |  8 ++++----
>  4 files changed, 13 insertions(+), 16 deletions(-)

Kevin

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Daniel P. Berrangé]

On Mon, Jun 15, 2026 at 09:31:36AM +0100, Peter Maydell wrote:
> On Mon, 15 Jun 2026 at 09:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > Do we know how many examples we have of embedding objects inside
> > another ?
> 
> It is an extremely common pattern for device and SoC model
> implementations; we've been recommending it for years.
> 
> > I would much prefer if we forbid the embedding of objects. It is
> > horrible design practice to have some QOM objects which can be
> > freed via reference count and some which cannot.
> 
> That would be a very large amount of code to rewrite to the
> new paradigm. I don't object inherently ("you have pointers to
> your child objects" works better when they might be implemented
> in Rust and might play better with being able to create
> machines and wire them up on the command line); I'm just
> noting how much work it would be if you wanted to make
> embedding forbidden.

Would it be a more tractable problem to "fix" object structure only
incrementally as they gain a need to be managed/reference from
Rust code, or does the Rust usage already implicitly extend too
broadly 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 1/2] qom: Reject temporary object resurrection

[Akihiko Odaki]

On 2026/06/15 17:34, Kevin Wolf wrote:
> Am 15.06.2026 um 06:11 hat Akihiko Odaki geschrieben:
>> If object_ref() is called during finalization, it will temporarily
>> "resurrect" the object. Although object_finalize() asserts that no
>> resurrecting reference remains before freeing the object, the assertion
>> cannot catch the case where the resurrecting reference is dropped before
>> freeing the object.
> 
> Ok, and why is that a problem?
> 
> In the cover letter, you referred to an earlier patch you had posted:
> 
>      The temporary object resurrection patch originated from:
>      https://lore.kernel.org/qemu-devel/20250906-mr-v2-1-2820f5a3d282@rsg.ci.i.u-tokyo.ac.jp/
>      ("[PATCH v2 1/3] qom: Do not finalize twice")
> 
>  From that patch, it's clear that one problem is that .finalize() can be
> called multiple times, which you probably don't want. But what is the
> reason for switching from just ignoring the second .finalize() like in
> the old patch to completely forbidding a temporary refcount increase?

None. I thought merely skipping finalization is not a well-defined 
semantics (e.g., breaking Rust safety guarantee), but, reconsidering 
now, I think that's fine.

> 
>> One way to catch this would be to add a check in object_ref() to reject
>> resurrecting references before they are created. However, object_ref()
>> is frequently called so it is better to minimize the overhead.
> 
> We're talking about a single comparison with zero here. So if that's the
> worse alternative, the other one can't have any drawbacks.

True.

> 
>> To avoid adding the overhead, change how the reference count is
>> represented and let an existing assertion detect resurrection. More
>> concretely, obj->ref now stores the current reference count minus 1. The
>> stored count is therefore 0 after object_initialize(), and the final
>> object_unref() decrements it from 0 to UINT32_MAX and starts
>> finalization. A resurrecting object_ref() will then trip the existing
>> obj->ref < INT_MAX assertion.
> 
> A refcount field where 0 means that a last reference is still remaining
> is highly confusing. If we do want to forbid taking a temporary new
> reference in .finalize() for some reason, the explicit check in
> object_ref() sounds much better to me.

The rationale here is that the field is mostly contained in qom/object.c 
so the awkward semantics won't have a wide impact. But it's just 
"mostly" and there are two exceptions: block/throttle-groups.c and 
tests/unit/check-qom-proplist.c.

I withdraw this patch for the drawbacks discussed above.

Regards,
Akihiko Odaki

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Peter Maydell]

On Mon, 15 Jun 2026 at 09:35, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Jun 15, 2026 at 09:31:36AM +0100, Peter Maydell wrote:
> > On Mon, 15 Jun 2026 at 09:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > Do we know how many examples we have of embedding objects inside
> > > another ?
> >
> > It is an extremely common pattern for device and SoC model
> > implementations; we've been recommending it for years.
> >
> > > I would much prefer if we forbid the embedding of objects. It is
> > > horrible design practice to have some QOM objects which can be
> > > freed via reference count and some which cannot.
> >
> > That would be a very large amount of code to rewrite to the
> > new paradigm. I don't object inherently ("you have pointers to
> > your child objects" works better when they might be implemented
> > in Rust and might play better with being able to create
> > machines and wire them up on the command line); I'm just
> > noting how much work it would be if you wanted to make
> > embedding forbidden.
>
> Would it be a more tractable problem to "fix" object structure only
> incrementally as they gain a need to be managed/reference from
> Rust code, or does the Rust usage already implicitly extend too
> broadly

At the moment we have exactly 2 Rust devices. You can see the
workaround we ended up with for the PL011 in commits 5b87a07e768
and cc3d262aa93a4, where we pad out the C device struct and
assert on the Rust side that its version isn't any bigger.

For that particular problem we could say "you need to refactor
all users of device X to not embed it before creating the Rust
version". There is I think only one embedded use of the PL011
(I was actually expecting more). This does have the awkward
effect that anybody wanting to do a "rewrite device in rust"
is now forced into "do a big refactor of some old C code they
don't care about". We would also need to actually document and
provide some good examples of "this is how we think we should
be writing device models that have child objects now".

But embedded MemoryRegions are enormously common, and devices
that embed other devices that embed MemoryRegions are also
pretty common. So I don't know how much of a gradual approach
you could take for the issue descibed in this patch.

thanks
-- PMM

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Daniel P. Berrangé]

On Mon, Jun 15, 2026 at 01:11:07PM +0900, Akihiko Odaki wrote:
> One concrete case is qemu-xhci: XHCIPciState embeds XHCIState as a
> child object, and the embedded XHCIState owns the MemoryRegion used for
> PCI BAR 0. When memory core references that region, memory_region_ref()
> references the embedded XHCIState owner, but that does not keep the
> containing XHCIPciState storage alive across runtime unplug.

Taking this example and looking at the patch,

Immediately after XHCIPciState is allocated, we'll have the
following XHCIPciState QOM state:

   free == object_free
   storage == <undefined>
   ref == 1
   storage_ref == 1   <up-ref from XHCIState>

Meanwhile XHCIState is embedded and so after construction it
has

   free == <undefined>
   storage == ptr<XHCIPciState>
   ref == 1
   storage_ref == UINT32_MAX


The simple scenario, no 3rd party refs on XHCIState, then we
have a flow that looks like this:

  object_unref(XHCIPciState)
   => ref == 0
       => object_finalize(XHCIPciState)
           => object_unref(XHCIState)
	       => ref == 0
	       => object_storage_unref(XHCIState)
	            => obj = obj.storage
	            => XHCIPciState.storage_ref--
	            => XHCIPciState.storage_ref == 0
                    => object_free(XHCIPciState)
       => Remainder of ...object_finalize(XHCIPciState) is
          using free'd mem now...

This doesn't seem to work AFAICS ?

Now lets consider something took a reference on XHCIState
first.

  object_ref(XHCIState)
  object_unref(XHCIPciState)
   => ref == 0
       => object_finalize(XHCIPciState)
           => object_unref(XHCIState)
	       => ref == 1
       => object_storage_unref(XHCIPciState)
          => storage_ref--
          => storage_ref == 0
              => object_free(XHCIPciState)

  object_unref(XHCIState)
   => ref == 0
   => object_storage_unref(XHCIPciState)
      => storage_ref == -1

Again this doesn't seem to actually work as billed ?


For the sake of argument though, lets say this storage_ref
was handled correctly such that we delay free'ing the
XHCIPciState memory  until *both*  XHCIPciState and
XHCIState have hit ref == 0, and both their finalize
methods are completed.

I'm still not convinced this would actually be safe.

This patch is delaying the free'ing of the memory that
XHCIState is embedded in, but is the embedded XHCIState
still functional enough when its parent and all sibling
objects will have been finalized already ? That is not
obviously ok to me. The "XHCIPciState" will have been
finalized, but all its fields are mostly pointing to
data that has been deinitialized. Can we really assume
there are no dependencies between the embedded objects ?


Conceptually it feels like finalization of XHCIPciState
ought to be an all-or-nothing affair, rather than
finalizing XHCIPciState and some of its embedded children
but arbtirarily keeping other children alive. I'm not
sure how to arrange that though - its essentially a need
for a circular reference, but then how/when do you break
it.

> diff --git a/include/qom/object.h b/include/qom/object.h
> index c4dcf65dcf7f..b5d656e3758c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -155,8 +155,12 @@ struct Object
>  {
>      /* private: */
>      ObjectClass *class;
> -    ObjectFree *free;
> +    union {
> +        ObjectFree *free;
> +        Object *storage;
> +    };
>      GHashTable *properties;
> +    uint32_t storage_ref;
>      uint32_t ref;
>      Object *parent;
>  };
> @@ -613,7 +617,7 @@ struct InterfaceClass
>   * @klass: The class to instantiate.
>   *
>   * This function will initialize a new object using heap allocated memory.
> - * The returned object has a reference count of 1, and will be freed when
> + * The returned object has a reference count of 1, and will be finalized when
>   * the last reference is dropped.
>   *
>   * Returns: The newly allocated and instantiated object.
> @@ -625,7 +629,7 @@ Object *object_new_with_class(ObjectClass *klass);
>   * @typename: The name of the type of the object to instantiate.
>   *
>   * This function will initialize a new object using heap allocated memory.
> - * The returned object has a reference count of 1, and will be freed when
> + * The returned object has a reference count of 1, and will be finalized when
>   * the last reference is dropped.
>   *
>   * Returns: The newly allocated and instantiated object.
> @@ -641,7 +645,7 @@ Object *object_new(const char *typename);
>   * @...: list of property names and values
>   *
>   * This function will initialize a new object using heap allocated memory.
> - * The returned object has a reference count of 1, and will be freed when
> + * The returned object has a reference count of 1, and will be finalized when
>   * the last reference is dropped.
>   *
>   * The @id parameter will be used when registering the object as a
> @@ -1120,8 +1124,8 @@ GSList *object_class_get_list_sorted(const char *implements_type,
>   * object_ref:
>   * @obj: the object
>   *
> - * Increase the reference count of a object.  A object cannot be freed as long
> - * as a reference remains.
> + * Increase the reference count of a object.  A object cannot be finalized as
> + * long as a reference remains.
>   * Returns: @obj
>   */
>  Object *object_ref(void *obj);
> @@ -1130,11 +1134,30 @@ Object *object_ref(void *obj);
>   * object_unref:
>   * @obj: the object
>   *
> - * Decrease the reference count of a object.  A object cannot be freed as long
> - * as a reference remains.
> + * Decrease the reference count of a object.  A object cannot be finalized as
> + * long as a reference remains.
>   */
>  void object_unref(void *obj);
>  
> +/**
> + * object_storage_ref:
> + * @obj: the object
> + *
> + * Increase the reference count of a object's storage.  A object cannot be
> + * freed as long as its storage is referenced.
> + * Returns: the object that provides the storage
> + */
> +Object *object_storage_ref(void *obj);
> +
> +/**
> + * object_storage_unref:
> + * @obj: the object
> + *
> + * Decrease the reference count of a object's storage.  A object cannot be
> + * freed as long as its storage is referenced.
> + */
> +void object_storage_unref(void *obj);
> +
>  /**
>   * object_property_try_add:
>   * @obj: the object to add a property to
> diff --git a/qom/object.c b/qom/object.c
> index 761eff1337e6..0a2c7ce9710d 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -487,7 +487,8 @@ static void object_class_property_init_all(Object *obj)
>      }
>  }
>  
> -static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type)
> +static void object_initialize_with_type(Object *obj, size_t size,
> +                                        TypeImpl *type, Object *storage)
>  {
>      type_initialize(type);
>  
> @@ -496,6 +497,10 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
>      g_assert(size >= type->instance_size);
>  
>      memset(obj, 0, type->instance_size);
> +    if (storage) {
> +        obj->storage_ref = UINT32_MAX;
> +        obj->storage = object_storage_ref(storage);
> +    }
>      obj->class = type->class;
>      object_class_property_init_all(obj);
>      obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
> @@ -508,7 +513,7 @@ void object_initialize(void *data, size_t size, const char *typename)
>  {
>      TypeImpl *type = type_get_or_load_by_name(typename, &error_fatal);
>  
> -    object_initialize_with_type(data, size, type);
> +    object_initialize_with_type(data, size, type, NULL);
>  }
>  
>  bool object_initialize_child_with_props(Object *parentobj,
> @@ -531,14 +536,15 @@ bool object_initialize_child_with_props(Object *parentobj,
>  bool object_initialize_child_with_propsv(Object *parentobj,
>                                           const char *propname,
>                                           void *childobj, size_t size,
> -                                         const char *type,
> +                                         const char *typename,
>                                           Error **errp, va_list vargs)
>  {
> +    TypeImpl *type = type_get_or_load_by_name(typename, &error_fatal);
>      bool ok = false;
>      Object *obj;
>      UserCreatable *uc;
>  
> -    object_initialize(childobj, size, type);
> +    object_initialize_with_type(childobj, size, type, parentobj);
>      obj = OBJECT(childobj);
>  
>      if (!object_set_propv(obj, vargs, errp)) {
> @@ -667,9 +673,7 @@ static void object_finalize(void *data)
>      object_deinit(obj, ti);
>  
>      g_assert(obj->parent == NULL);
> -    if (obj->free) {
> -        obj->free(obj);
> -    }
> +    object_storage_unref(obj);
>  }
>  
>  /* Find the minimum alignment guaranteed by the system malloc. */
> @@ -708,7 +712,7 @@ static Object *object_new_with_type(Type type)
>          obj_free = qemu_vfree;
>      }
>  
> -    object_initialize_with_type(obj, size, type);
> +    object_initialize_with_type(obj, size, type, NULL);
>      obj->free = obj_free;
>  
>      trace_object_new(obj, obj->class->type->name);
> @@ -1334,6 +1338,46 @@ void object_unref(void *objptr)
>      }
>  }
>  
> +Object *object_storage_ref(void *objptr)
> +{
> +    Object *obj = OBJECT(objptr);
> +    uint32_t ref;
> +
> +    if (!obj) {
> +        return NULL;
> +    }
> +
> +    while (qatomic_read(&obj->storage_ref) == UINT32_MAX) {
> +        obj = obj->storage;
> +    }
> +
> +    ref = qatomic_fetch_inc(&obj->storage_ref);
> +    /* Assert waaay before the integer overflows */
> +    g_assert(ref < INT_MAX);
> +    return obj;
> +}
> +
> +void object_storage_unref(void *objptr)
> +{
> +    Object *obj = OBJECT(objptr);
> +    uint32_t ref;
> +
> +    if (!obj) {
> +        return;
> +    }
> +
> +    while (qatomic_read(&obj->storage_ref) == UINT32_MAX) {
> +        obj = obj->storage;
> +    }
> +
> +    ref = qatomic_fetch_dec(&obj->storage_ref);
> +    g_assert(ref < INT_MAX);
> +
> +    if (ref == 0 && obj->free) {
> +        obj->free(obj);
> +    }
> +}
> +
>  ObjectProperty *
>  object_property_try_add(Object *obj, const char *name, const char *type,
>                          ObjectPropertyAccessor *get,
> diff --git a/rust/qom/src/qom.rs b/rust/qom/src/qom.rs
> index cc00ddcfc988..34f7fff64083 100644
> --- a/rust/qom/src/qom.rs
> +++ b/rust/qom/src/qom.rs
> @@ -809,8 +809,8 @@ impl<T: ObjectType> Owned<T> {
>      /// # 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.
> +    /// The object's implementation must guarantee functionality as long as it
> +    /// is referenced.
>      ///
>      /// A raw pointer obtained via [`Owned::into_raw()`] can always be passed
>      /// back to `from_raw()` (assuming the original `Owned` was valid!),
> @@ -836,8 +836,8 @@ pub fn into_raw(src: Owned<T>) -> *mut T {
>      ///
>      /// # Safety
>      ///
> -    /// The object must not be embedded in another, unless the outer
> -    /// object is guaranteed to have a longer lifetime.
> +    /// The object's implementation must guarantee functionality as long as it
> +    /// is referenced.
>      pub unsafe fn from(obj: &T) -> Self {
>          unsafe {
>              object_ref(obj.as_object_mut_ptr().cast::<c_void>());
> 
> -- 
> 2.54.0
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Akihiko Odaki]

On 2026/06/15 19:59, Daniel P. Berrangé wrote:
> On Mon, Jun 15, 2026 at 01:11:07PM +0900, Akihiko Odaki wrote:
>> One concrete case is qemu-xhci: XHCIPciState embeds XHCIState as a
>> child object, and the embedded XHCIState owns the MemoryRegion used for
>> PCI BAR 0. When memory core references that region, memory_region_ref()
>> references the embedded XHCIState owner, but that does not keep the
>> containing XHCIPciState storage alive across runtime unplug.
> 
> Taking this example and looking at the patch,
> 
> Immediately after XHCIPciState is allocated, we'll have the
> following XHCIPciState QOM state:
> 
>     free == object_free
>     storage == <undefined>
>     ref == 1
>     storage_ref == 1   <up-ref from XHCIState>

It should be:

     ref == 0
     storage_ref == 1

Please see "[PATCH 1/2] qom: Reject temporary object resurrection" for 
the context. `Object::ref` stores the logical reference count minus one, 
so `ref == 0` means one reference remains.

`storage_ref` follows the same convention for non-embedded storage. 
Here, `storage_ref == 1` means two storage references: the storage's own 
lifetime reference, plus the reference held via the embedded XHCIState.

> 
> Meanwhile XHCIState is embedded and so after construction it
> has
> 
>     free == <undefined>
>     storage == ptr<XHCIPciState>
>     ref == 1
>     storage_ref == UINT32_MAX

`ref == 0` here too.

> 
> 
> The simple scenario, no 3rd party refs on XHCIState, then we
> have a flow that looks like this:
> 
>    object_unref(XHCIPciState)
>     => ref == 0

This makes `ref == UINT32_MAX`; that marks the object as being finalized.

>         => object_finalize(XHCIPciState)
>             => object_unref(XHCIState)
> 	       => ref == 0

This also makes `XHCIState.ref == UINT32_MAX`.

> 	       => object_storage_unref(XHCIState)
> 	            => obj = obj.storage
> 	            => XHCIPciState.storage_ref--
> 	            => XHCIPciState.storage_ref == 0

Yes. At this point there is still one storage reference left, represented
as `storage_ref == 0`: the storage's own lifetime reference, held by the
XHCIPciState finalization path itself.

>                      => object_free(XHCIPciState)

`obj->free()` does not happen when `XHCIPciState.storage_ref == 0`.

>         => Remainder of ...object_finalize(XHCIPciState) is
>            using free'd mem now...

The free happens only after `object_finalize(XHCIPciState)` reaches its
own final `object_storage_unref(XHCIPciState)`. That decrements
`XHCIPciState.storage_ref` from `0` to `UINT32_MAX`, and only then calls
`obj->free()`.

> 
> This doesn't seem to work AFAICS ?
> 
> Now lets consider something took a reference on XHCIState
> first.
> 
>    object_ref(XHCIState)
>    object_unref(XHCIPciState)
>     => ref == 0
>         => object_finalize(XHCIPciState)
>             => object_unref(XHCIState)
> 	       => ref == 1
>         => object_storage_unref(XHCIPciState)
>            => storage_ref--
>            => storage_ref == 0
>                => object_free(XHCIPciState)
> 
>    object_unref(XHCIState)
>     => ref == 0
>     => object_storage_unref(XHCIPciState)
>        => storage_ref == -1
> 
> Again this doesn't seem to actually work as billed ?

With the minus-one representation, this becomes:

     object_ref(XHCIState)
       => XHCIState.ref goes from 0 to 1

     object_unref(XHCIPciState)
       => XHCIPciState.ref goes from 0 to UINT32_MAX
       => object_finalize(XHCIPciState)
           => object_unref(XHCIState)
              => XHCIState.ref goes from 1 to 0
              => XHCIState is not finalized yet
           => object_storage_unref(XHCIPciState)
              => XHCIPciState.storage_ref goes from 1 to 0
              => no free yet

     object_unref(XHCIState)
       => XHCIState.ref goes from 0 to UINT32_MAX
       => object_finalize(XHCIState)
           => object_storage_unref(XHCIState)
              => follows XHCIState.storage to XHCIPciState
              => XHCIPciState.storage_ref goes from 0 to UINT32_MAX
              => object_free(XHCIPciState)

So the containing storage is not freed until both finalizers have
completed.

> 
> 
> For the sake of argument though, lets say this storage_ref
> was handled correctly such that we delay free'ing the
> XHCIPciState memory  until *both*  XHCIPciState and
> XHCIState have hit ref == 0, and both their finalize
> methods are completed.
> 
> I'm still not convinced this would actually be safe.
> 
> This patch is delaying the free'ing of the memory that
> XHCIState is embedded in, but is the embedded XHCIState
> still functional enough when its parent and all sibling
> objects will have been finalized already ? That is not
> obviously ok to me. The "XHCIPciState" will have been
> finalized, but all its fields are mostly pointing to
> data that has been deinitialized. Can we really assume
> there are no dependencies between the embedded objects ?

That concern is valid. Keeping the containing storage allocated does not 
by itself guarantee that the embedded object remains fully functional 
after its parent has started finalization. That is also why the Rust 
wrapper for `object_ref()` remains unsafe.

I think this is orthogonal to the storage lifetime problem this patch 
solves. The same functional-lifetime problem can exist even if XHCIState 
is not embedded and is allocated with `object_new()`, because it may 
still depend on state owned by the parent or by sibling objects.

This patch prevents freeing the memory backing an externally referenced 
embedded object. It does not claim that every object implementation is 
safe to operate after the parent has begun unrealize/finalization.

> 
> 
> Conceptually it feels like finalization of XHCIPciState
> ought to be an all-or-nothing affair, rather than
> finalizing XHCIPciState and some of its embedded children
> but arbtirarily keeping other children alive. I'm not
> sure how to arrange that though - its essentially a need
> for a circular reference, but then how/when do you break
> it.

I agree that all-or-nothing finalization is attractive conceptually, but 
I do not think it gives enough semantics once objects can refer to each 
other. We still need to define an ordering: for example, if a parent and 
an embedded child refer to each other, either the child is finalized 
first and must not be accessed by the parent afterwards, or the parent 
is finalized first and the child must not access the parent afterwards.

So the practical rule cannot just be "finalize the aggregate as one 
unit". We need explicit ordering and state transitions. In the case 
where the parent is finalized before an externally referenced child, the 
child may remain allocated, but it must treat the parent-side state as 
gone and avoid accessing it.

In the MemoryRegion case described in the commit message, one possible 
higher-level fix is exactly this kind of ordering rule: once the device 
is being unrealized, accesses through the device's memory regions should 
be forbidden or should fail gracefully. The MemoryRegion object may 
remain referenced for a while, but late accesses should not rely on the 
parent device still being operational.

Regards,
Akihiko Odaki

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Mark Cave-Ayland]

On 15/06/2026 09:12, Daniel P. Berrangé wrote:

> On Mon, Jun 15, 2026 at 01:11:07PM +0900, Akihiko Odaki wrote:
>> Problem
>> =======
>>
>> The Rust wrapper for object_ref() is marked unsafe and has:
>>> The object must not be embedded in another unless the outer
>>> object is guaranteed to have a longer lifetime.
>>
>> In other words, object_ref() does not work for embedded objects and
>> does not keep embedded objects alive. MemoryRegion has its own
>> memory_region_ref() helper to call object_ref() on its owner for this
>> reason.
>>
>> However, this is insufficient to avoid calling object_ref() for all
>> embedded objects. For example, consider an embedded Device that has a
>> MemoryRegion. When referencing a MemoryRegion for guest memory access,
>> QEMU automatically references the owning Device to keep the MemoryRegion
>> alive. However, that reference is ineffective if the Device itself is
>> embedded, because object_ref() does not keep the containing storage
>> alive.
> 
> Do we know how many examples we have of embedding objects inside
> another ?
> 
> I would much prefer if we forbid the embedding of objects. It is
> horrible design practice to have some QOM objects which can be
> freed via reference count and some which cannot.

This is something I've been discussing with a few other people for some 
time now: shouldn't we deprecate object_initialize_child() and use 
references for everything instead? Not only would this help solve a lot 
of lifecycle problems, but it would help reviewers because at the moment 
we have too many different ways of doing the same thing.


ATB,

Mark.

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Mark Cave-Ayland]

On 15/06/2026 10:28, Peter Maydell wrote:

> On Mon, 15 Jun 2026 at 09:35, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Mon, Jun 15, 2026 at 09:31:36AM +0100, Peter Maydell wrote:
>>> On Mon, 15 Jun 2026 at 09:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>> Do we know how many examples we have of embedding objects inside
>>>> another ?
>>>
>>> It is an extremely common pattern for device and SoC model
>>> implementations; we've been recommending it for years.
>>>
>>>> I would much prefer if we forbid the embedding of objects. It is
>>>> horrible design practice to have some QOM objects which can be
>>>> freed via reference count and some which cannot.
>>>
>>> That would be a very large amount of code to rewrite to the
>>> new paradigm. I don't object inherently ("you have pointers to
>>> your child objects" works better when they might be implemented
>>> in Rust and might play better with being able to create
>>> machines and wire them up on the command line); I'm just
>>> noting how much work it would be if you wanted to make
>>> embedding forbidden.
>>
>> Would it be a more tractable problem to "fix" object structure only
>> incrementally as they gain a need to be managed/reference from
>> Rust code, or does the Rust usage already implicitly extend too
>> broadly
> 
> At the moment we have exactly 2 Rust devices. You can see the
> workaround we ended up with for the PL011 in commits 5b87a07e768
> and cc3d262aa93a4, where we pad out the C device struct and
> assert on the Rust side that its version isn't any bigger.
> 
> For that particular problem we could say "you need to refactor
> all users of device X to not embed it before creating the Rust
> version". There is I think only one embedded use of the PL011
> (I was actually expecting more). This does have the awkward
> effect that anybody wanting to do a "rewrite device in rust"
> is now forced into "do a big refactor of some old C code they
> don't care about". We would also need to actually document and
> provide some good examples of "this is how we think we should
> be writing device models that have child objects now".

I think given the legacy of the QEMU codebase then this will always be 
an issue, but we've solved this in the past with the introduction of 
MemoryRegions so there's no reason we couldn't do it again.

This is a similar problem to the use of non-class object properties I 
was discussing in the thread with Daniel: it seems the general consensus 
is that they shouldn't exist, but this hasn't been formalised anywhere. 
If we can formalise the decision not to use embedded QOM objects, then 
we can start by ensuring that new submissions don't use them and go from 
there.

Question: who currently should make these decisions? Is it restricted to 
the QOM maintainers?

> But embedded MemoryRegions are enormously common, and devices
> that embed other devices that embed MemoryRegions are also
> pretty common. So I don't know how much of a gradual approach
> you could take for the issue descibed in this patch.

Zoltan did post a patch that allowed for adding externally referenced 
MemoryRegions a while back: my reason for not supporting the approach 
was because it added yet another way of adding MemoryRegions to work 
around a particular legacy use case. However if there is agreement that 
this is the way we should do things, I suspect that the series will have 
solved most of the problems in this area.


ATB,

Mark.

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Akihiko Odaki]

On 2026/06/15 22:42, Mark Cave-Ayland wrote:
> On 15/06/2026 10:28, Peter Maydell wrote:
> 
>> On Mon, 15 Jun 2026 at 09:35, Daniel P. Berrangé <berrange@redhat.com> 
>> wrote:
>>>
>>> On Mon, Jun 15, 2026 at 09:31:36AM +0100, Peter Maydell wrote:
>>>> On Mon, 15 Jun 2026 at 09:13, Daniel P. Berrangé 
>>>> <berrange@redhat.com> wrote:
>>>>> Do we know how many examples we have of embedding objects inside
>>>>> another ?
>>>>
>>>> It is an extremely common pattern for device and SoC model
>>>> implementations; we've been recommending it for years.
>>>>
>>>>> I would much prefer if we forbid the embedding of objects. It is
>>>>> horrible design practice to have some QOM objects which can be
>>>>> freed via reference count and some which cannot.
>>>>
>>>> That would be a very large amount of code to rewrite to the
>>>> new paradigm. I don't object inherently ("you have pointers to
>>>> your child objects" works better when they might be implemented
>>>> in Rust and might play better with being able to create
>>>> machines and wire them up on the command line); I'm just
>>>> noting how much work it would be if you wanted to make
>>>> embedding forbidden.
>>>
>>> Would it be a more tractable problem to "fix" object structure only
>>> incrementally as they gain a need to be managed/reference from
>>> Rust code, or does the Rust usage already implicitly extend too
>>> broadly
>>
>> At the moment we have exactly 2 Rust devices. You can see the
>> workaround we ended up with for the PL011 in commits 5b87a07e768
>> and cc3d262aa93a4, where we pad out the C device struct and
>> assert on the Rust side that its version isn't any bigger.
>>
>> For that particular problem we could say "you need to refactor
>> all users of device X to not embed it before creating the Rust
>> version". There is I think only one embedded use of the PL011
>> (I was actually expecting more). This does have the awkward
>> effect that anybody wanting to do a "rewrite device in rust"
>> is now forced into "do a big refactor of some old C code they
>> don't care about". We would also need to actually document and
>> provide some good examples of "this is how we think we should
>> be writing device models that have child objects now".
> 
> I think given the legacy of the QEMU codebase then this will always be 
> an issue, but we've solved this in the past with the introduction of 
> MemoryRegions so there's no reason we couldn't do it again.
> 
> This is a similar problem to the use of non-class object properties I 
> was discussing in the thread with Daniel: it seems the general consensus 
> is that they shouldn't exist, but this hasn't been formalised anywhere. 
> If we can formalise the decision not to use embedded QOM objects, then 
> we can start by ensuring that new submissions don't use them and go from 
> there.
> 
> Question: who currently should make these decisions? Is it restricted to 
> the QOM maintainers?
> 
>> But embedded MemoryRegions are enormously common, and devices
>> that embed other devices that embed MemoryRegions are also
>> pretty common. So I don't know how much of a gradual approach
>> you could take for the issue descibed in this patch.
> 
> Zoltan did post a patch that allowed for adding externally referenced 
> MemoryRegions a while back: my reason for not supporting the approach 
> was because it added yet another way of adding MemoryRegions to work 
> around a particular legacy use case. However if there is agreement that 
> this is the way we should do things, I suspect that the series will have 
> solved most of the problems in this area.

Personally, I do not have a strong preference between supporting 
references to embedded objects and deprecating object embedding 
entirely, so this patch takes the smaller step needed to address the 
immediate issue. I would not object to deprecating embedding if that is 
the direction we want to take.

Regards,
Akihiko Odaki

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[Daniel P. Berrangé]

On Mon, Jun 15, 2026 at 02:42:10PM +0100, Mark Cave-Ayland wrote:
> On 15/06/2026 10:28, Peter Maydell wrote:
> 
> > On Mon, 15 Jun 2026 at 09:35, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > 
> > > On Mon, Jun 15, 2026 at 09:31:36AM +0100, Peter Maydell wrote:
> > > > On Mon, 15 Jun 2026 at 09:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > > Do we know how many examples we have of embedding objects inside
> > > > > another ?
> > > > 
> > > > It is an extremely common pattern for device and SoC model
> > > > implementations; we've been recommending it for years.
> > > > 
> > > > > I would much prefer if we forbid the embedding of objects. It is
> > > > > horrible design practice to have some QOM objects which can be
> > > > > freed via reference count and some which cannot.
> > > > 
> > > > That would be a very large amount of code to rewrite to the
> > > > new paradigm. I don't object inherently ("you have pointers to
> > > > your child objects" works better when they might be implemented
> > > > in Rust and might play better with being able to create
> > > > machines and wire them up on the command line); I'm just
> > > > noting how much work it would be if you wanted to make
> > > > embedding forbidden.
> > > 
> > > Would it be a more tractable problem to "fix" object structure only
> > > incrementally as they gain a need to be managed/reference from
> > > Rust code, or does the Rust usage already implicitly extend too
> > > broadly
> > 
> > At the moment we have exactly 2 Rust devices. You can see the
> > workaround we ended up with for the PL011 in commits 5b87a07e768
> > and cc3d262aa93a4, where we pad out the C device struct and
> > assert on the Rust side that its version isn't any bigger.
> > 
> > For that particular problem we could say "you need to refactor
> > all users of device X to not embed it before creating the Rust
> > version". There is I think only one embedded use of the PL011
> > (I was actually expecting more). This does have the awkward
> > effect that anybody wanting to do a "rewrite device in rust"
> > is now forced into "do a big refactor of some old C code they
> > don't care about". We would also need to actually document and
> > provide some good examples of "this is how we think we should
> > be writing device models that have child objects now".
> 
> I think given the legacy of the QEMU codebase then this will always be an
> issue, but we've solved this in the past with the introduction of
> MemoryRegions so there's no reason we couldn't do it again.
> 
> This is a similar problem to the use of non-class object properties I was
> discussing in the thread with Daniel: it seems the general consensus is that
> they shouldn't exist, but this hasn't been formalised anywhere. If we can
> formalise the decision not to use embedded QOM objects, then we can start by
> ensuring that new submissions don't use them and go from there.
> 
> Question: who currently should make these decisions? Is it restricted to the
> QOM maintainers?

Ultimately it would need a docs update on object.h to declare the
object_initialize_child related methods to be deprecated which the
QOM maintainers would have to queue.

Since this impacts device maintainers in general though, IMHO we need
broad consensus that it is a better approach & we're willing to convert
existing code incrementally.

We have ~150 devices using this for 824 children

$ git grep -l  object_initialize_child  | wc -l
150

$ git grep object_initialize_child | wc -l
824


Where updates are desired, we'd be looking at tedious changes like
thsi:

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 31fa53e6a4..86757c56e5 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -355,16 +355,23 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
 
     /* USB */
     if (d->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &d->uhci, uhci_type);
-        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
-        if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
+        d->uhci = UHCI(object_new_with_props(
+                           uhci_type, OBJECT(d), "uhci", errp, NULL));
+        if (!d->uhci) {
+            return;
+        }
+        qdev_prop_set_int32(DEVICE(d->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(d->uhci), BUS(pci_bus), errp)) {
             return;
         }
     }
diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 31fa53e6a4..86757c56e5 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -355,16 +355,23 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
 
     /* USB */
     if (d->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &d->uhci, uhci_type);
-        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
-        if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
+        d->uhci = UHCI(object_new_with_props(
+                           uhci_type, OBJECT(d), "uhci", errp, NULL));
+        if (!d->uhci) {
+            return;
+        }
+        qdev_prop_set_int32(DEVICE(d->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(d->uhci), BUS(pci_bus), errp)) {
             return;
         }
     }


Noting that the compiler won't complain if we forget to update any
lines like

-        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
+        qdev_prop_set_int32(DEVICE(d->uhci), "addr", dev->devfn + 2);

because the explicit DEVICE(..) cast is hiding the type error we would
otherwise get from using "Device **" instead of "Device *".


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 2/2] qom: Manage references to embedded child objects

[BALATON Zoltan]

On Mon, 15 Jun 2026, Mark Cave-Ayland wrote:
>> But embedded MemoryRegions are enormously common, and devices
>> that embed other devices that embed MemoryRegions are also
>> pretty common. So I don't know how much of a gradual approach
>> you could take for the issue descibed in this patch.
>
> Zoltan did post a patch that allowed for adding externally referenced 
> MemoryRegions a while back: my reason for not supporting the approach was 
> because it added yet another way of adding MemoryRegions to work around a 
> particular legacy use case. However if there is agreement that this is the 
> way we should do things, I suspect that the series will have solved most of 
> the problems in this area.

That series was blocked by Paolo saying no: 
https://lists.nongnu.org/archive/html/qemu-devel/2026-05/msg06665.html 
without ever explaining why it should not be allowed to create ref counted 
memory regions. even though it's documented and implemented that way 
already, but the current memory_region_init() functions don't allow using 
that and forces every memory region to either be embedded or leaked. I 
still think that using QOM to manage memory region life time with ref 
counts is useful as it also makes device models simpler and my series 
allows incremental conversion as it only adds the functions to create QOM 
managed memory regions leaving the exising embedded regions untouched so 
they could be converted one by one when needed. If there's more interest 
in this now I can update and repost it again or I'd still like to see an 
explanation why it is not wanted.

Regards,
BALATON Zoltan