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
