On 2026/01/29 0:46, BALATON Zoltan wrote:
On Wed, 28 Jan 2026, Peter Maydell wrote:
On Wed, 28 Jan 2026 at 11:40, BALATON Zoltan <[email protected]> wrote:
OK I try to summarise the motivation again:
1. Documentation in docs/devel/memory.rst says that memory regions'
lifecycle is managed by QOM and they are freed with their owner or when
nothing else uses them. This is also already implemented for a long time
as described but cannot be used because the only constructors available
kill this feature when calling object_initialize that clears the free
function added by object_new. (The life time management is implemented
through adding memory regions as children to the owner and unparenting
them on freeing the owner which decreases ref count of the memory region
and will free it when nothing else references it as far as I can tell.)
If we have leaks because of our very common pattern of "embed
a MemoryRegion struct in the device state struct" then we must
fix those, because there's no way we're going to convert all
that existing code to a new set of APIs. But I was under the
impression we had already dealt with those, because MRs track
their owner's refcount, and don't have their own independent one ?
I'm not sure if all those leaks are resolved as there were some patches
and discussion about this recently but I think that problem or the need
to use the owner's ref count to circumwent it instead of using the
memory region's own ref count may also come from that there's currently
no way to allocate memory regions that are ref counted and automatically
freed as it should work with QOM and the documentation implies. (Only
the constuctor is missing that is all this series adds, the mechanism is
already there and implemented.) There may still be a problem with
circular references if the memory region needs the owner so the owner
can't be freed until the memory region is also freed but the memory
region is not freed until the owner is freed but if both the owner and
memory region used their own ref count things may become a bit less
confusing and could be easier to find a way to break circular reference
(e.g. by owner unparent child regions on unrealize but isn't freed until
memory regions unref owner in their free method).
These are my motivation for this change. What is the motivation for
using
embedded memory regions instead and against this change?
Simply that it's a consistent pattern we use in a lot of the codebase:
the device embeds a lot of the structs it uses, rather than allocating
memory for them and keeping pointers to that allocated memory. We
You mix in the issue of SoCs and complex devices using other devices in
which case the recommendation was to embed those in the parent device so
they don't have to be freed or kept track of by a pointer but won't be
leaked. This series does not mean to change that, it's only limited to
memory regions. (Although that problem may also stem from similar issue
with object_initialize_child not allowing creating reference counted
objects only initializing preallocated instances but that's not
something this series touches.)
We can say that memory regions are like other embedded objects but they
are often used for sysbus and PCI devices only to be registered in the
parent device that already has pointers in their state to track these so
there's no need to keep track of them in the subclass if we can rely on
QOM freeing them when not needed any more and this is already
implemented and documented that way. So even if we keep embedding other
child devices into complex parent devices that I think does not directly
apply to memory regions and we could use what the documentation and
implementation already allows and says for memory regions at least.
still have also various older device models that use the previous
pattern of "allocate memory and have pointers" too, but most new
code doesn't do that. I think we should for preference write code
in one pattern, not two, and "embed structs" seems to be what
we have mostly settled on for new code.
There is an argument to be made that the pointer model would
fit better with a possible future world of "the user can wire
configurably wire up their own board model from devices", and
that it works better in a part-Rust-part-C world where the two
different languages don't have convenient access to the exact
size of structs defined in the other language. But that future
model is not something anybody has yet really fleshed out in any
detail, so it's still a bit speculative.
You keep mentioning pointers but the point of ref counts and regisrering
memory region as child of an owner is to avoid needing a pointer or
embedding it in the subclass state as the relationship and lifecycle
management are then handled by QOM. If we don't use that we could remove
this from QOM and memory regions to simplify it but if it's already
there and makes the device state simpler I think we better use it.
I'm not actually opposed to the idea of making a design decision
that this struct-embedding is no longer what we want to do, and defining
that something else is our new best practice for how to write devices.
But I think we would need to start by reaching a consensus that that
*is* what we want to do, and documenting that "best practice" somewhere
in docs/devel/. Then we can examine proposed new APIs and all be
on the same page about the design patterns we want and it will
be clearer to reviewers whether the new APIs fit into those
patterns or not.
I think we're in that discussion now in this thread. I don't propose to
change the struct-embedding for sub devices used in SoC or south bridge
or other complex devices but only propose to not embed memory regions
that are already documented as and handled by QOM and simply allocate
them and let QOM handle them so we only need to reference them in the
devices state unless they are needed for some reason by the device
methods which is rarely the case. So this is limited to memory regions
and the series only seems to add a lot of lines because of the extensive
documentation comments. The actual change is just factoring out actual
memory region init from memory_region_init functions then add a
memory_region_new variant that does object_new; do_init and keep the
memory_region_init do object_initializel do_init. Nothing else is
changed, the way to manage and free regions based on ref counting is
already there this series just enables them to be actually used becase
currently despite what the docs say memory regions are either leaked or
must be embedded.
I actually think deprecating struct-embedding for all QOM objects is a
good idea.
The problem initially stated in this thread is that embedding requires
having extra field, but people see the benefit is too small. There is no
real logic involved in having such fields so it does not reduce code
complexity much; it saves some lines and that's it.
However, I see another problem in struct embedding; it breaks
object_ref(). When embedding, the child object effectively takes the
reference to the storage of the parent object, but this reference is not
counted, so use-after-free can happen if someone takes a reference to
the child object with object_ref(). That is why the wrapper of
object_ref() in rust/qom/src/qom.rs needs to be marked unsafe. Memory
regions workaround this with memory_region_ref(), but it's not perfect
since it relies on object_ref() in the end.
For this reason I think object_initialize(), object_initialize_child(),
and the like are better to be noted as deprecated in
include/qom/object.h. Then memory_region_init() can be deprecated
referring to them.
Regards,
Akihiko Odaki
