[PATCH 0/3] system: Don't leak CPU AddressSpaces

[PATCH 0/3] system: Don't leak CPU AddressSpaces

[Peter Maydell]

When a vCPU is created, it typically calls cpu_address_space_init()
one or more times to set up its address spaces. We don't currently
do anything to destroy these address spaces, which means that we
will leak them on a vcpu hot-plug -> hot-unplug cycle.

This patchset fixes the leak by replacing the current
cpu_address_space_destroy() (which has an awkward API, includes
a bug, and is never called from anywhere) with a new
cpu_destroy_address_spaces() which cleans up all the ASes a CPU
has and is called from generic unrealize code.

Patch 1 is just a comment improvement to clarify that
address_space_destroy() defers most of its work to RCU and you
can't free the memory for the AS struct itself until it's done.

Patch 2 is from Peter Xu; we need to be able to do "destroy and
free an AS" via RCU, and at the moment you can't do that.

Patch 3 is the bugfix proper.

thanks
-- PMM

Peter Maydell (2):
  include/system/memory.h: Clarify address_space_destroy() behaviour
  physmem: Destroy all CPU AddressSpaces on unrealize

Peter Xu (1):
  memory: New AS helper to serialize destroy+free

 include/exec/cpu-common.h          | 10 ++++-----
 include/hw/core/cpu.h              |  1 -
 include/system/memory.h            | 24 ++++++++++++++++++---
 hw/core/cpu-common.c               |  1 +
 stubs/cpu-destroy-address-spaces.c | 15 +++++++++++++
 system/memory.c                    | 20 +++++++++++++++++-
 system/physmem.c                   | 34 ++++++++++++++----------------
 stubs/meson.build                  |  1 +
 8 files changed, 78 insertions(+), 28 deletions(-)
 create mode 100644 stubs/cpu-destroy-address-spaces.c

-- 
2.43.0

[PATCH 1/3] include/system/memory.h: Clarify address_space_destroy() behaviour

[Peter Maydell]

address_space_destroy() doesn't actually immediately destroy the AS;
it queues it to be destroyed via RCU. This means you can't g_free()
the memory the AS struct is in until that has happened.

Clarify this in the documentation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/system/memory.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/system/memory.h b/include/system/memory.h
index aa85fc27a10..827e2c5aa44 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2727,9 +2727,14 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
 /**
  * address_space_destroy: destroy an address space
  *
- * Releases all resources associated with an address space.  After an address space
- * is destroyed, its root memory region (given by address_space_init()) may be destroyed
- * as well.
+ * Releases all resources associated with an address space.  After an
+ * address space is destroyed, the reference the AddressSpace had to
+ * its root memory region is dropped, which may result in the
+ * destruction of that memory region as well.
+ *
+ * Note that destruction of the AddressSpace is done via RCU;
+ * it is therefore not valid to free the memory the AddressSpace
+ * struct is in until after that RCU callback has completed.
  *
  * @as: address space to be destroyed
  */
-- 
2.43.0

[PATCH 2/3] memory: New AS helper to serialize destroy+free

[Peter Maydell]

From: Peter Xu <peterx@redhat.com>

If an AddressSpace has been created in its own allocated
memory, cleaning it up requires first destroying the AS
and then freeing the memory. Doing this doesn't work:

    address_space_destroy(as);
    g_free_rcu(as, rcu);

because both address_space_destroy() and g_free_rcu()
try to use the same 'rcu' node in the AddressSpace struct
and the address_space_destroy hook gets overwritten.

Provide a new address_space_destroy_free() function which
will destroy the AS and then free the memory it uses, all
in one RCU callback.

(CC to stable because the next commit needs this function.)

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Xu <peterx@redhat.com>
[PMM: Expanded commit message with motivation, tweaked comment]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/system/memory.h | 13 +++++++++++++
 system/memory.c         | 20 +++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/system/memory.h b/include/system/memory.h
index 827e2c5aa44..08daf0fc59e 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2735,11 +2735,24 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
  * Note that destruction of the AddressSpace is done via RCU;
  * it is therefore not valid to free the memory the AddressSpace
  * struct is in until after that RCU callback has completed.
+ * If you want to g_free() the AddressSpace after destruction you
+ * can do that with address_space_destroy_free().
  *
  * @as: address space to be destroyed
  */
 void address_space_destroy(AddressSpace *as);
 
+/**
+ * address_space_destroy_free: destroy an address space and free it
+ *
+ * This does the same thing as address_space_destroy(), and then also
+ * frees (via g_free()) the AddressSpace itself once the destruction
+ * is complete.
+ *
+ * @as: address space to be destroyed
+ */
+void address_space_destroy_free(AddressSpace *as);
+
 /**
  * address_space_remove_listeners: unregister all listeners of an address space
  *
diff --git a/system/memory.c b/system/memory.c
index cf8cad69611..fe8b28a096b 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3278,7 +3278,14 @@ static void do_address_space_destroy(AddressSpace *as)
     memory_region_unref(as->root);
 }
 
-void address_space_destroy(AddressSpace *as)
+static void do_address_space_destroy_free(AddressSpace *as)
+{
+    do_address_space_destroy(as);
+    g_free(as);
+}
+
+/* Detach address space from global view, notify all listeners */
+static void address_space_detach(AddressSpace *as)
 {
     MemoryRegion *root = as->root;
 
@@ -3293,9 +3300,20 @@ void address_space_destroy(AddressSpace *as)
      * values to expire before freeing the data.
      */
     as->root = root;
+}
+
+void address_space_destroy(AddressSpace *as)
+{
+    address_space_detach(as);
     call_rcu(as, do_address_space_destroy, rcu);
 }
 
+void address_space_destroy_free(AddressSpace *as)
+{
+    address_space_detach(as);
+    call_rcu(as, do_address_space_destroy_free, rcu);
+}
+
 static const char *memory_region_type(MemoryRegion *mr)
 {
     if (mr->alias) {
-- 
2.43.0

[PATCH 3/3] physmem: Destroy all CPU AddressSpaces on unrealize

[Peter Maydell]

When we unrealize a CPU object (which happens on vCPU hot-unplug), we
should destroy all the AddressSpace objects we created via calls to
cpu_address_space_init() when the CPU was realized.

Commit 24bec42f3d6eae added a function to do this for a specific
AddressSpace, but did not add any places where the function was
called.

Since we always want to destroy all the AddressSpaces on unrealize,
regardless of the target architecture, we don't need to try to keep
track of how many are still undestroyed, or make the target
architecture code manually call a destroy function for each AS it
created.  Instead we can adjust the function to always completely
destroy the whole cpu->ases array, and arrange for it to be called
during CPU unrealize as part of the common code.

Without this fix, AddressSanitizer will report a leak like this
from a run where we hot-plugged and then hot-unplugged an x86 KVM
vCPU:

Direct leak of 416 byte(s) in 1 object(s) allocated from:
    #0 0x5b638565053d in calloc (/data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/qemu-system-x86_64+0x1ee153d) (BuildId: c1cd6022b195142106e1bffeca23498c2b752bca)
    #1 0x7c28083f77b1 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x637b1) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
    #2 0x5b6386999c7c in cpu_address_space_init /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../system/physmem.c:797:25
    #3 0x5b638727f049 in kvm_cpu_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../target/i386/kvm/kvm-cpu.c:102:5
    #4 0x5b6385745f40 in accel_cpu_common_realize /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../accel/accel-common.c:101:13
    #5 0x5b638568fe3c in cpu_exec_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/cpu-common.c:232:10
    #6 0x5b63874a2cd5 in x86_cpu_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../target/i386/cpu.c:9321:5
    #7 0x5b6387a0469a in device_set_realized /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/qdev.c:494:13
    #8 0x5b6387a27d9e in property_set_bool /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:2375:5
    #9 0x5b6387a2090b in object_property_set /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:1450:5
    #10 0x5b6387a35b05 in object_property_set_qobject /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/qom-qobject.c:28:10
    #11 0x5b6387a21739 in object_property_set_bool /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:1520:15
    #12 0x5b63879fe510 in qdev_realize /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/qdev.c:276:12


Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2517
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu-common.h          | 10 ++++-----
 include/hw/core/cpu.h              |  1 -
 hw/core/cpu-common.c               |  1 +
 stubs/cpu-destroy-address-spaces.c | 15 +++++++++++++
 system/physmem.c                   | 34 ++++++++++++++----------------
 stubs/meson.build                  |  1 +
 6 files changed, 38 insertions(+), 24 deletions(-)
 create mode 100644 stubs/cpu-destroy-address-spaces.c

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index f373781ae07..b96ac49844a 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -123,13 +123,13 @@ size_t qemu_ram_pagesize_largest(void);
 void cpu_address_space_init(CPUState *cpu, int asidx,
                             const char *prefix, MemoryRegion *mr);
 /**
- * cpu_address_space_destroy:
- * @cpu: CPU for which address space needs to be destroyed
- * @asidx: integer index of this address space
+ * cpu_destroy_address_spaces:
+ * @cpu: CPU for which address spaces need to be destroyed
  *
- * Note that with KVM only one address space is supported.
+ * Destroy all address spaces associated with this CPU; this
+ * is called as part of unrealizing the CPU.
  */
-void cpu_address_space_destroy(CPUState *cpu, int asidx);
+void cpu_destroy_address_spaces(CPUState *cpu);
 
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c9f40c25392..0fcbc923f38 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -515,7 +515,6 @@ struct CPUState {
     QSIMPLEQ_HEAD(, qemu_work_item) work_list;
 
     struct CPUAddressSpace *cpu_ases;
-    int cpu_ases_count;
     int num_ases;
     AddressSpace *as;
     MemoryRegion *memory;
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 41a339903ca..8c306c89e45 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -294,6 +294,7 @@ void cpu_exec_unrealizefn(CPUState *cpu)
      * accel_cpu_common_unrealize, which may free fields using call_rcu.
      */
     accel_cpu_common_unrealize(cpu);
+    cpu_destroy_address_spaces(cpu);
 }
 
 static void cpu_common_initfn(Object *obj)
diff --git a/stubs/cpu-destroy-address-spaces.c b/stubs/cpu-destroy-address-spaces.c
new file mode 100644
index 00000000000..dc6813f5bd1
--- /dev/null
+++ b/stubs/cpu-destroy-address-spaces.c
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "exec/cpu-common.h"
+
+/*
+ * user-mode CPUs never create address spaces with
+ * cpu_address_space_init(), so the cleanup function doesn't
+ * need to do anything. We need this stub because cpu-common.c
+ * is built-once so it can't #ifndef CONFIG_USER around the
+ * call; the real function is in physmem.c which is system-only.
+ */
+void cpu_destroy_address_spaces(CPUState *cpu)
+{
+}
diff --git a/system/physmem.c b/system/physmem.c
index ae8ecd50ea1..dbb2a4e0175 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -795,7 +795,6 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 
     if (!cpu->cpu_ases) {
         cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
-        cpu->cpu_ases_count = cpu->num_ases;
     }
 
     newas = &cpu->cpu_ases[asidx];
@@ -809,30 +808,29 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
     }
 }
 
-void cpu_address_space_destroy(CPUState *cpu, int asidx)
+void cpu_destroy_address_spaces(CPUState *cpu)
 {
     CPUAddressSpace *cpuas;
+    int asidx;
 
     assert(cpu->cpu_ases);
-    assert(asidx >= 0 && asidx < cpu->num_ases);
 
-    cpuas = &cpu->cpu_ases[asidx];
-    if (tcg_enabled()) {
-        memory_listener_unregister(&cpuas->tcg_as_listener);
+    /* convenience alias just points to some cpu_ases[n] */
+    cpu->as = NULL;
+
+    for (asidx = 0; asidx < cpu->num_ases; asidx++) {
+        cpuas = &cpu->cpu_ases[asidx];
+        if (!cpuas->as) {
+            /* This index was never initialized; no deinit needed */
+            continue;
+        }
+        if (tcg_enabled()) {
+            memory_listener_unregister(&cpuas->tcg_as_listener);
+        }
+        g_clear_pointer(&cpuas->as, address_space_destroy_free);
     }
 
-    address_space_destroy(cpuas->as);
-    g_free_rcu(cpuas->as, rcu);
-
-    if (asidx == 0) {
-        /* reset the convenience alias for address space 0 */
-        cpu->as = NULL;
-    }
-
-    if (--cpu->cpu_ases_count == 0) {
-        g_free(cpu->cpu_ases);
-        cpu->cpu_ases = NULL;
-    }
+    g_clear_pointer(&cpu->cpu_ases, g_free);
 }
 
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
diff --git a/stubs/meson.build b/stubs/meson.build
index cef046e6854..5d577467bfd 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -55,6 +55,7 @@ endif
 if have_user
   # Symbols that are used by hw/core.
   stub_ss.add(files('cpu-synchronize-state.c'))
+  stub_ss.add(files('cpu-destroy-address-spaces.c'))
 
   # Stubs for QAPI events.  Those can always be included in the build, but
   # they are not built at all for --disable-system builds.
-- 
2.43.0

Re: [PATCH 0/3] system: Don't leak CPU AddressSpaces

[Philippe Mathieu-Daudé]

Cc'ing Salil for previous discussions on
https://lore.kernel.org/qemu-devel/20230918160257.30127-1-philmd@linaro.org/

On 29/9/25 16:42, Peter Maydell wrote:
> When a vCPU is created, it typically calls cpu_address_space_init()
> one or more times to set up its address spaces. We don't currently
> do anything to destroy these address spaces, which means that we
> will leak them on a vcpu hot-plug -> hot-unplug cycle.
> 
> This patchset fixes the leak by replacing the current
> cpu_address_space_destroy() (which has an awkward API, includes
> a bug, and is never called from anywhere) with a new
> cpu_destroy_address_spaces() which cleans up all the ASes a CPU
> has and is called from generic unrealize code.
> 
> Patch 1 is just a comment improvement to clarify that
> address_space_destroy() defers most of its work to RCU and you
> can't free the memory for the AS struct itself until it's done.
> 
> Patch 2 is from Peter Xu; we need to be able to do "destroy and
> free an AS" via RCU, and at the moment you can't do that.
> 
> Patch 3 is the bugfix proper.
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    include/system/memory.h: Clarify address_space_destroy() behaviour
>    physmem: Destroy all CPU AddressSpaces on unrealize
> 
> Peter Xu (1):
>    memory: New AS helper to serialize destroy+free
> 
>   include/exec/cpu-common.h          | 10 ++++-----
>   include/hw/core/cpu.h              |  1 -
>   include/system/memory.h            | 24 ++++++++++++++++++---
>   hw/core/cpu-common.c               |  1 +
>   stubs/cpu-destroy-address-spaces.c | 15 +++++++++++++
>   system/memory.c                    | 20 +++++++++++++++++-
>   system/physmem.c                   | 34 ++++++++++++++----------------
>   stubs/meson.build                  |  1 +
>   8 files changed, 78 insertions(+), 28 deletions(-)
>   create mode 100644 stubs/cpu-destroy-address-spaces.c
> 

Re: [PATCH 3/3] physmem: Destroy all CPU AddressSpaces on unrealize

[Philippe Mathieu-Daudé]

On 29/9/25 16:42, Peter Maydell wrote:
> When we unrealize a CPU object (which happens on vCPU hot-unplug), we
> should destroy all the AddressSpace objects we created via calls to
> cpu_address_space_init() when the CPU was realized.
> 
> Commit 24bec42f3d6eae added a function to do this for a specific
> AddressSpace, but did not add any places where the function was
> called.
> 
> Since we always want to destroy all the AddressSpaces on unrealize,
> regardless of the target architecture, we don't need to try to keep
> track of how many are still undestroyed, or make the target
> architecture code manually call a destroy function for each AS it
> created.  Instead we can adjust the function to always completely
> destroy the whole cpu->ases array, and arrange for it to be called
> during CPU unrealize as part of the common code.
> 
> Without this fix, AddressSanitizer will report a leak like this
> from a run where we hot-plugged and then hot-unplugged an x86 KVM
> vCPU:
> 
> Direct leak of 416 byte(s) in 1 object(s) allocated from:
>      #0 0x5b638565053d in calloc (/data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/qemu-system-x86_64+0x1ee153d) (BuildId: c1cd6022b195142106e1bffeca23498c2b752bca)
>      #1 0x7c28083f77b1 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x637b1) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>      #2 0x5b6386999c7c in cpu_address_space_init /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../system/physmem.c:797:25
>      #3 0x5b638727f049 in kvm_cpu_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../target/i386/kvm/kvm-cpu.c:102:5
>      #4 0x5b6385745f40 in accel_cpu_common_realize /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../accel/accel-common.c:101:13
>      #5 0x5b638568fe3c in cpu_exec_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/cpu-common.c:232:10
>      #6 0x5b63874a2cd5 in x86_cpu_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../target/i386/cpu.c:9321:5
>      #7 0x5b6387a0469a in device_set_realized /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/qdev.c:494:13
>      #8 0x5b6387a27d9e in property_set_bool /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:2375:5
>      #9 0x5b6387a2090b in object_property_set /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:1450:5
>      #10 0x5b6387a35b05 in object_property_set_qobject /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/qom-qobject.c:28:10
>      #11 0x5b6387a21739 in object_property_set_bool /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:1520:15
>      #12 0x5b63879fe510 in qdev_realize /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/qdev.c:276:12
> 
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2517
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/exec/cpu-common.h          | 10 ++++-----
>   include/hw/core/cpu.h              |  1 -
>   hw/core/cpu-common.c               |  1 +
>   stubs/cpu-destroy-address-spaces.c | 15 +++++++++++++
>   system/physmem.c                   | 34 ++++++++++++++----------------
>   stubs/meson.build                  |  1 +
>   6 files changed, 38 insertions(+), 24 deletions(-)
>   create mode 100644 stubs/cpu-destroy-address-spaces.c


> -void cpu_address_space_destroy(CPUState *cpu, int asidx)
> +void cpu_destroy_address_spaces(CPUState *cpu)
>   {
>       CPUAddressSpace *cpuas;
> +    int asidx;
>   
>       assert(cpu->cpu_ases);
> -    assert(asidx >= 0 && asidx < cpu->num_ases);
>   
> -    cpuas = &cpu->cpu_ases[asidx];
> -    if (tcg_enabled()) {
> -        memory_listener_unregister(&cpuas->tcg_as_listener);
> +    /* convenience alias just points to some cpu_ases[n] */
> +    cpu->as = NULL;
> +
> +    for (asidx = 0; asidx < cpu->num_ases; asidx++) {
> +        cpuas = &cpu->cpu_ases[asidx];
> +        if (!cpuas->as) {
> +            /* This index was never initialized; no deinit needed */
> +            continue;
> +        }
> +        if (tcg_enabled()) {
> +            memory_listener_unregister(&cpuas->tcg_as_listener);
> +        }
> +        g_clear_pointer(&cpuas->as, address_space_destroy_free);
>       }
>   
> -    address_space_destroy(cpuas->as);
> -    g_free_rcu(cpuas->as, rcu);
> -
> -    if (asidx == 0) {
> -        /* reset the convenience alias for address space 0 */
> -        cpu->as = NULL;
> -    }
> -
> -    if (--cpu->cpu_ases_count == 0) {
> -        g_free(cpu->cpu_ases);
> -        cpu->cpu_ases = NULL;
> -    }
> +    g_clear_pointer(&cpu->cpu_ases, g_free);
>   }

Good, this API respects Richard's suggestion on previous attempt:
https://lore.kernel.org/qemu-devel/594b2550-9a73-684f-6e54-29401dc6cd7a@linaro.org/

Re: [PATCH 1/3] include/system/memory.h: Clarify address_space_destroy() behaviour

[David Hildenbrand]

On 29.09.25 16:42, Peter Maydell wrote:
> address_space_destroy() doesn't actually immediately destroy the AS;
> it queues it to be destroyed via RCU. This means you can't g_free()
> the memory the AS struct is in until that has happened.
> 
> Clarify this in the documentation.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/system/memory.h | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/system/memory.h b/include/system/memory.h
> index aa85fc27a10..827e2c5aa44 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -2727,9 +2727,14 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
>   /**
>    * address_space_destroy: destroy an address space
>    *
> - * Releases all resources associated with an address space.  After an address space
> - * is destroyed, its root memory region (given by address_space_init()) may be destroyed
> - * as well.
> + * Releases all resources associated with an address space.  After an
> + * address space is destroyed, the reference the AddressSpace had to
> + * its root memory region is dropped, which may result in the
> + * destruction of that memory region as well.
> + *
> + * Note that destruction of the AddressSpace is done via RCU;
> + * it is therefore not valid to free the memory the AddressSpace
> + * struct is in until after that RCU callback has completed.
>    *
>    * @as: address space to be destroyed
>    */

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH 2/3] memory: New AS helper to serialize destroy+free

[David Hildenbrand]

On 29.09.25 16:42, Peter Maydell wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> If an AddressSpace has been created in its own allocated
> memory, cleaning it up requires first destroying the AS
> and then freeing the memory. Doing this doesn't work:
> 
>      address_space_destroy(as);
>      g_free_rcu(as, rcu);
> 
> because both address_space_destroy() and g_free_rcu()
> try to use the same 'rcu' node in the AddressSpace struct
> and the address_space_destroy hook gets overwritten.
> 
> Provide a new address_space_destroy_free() function which
> will destroy the AS and then free the memory it uses, all
> in one RCU callback.
> 
> (CC to stable because the next commit needs this function.)
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> [PMM: Expanded commit message with motivation, tweaked comment]
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH 3/3] physmem: Destroy all CPU AddressSpaces on unrealize

[David Hildenbrand]

On 29.09.25 16:42, Peter Maydell wrote:
> When we unrealize a CPU object (which happens on vCPU hot-unplug), we
> should destroy all the AddressSpace objects we created via calls to
> cpu_address_space_init() when the CPU was realized.
> 
> Commit 24bec42f3d6eae added a function to do this for a specific
> AddressSpace, but did not add any places where the function was
> called.
> 
> Since we always want to destroy all the AddressSpaces on unrealize,
> regardless of the target architecture, we don't need to try to keep
> track of how many are still undestroyed, or make the target
> architecture code manually call a destroy function for each AS it
> created.  Instead we can adjust the function to always completely
> destroy the whole cpu->ases array, and arrange for it to be called
> during CPU unrealize as part of the common code.
> 
> Without this fix, AddressSanitizer will report a leak like this
> from a run where we hot-plugged and then hot-unplugged an x86 KVM
> vCPU:
> 
> Direct leak of 416 byte(s) in 1 object(s) allocated from:
>      #0 0x5b638565053d in calloc (/data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/qemu-system-x86_64+0x1ee153d) (BuildId: c1cd6022b195142106e1bffeca23498c2b752bca)
>      #1 0x7c28083f77b1 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x637b1) (BuildId: 1eb6131419edb83b2178b682829a6913cf682d75)
>      #2 0x5b6386999c7c in cpu_address_space_init /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../system/physmem.c:797:25
>      #3 0x5b638727f049 in kvm_cpu_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../target/i386/kvm/kvm-cpu.c:102:5
>      #4 0x5b6385745f40 in accel_cpu_common_realize /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../accel/accel-common.c:101:13
>      #5 0x5b638568fe3c in cpu_exec_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/cpu-common.c:232:10
>      #6 0x5b63874a2cd5 in x86_cpu_realizefn /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../target/i386/cpu.c:9321:5
>      #7 0x5b6387a0469a in device_set_realized /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/qdev.c:494:13
>      #8 0x5b6387a27d9e in property_set_bool /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:2375:5
>      #9 0x5b6387a2090b in object_property_set /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:1450:5
>      #10 0x5b6387a35b05 in object_property_set_qobject /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/qom-qobject.c:28:10
>      #11 0x5b6387a21739 in object_property_set_bool /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../qom/object.c:1520:15
>      #12 0x5b63879fe510 in qdev_realize /data_nvme1n1/linaro/qemu-from-laptop/qemu/build/x86-tgts-asan/../../hw/core/qdev.c:276:12
> 
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2517
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Complicated stuff, but LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH 0/3] system: Don't leak CPU AddressSpaces

[Peter Xu]

On Mon, Sep 29, 2025 at 05:02:40PM +0200, Philippe Mathieu-Daudé wrote:
> Cc'ing Salil for previous discussions on
> https://lore.kernel.org/qemu-devel/20230918160257.30127-1-philmd@linaro.org/

Thanks.

While waiting for more comments, I pre-queued this series.

Regarding to patch 1: I still think it's almost impossible to use it
correctly on address_space_destroy(), even with the doc.. but I agree
that's the best we can have right now.

The problem is, afaiu we don't even have a way to know when a rcu callback
is processed, unlike the grace period (which corresponds to synchronize_rcu()).

I think it means there's almost no existing way for the user to properly
release the AddressSpace memory if it is e.g. embeded inside a Device
object.  Here, we will need to rcu-free the Device object too (which I
don't think we do at all..), making sure it's called after ASes' rcu
callback.

Even if we'll do so, we'll have yet another problem of having the Device
free() rcu callback to happen after the AddressSpace's destroy rcu
callback, we could rely on what Paolo mentioned on FIFO behavior of rcu
queues, but it looks unwanted.

For the long term, IMHO we may need to convert all embeded AddressSpaces
into using dynamically allocated AddressSpaces (for CPUs, we can stick with
cpu's address space API, but we have other places using ASes too that may
also need to be converted to heap allocations).  Then after all users
switching to that we may be able to drop address_space_destroy().

Thanks,

-- 
Peter Xu

Re: [PATCH 0/3] system: Don't leak CPU AddressSpaces

[Richard Henderson]

On 9/29/25 07:42, Peter Maydell wrote:
> When a vCPU is created, it typically calls cpu_address_space_init()
> one or more times to set up its address spaces. We don't currently
> do anything to destroy these address spaces, which means that we
> will leak them on a vcpu hot-plug -> hot-unplug cycle.
> 
> This patchset fixes the leak by replacing the current
> cpu_address_space_destroy() (which has an awkward API, includes
> a bug, and is never called from anywhere) with a new
> cpu_destroy_address_spaces() which cleans up all the ASes a CPU
> has and is called from generic unrealize code.
> 
> Patch 1 is just a comment improvement to clarify that
> address_space_destroy() defers most of its work to RCU and you
> can't free the memory for the AS struct itself until it's done.
> 
> Patch 2 is from Peter Xu; we need to be able to do "destroy and
> free an AS" via RCU, and at the moment you can't do that.
> 
> Patch 3 is the bugfix proper.

So... I wonder this is really the right direction.

To be specific:

Over in split-accel-land we recently had a bit of a kerfuffle over TCG registering its 
MemoryListener with all of the per-cpu address spaces and HVF not doing so.  Things get 
even more complicated when one finds out that some MemoryListener callbacks are only used 
with "global" address spaces and some are only used with the per-cpu address spaces.

On reflection, it seems to me that no address spaces should be owned by the cpus.  All 
address spaces should be owned by the board model, and it should be the board model that 
configures the address spaces used by each cpu.

If we do this then address spaces, and even the array of address spaces, may be created 
once by the board, shared between all (relevant) cpus, and not destroyed by cpu unplug.

Moreover, in the context of virtualization, there's now exactly one address space (since 
Arm Secure and Tag memory spaces are not required or supported except by emulation; I 
assume the same is true for x86 smm), and the aforementioned kerfuffle goes away.  There's 
no difference between global and per-cpu address spaces.

Anyway, it seems like this provides the same flexibility in the complex heterogeneous case 
and simplifies things for the homogeneous virtualization case.

Thoughts?


r~

Re: [PATCH 0/3] system: Don't leak CPU AddressSpaces

[Peter Xu]

On Wed, Oct 01, 2025 at 01:36:16PM -0700, Richard Henderson wrote:
> On 9/29/25 07:42, Peter Maydell wrote:
> > When a vCPU is created, it typically calls cpu_address_space_init()
> > one or more times to set up its address spaces. We don't currently
> > do anything to destroy these address spaces, which means that we
> > will leak them on a vcpu hot-plug -> hot-unplug cycle.
> > 
> > This patchset fixes the leak by replacing the current
> > cpu_address_space_destroy() (which has an awkward API, includes
> > a bug, and is never called from anywhere) with a new
> > cpu_destroy_address_spaces() which cleans up all the ASes a CPU
> > has and is called from generic unrealize code.
> > 
> > Patch 1 is just a comment improvement to clarify that
> > address_space_destroy() defers most of its work to RCU and you
> > can't free the memory for the AS struct itself until it's done.
> > 
> > Patch 2 is from Peter Xu; we need to be able to do "destroy and
> > free an AS" via RCU, and at the moment you can't do that.
> > 
> > Patch 3 is the bugfix proper.
> 
> So... I wonder this is really the right direction.
> 
> To be specific:
> 
> Over in split-accel-land we recently had a bit of a kerfuffle over TCG
> registering its MemoryListener with all of the per-cpu address spaces and
> HVF not doing so.  Things get even more complicated when one finds out that
> some MemoryListener callbacks are only used with "global" address spaces and
> some are only used with the per-cpu address spaces.

I only have a very preliminary understanding on this, so please bare with
me if I'll make silly mistakes...

I was expecting QEMU provides both the global view (address_space_memory),
and the per-vcpu view.  Then we can register to any of them on demand.
Then the global views can be the so-called "board model" mentioned below.

Is it not the case?

The root MR is also shared in this case, making sure the address space
operations internally will share the same flatview.

> 
> On reflection, it seems to me that no address spaces should be owned by the
> cpus.  All address spaces should be owned by the board model, and it should
> be the board model that configures the address spaces used by each cpu.
> 
> If we do this then address spaces, and even the array of address spaces, may
> be created once by the board, shared between all (relevant) cpus, and not
> destroyed by cpu unplug.
> 
> Moreover, in the context of virtualization, there's now exactly one address
> space (since Arm Secure and Tag memory spaces are not required or supported
> except by emulation; I assume the same is true for x86 smm), and the
> aforementioned kerfuffle goes away.  There's no difference between global
> and per-cpu address spaces.
> 
> Anyway, it seems like this provides the same flexibility in the complex
> heterogeneous case and simplifies things for the homogeneous virtualization
> case.

We have another question to ask besides this design discussion: Peter's
series here solidly fixes a memory leak that can easily reproduce with
x86_64 + KVM on cpu hotplugs.

Should we still merge it first considering it didn't change how we manage
per-vcpu address spaces, but only fixing it?  Then anything like a big
overhaul can happen on top too.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/3] system: Don't leak CPU AddressSpaces

[Richard Henderson]

On 10/1/25 14:37, Peter Xu wrote:
> I only have a very preliminary understanding on this, so please bare with
> me if I'll make silly mistakes...
> 
> I was expecting QEMU provides both the global view (address_space_memory),
> and the per-vcpu view.

My hypothesis is that separate views for each cpu doesn't make sense.

In the true symmetric multiprocessor case, which is the vast majority of everything we 
emulate in QEMU, each processor has the exact same view(s).

In the rare asymmetric multiprocessor case, of which we have a few, we have a couple of 
clusters and within each cluster all cpus share the exact same view(s).

Thus access to address spaces on a per-cpu basis makes sense, but allocating them on a 
per-cpu basis does not.

> We have another question to ask besides this design discussion: Peter's
> series here solidly fixes a memory leak that can easily reproduce with
> x86_64 + KVM on cpu hotplugs.
> 
> Should we still merge it first considering it didn't change how we manage
> per-vcpu address spaces, but only fixing it?  Then anything like a big
> overhaul can happen on top too.

Sure, I'm happy with that.  I just wanted to raise the related problem that we encountered 
over this past week.



r~

Re: [PATCH 0/3] system: Don't leak CPU AddressSpaces

[Peter Maydell]

On Wed, 1 Oct 2025 at 22:59, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/1/25 14:37, Peter Xu wrote:
> > I only have a very preliminary understanding on this, so please bare with
> > me if I'll make silly mistakes...
> >
> > I was expecting QEMU provides both the global view (address_space_memory),
> > and the per-vcpu view.
>
> My hypothesis is that separate views for each cpu doesn't make sense.
>
> In the true symmetric multiprocessor case, which is the vast majority of everything we
> emulate in QEMU, each processor has the exact same view(s).

This is not the case in general. Some CPUs have per-CPU
devices which show up only for that CPU. Examples include
the A9's per-CPU peripherals (which we don't currently model
that way, but perhaps ought to) and some stuff in one of the
M-profile boards.

TCG registers its CPU listener with every AS because
the information that TCG is caching that is stale is
something it cares about for every AS the CPU has to
deal with.

HVF and KVM register their CPU listener only with
address_space_memory because the information that those
accelerators are caching is the details of RAM layout in
the system, which in both cases the kernel imposes
the limitation of being the same for every CPU.
Similarly in target/arm's kvm_arm_register_device()
we listen on address_space_memory because the thing
we're listening for is "where has this device's
MemoryRegion been put", and again the kernel imposes
the requirement that the GIC (or whatever) is at the
same location for all CPUs.

The "global" view of address_space_memory and
system_memory is a bit theoretically dubious because
there's nothing saying all CPUs have to have some
underlying mostly-the-same view of things. But in
practice almost all systems do, plus KVM and HVF impose
that world-model, so it's easier to retain this than try
to think about how we would eliminate it.

> In the rare asymmetric multiprocessor case, of which we have a few, we have a couple of
> clusters and within each cluster all cpus share the exact same view(s).
>
> Thus access to address spaces on a per-cpu basis makes sense, but allocating them on a
> per-cpu basis does not.

Also, in general, per-CPU address spaces fits the rest of
how we model memory-transaction-capable devices. The
device (e.g. a DMA controller or similar) has a QOM
property which takes a pointer to a MemoryRegion describing
the view of the world that that device has; it then internally
creates an AddressSpace whose purpose is to provide the
APIs for making memory transactions into that MemoryRegion.

cpu_address_space_init() is just some convenience wrapping
around that.

thanks
-- PMM