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

["Daniel P. Berrangé" <berrange@redhat.com>]

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