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

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