* [PULL 1/2] memory: Fix addr/len for flatview_access_allowed()
2025-09-15 16:03 [PULL 0/2] Mem staging patches Peter Xu
@ 2025-09-15 16:03 ` Peter Xu
2025-09-15 16:03 ` [PULL 2/2] memory: Fix leaks due to owner-shared MRs circular references Peter Xu
2025-09-16 19:21 ` [PULL 0/2] Mem staging patches Richard Henderson
2 siblings, 0 replies; 4+ messages in thread
From: Peter Xu @ 2025-09-15 16:03 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: peterx, David Hildenbrand, Paolo Bonzini,
Philippe Mathieu-Daudé
flatview_access_allowed() should pass in the address offset of the memory
region, rather than the global address space. Shouldn't be a major issue
yet, since the addr is only used in an error log.
Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Fixes: 3ab6fdc91b ("softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_ACCESS_ERROR")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Link: https://lore.kernel.org/r/20250903142932.1038765-1-peterx@redhat.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
system/physmem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/system/physmem.c b/system/physmem.c
index 311011156c..ddd58e9eb8 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3027,7 +3027,7 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
l = len;
mr = flatview_translate(fv, addr, &mr_addr, &l, true, attrs);
- if (!flatview_access_allowed(mr, attrs, addr, len)) {
+ if (!flatview_access_allowed(mr, attrs, mr_addr, l)) {
return MEMTX_ACCESS_ERROR;
}
return flatview_write_continue(fv, addr, attrs, buf, len,
@@ -3118,7 +3118,7 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
l = len;
mr = flatview_translate(fv, addr, &mr_addr, &l, false, attrs);
- if (!flatview_access_allowed(mr, attrs, addr, len)) {
+ if (!flatview_access_allowed(mr, attrs, mr_addr, l)) {
return MEMTX_ACCESS_ERROR;
}
return flatview_read_continue(fv, addr, attrs, buf, len,
--
2.50.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PULL 2/2] memory: Fix leaks due to owner-shared MRs circular references
2025-09-15 16:03 [PULL 0/2] Mem staging patches Peter Xu
2025-09-15 16:03 ` [PULL 1/2] memory: Fix addr/len for flatview_access_allowed() Peter Xu
@ 2025-09-15 16:03 ` Peter Xu
2025-09-16 19:21 ` [PULL 0/2] Mem staging patches Richard Henderson
2 siblings, 0 replies; 4+ messages in thread
From: Peter Xu @ 2025-09-15 16:03 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: peterx, David Hildenbrand, Paolo Bonzini, Akihiko Odaki,
Clément Mathieu--Drif
Currently, QEMU refcounts the MR by always taking it from the owner.
It's common that one object will have multiple MR objects embeded in the
object itself. All the MRs in this case share the same lifespan of the
owner object.
It's also common that in the instance_init() of an object, MR A can be a
container of MR B, C, D, by using memory_region_add_subregion*() set of
memory region APIs.
Now we have a circular reference issue, as when adding subregions for MR A,
we essentially incremented the owner's refcount within the instance_init(),
meaning the object will be self-boosted and its refcount can never go down
to zero if the MRs won't get detached properly before object's finalize().
Delete subregions within object's finalize() won't work either, because
finalize() will be invoked only if the refcount goes to zero first. What
is worse, object_finalize() will do object_property_del_all() first before
object_deinit(). Since embeded MRs will be properties of the owner object,
it means they'll be freed _before_ the owner's finalize().
To fix that, teach memory API to stop refcount on MRs that share the same
owner. Because if they share the lifecycle of the owner, then they share
the same lifecycle between themselves, hence the refcount doesn't help but
only introduce troubles.
Meanwhile, allow auto-detachments of MRs during finalize() of MRs even
against its container, as long as they belong to the same owner.
The latter is needed because now it's possible to have MRs' finalize()
happen in any order when they share the same lifespan with a same owner.
In this case, we should allow finalize() to happen in any order of either
the parent or child MR. Loose the mr->container check in MR's finalize()
to allow auto-detach. Double check it shares the same owner.
Proper document this behavior in code.
This patch is heavily based on the work done by Akihiko Odaki:
https://lore.kernel.org/r/CAFEAcA8DV40fGsci76r4yeP1P-SP_QjNRDD2OzPxjx5wRs0GEg@mail.gmail.com
Cc: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Peter Maydell <peter.maydell@linaro.org>
Link: https://lore.kernel.org/r/20250826221750.285242-1-peterx@redhat.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
docs/devel/memory.rst | 7 +++++--
system/memory.c | 46 ++++++++++++++++++++++++++++++++++---------
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 57fb2aec76..42d3ca29c4 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -158,8 +158,11 @@ ioeventfd) can be changed during the region lifecycle. They take effect
as soon as the region is made visible. This can be immediately, later,
or never.
-Destruction of a memory region happens automatically when the owner
-object dies.
+Destruction of a memory region happens automatically when the owner object
+dies. When there are multiple memory regions under the same owner object,
+the memory API will guarantee all memory regions will be properly detached
+and finalized one by one. The order in which memory regions will be
+finalized is not guaranteed.
If however the memory region is part of a dynamically allocated data
structure, you should call object_unparent() to destroy the memory region
diff --git a/system/memory.c b/system/memory.c
index 44701c465c..cf8cad6961 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1796,16 +1796,37 @@ static void memory_region_finalize(Object *obj)
{
MemoryRegion *mr = MEMORY_REGION(obj);
- assert(!mr->container);
-
- /* We know the region is not visible in any address space (it
- * does not have a container and cannot be a root either because
- * it has no references, so we can blindly clear mr->enabled.
- * memory_region_set_enabled instead could trigger a transaction
- * and cause an infinite loop.
+ /*
+ * Each memory region (that can be freed) must have an owner, and it
+ * always has the same lifecycle of its owner. It means when reaching
+ * here, the memory region's owner's refcount is zero.
+ *
+ * Here it is possible that the MR has:
+ *
+ * (1) mr->container set, which means this MR is a subregion of a
+ * container MR. In this case they must share the same owner as the
+ * container (otherwise the container should have kept a refcount
+ * of this MR's owner).
+ *
+ * (2) mr->subregions non-empty, which means this MR is a container of
+ * one or more other MRs (which might have the the owner as this
+ * MR, or a different owner).
+ *
+ * We know the MR, or any MR that is attached to this one as either
+ * container or children, is not visible in any address space, because
+ * otherwise the address space should have taken at least one refcount
+ * of this MR's owner. So we can blindly clear mr->enabled.
+ *
+ * memory_region_set_enabled instead could trigger a transaction and
+ * cause an infinite loop.
*/
mr->enabled = false;
memory_region_transaction_begin();
+ if (mr->container) {
+ /* Must share the owner; see above comments */
+ assert(mr->container->owner == mr->owner);
+ memory_region_del_subregion(mr->container, mr);
+ }
while (!QTAILQ_EMPTY(&mr->subregions)) {
MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
memory_region_del_subregion(mr, subregion);
@@ -2640,7 +2661,10 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion)
memory_region_transaction_begin();
- memory_region_ref(subregion);
+ if (mr->owner != subregion->owner) {
+ memory_region_ref(subregion);
+ }
+
QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
if (subregion->priority >= other->priority) {
QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2698,7 +2722,11 @@ void memory_region_del_subregion(MemoryRegion *mr,
assert(alias->mapped_via_alias >= 0);
}
QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
- memory_region_unref(subregion);
+
+ if (mr->owner != subregion->owner) {
+ memory_region_unref(subregion);
+ }
+
memory_region_update_pending |= mr->enabled && subregion->enabled;
memory_region_transaction_commit();
}
--
2.50.1
^ permalink raw reply related [flat|nested] 4+ messages in thread