Re: [PATCH v3 2/9] igvm: track memory regions

> On 1 Aug 2026, at 7:57 PM, Ani Sinha <[email protected]> wrote:
> 
> On Sat, Aug 1, 2026 at 5:58 PM Ani Sinha <[email protected]> wrote:
>> 
>> 
>> 
>>> On 31 Jul 2026, at 3:28 PM, Alexander Graf <[email protected]> wrote:
>>> 
>>> 
>>> On 30.07.26 09:12, Ani Sinha wrote:
>>>> From: Gerd Hoffmann <[email protected]>
>>>> 
>>>> Memory regions added by the current IGVM needs to be tracked so that they can be
>>>> freed when a new IGVM is loaded.
>>>> 
>>>> Reviewed-by: Ani Sinha <[email protected]>
>>>> Signed-off-by: Gerd Hoffmann <[email protected]>
>>>> ---
>>>> backends/igvm-cfg.c            |  1 +
>>>> backends/igvm.c                | 20 ++++++++++++--------
>>>> include/system/igvm-internal.h |  6 ++++++
>>>> 3 files changed, 19 insertions(+), 8 deletions(-)
>>>> 
>>>> diff --git a/backends/igvm-cfg.c b/backends/igvm-cfg.c
>>>> index e1f09855f6..38438a7b1e 100644
>>>> --- a/backends/igvm-cfg.c
>>>> +++ b/backends/igvm-cfg.c
>>>> @@ -65,6 +65,7 @@ static void igvm_complete(UserCreatable *uc, Error **errp)
>>>>     IgvmCfg *igvm = IGVM_CFG(uc);
>>>>       igvm->file = qigvm_file_init(igvm->filename, errp);
>>>> +    QTAILQ_INIT(&igvm->memory_regions);
>>>> }
>>>>   OBJECT_DEFINE_TYPE_WITH_INTERFACES(IgvmCfg, igvm_cfg, IGVM_CFG, OBJECT,
>>>> diff --git a/backends/igvm.c b/backends/igvm.c
>>>> index 534032fed8..9e7c90d386 100644
>>>> --- a/backends/igvm.c
>>>> +++ b/backends/igvm.c
>>>> @@ -220,7 +220,7 @@ static void *qigvm_prepare_memory(QIgvm *ctx, uint64_t addr, uint64_t size,
>>>>                                   int region_identifier, Error **errp)
>>>> {
>>>>     ERRP_GUARD();
>>>> -    MemoryRegion *igvm_pages = NULL;
>>>> +    IgvmMemoryRegion *imr = NULL;
>>>>     Int128 gpa_region_size;
>>>>     MemoryRegionSection mrs =
>>>>         memory_region_find(get_system_memory(), addr, size);
>>>> @@ -254,23 +254,27 @@ static void *qigvm_prepare_memory(QIgvm *ctx, uint64_t addr, uint64_t size,
>>>>          */
>>>>         g_autofree char *region_name =
>>>>             g_strdup_printf("igvm.%X", region_identifier);
>>>> -        igvm_pages = g_new0(MemoryRegion, 1);
>>>> +        imr = g_new0(IgvmMemoryRegion, 1);
>>>> +        imr->mr = g_new0(MemoryRegion, 1);
>>>>         if (ctx->machine_state->cgs &&
>>>>             ctx->machine_state->cgs->require_guest_memfd) {
>>>> -            if (!memory_region_init_ram_guest_memfd(igvm_pages, NULL,
>>>> +            if (!memory_region_init_ram_guest_memfd(imr->mr, NULL,
>>> 
>>> 
>>> Please make sure to track them with owner set to OBJECT(ctx->machine_state).
>> 
>> This will not work as the owner will have to be of TYPE_DEVICE not of TYPE_MACHINE.
> 
> Looking closer, these memory regions are subregions of system_mmeory.
> system_memory is initialized with owner as NULL in memory_map_init().
> 
> memory_region_finalize() says .,,
> 
> Here it is possible that the MR has:
> 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).
> 
> And then there is this assertion:
> 
> /* Must share the owner; see above comments */
>        assert(mr->container->owner == mr->owner);
> 
> So i think the owner should be NULL.
> 
> I think that means we should garbage collect system_memory and its sub-regions?

I looked at this deeply and I believe this change triggers the mr cleanup:

diff --git a/backends/igvm.c b/backends/igvm.c
index da09e6195c..44cf74dfc6 100644
--- a/backends/igvm.c
+++ b/backends/igvm.c
@@ -1124,11 +1124,9 @@ void qigvm_cleanup_memory(IgvmCfg *cfg)
         memory_region_del_subregion(get_system_memory(), imr->mr);
         vmstate_unregister_ram(imr->mr, NULL);
         QTAILQ_REMOVE(&cfg->memory_regions, imr, next);
-        /*
-         * imr->mr will be freed when all references to the memory
-         * region are dropped. Freeing it here will cause memory
-         * corruption.
-         */
+        /* this triggers MemoryRegion cleanup */
+        object_unparent(OBJECT(imr->mr));
+        imr->mr = NULL;
         g_free(imr);
     }
 }


Using added tracing, I see the following in the log:

$ grep igvm /tmp/qemu-debug.log 
qigvm_cleanup_memory freeing mr igvm.23B
memory_region_finalize mr name igvm.23B
qigvm_cleanup_memory freeing mr igvm.23B
memory_region_finalize mr name igvm.23B


So the mr which is cleaned up in qigvm_cleanup_memory() is actually freed and there is no memory leak.

The cleanup triggered by object_unparent() has the following call chain as seen in debugger:

#0  memory_region_finalize (obj=0x55ef77ebbc30) at /workspace/qemu-ani/include/system/memory.h:41
#1  0x000055ef6b195261 in object_deinit (obj=<optimized out>, type=<optimized out>) at ../qom/object.c:655
#2  object_finalize (data=0x55ef77ebbc30) at ../qom/object.c:669
#3  object_unref (objptr=0x55ef77ebbc30) at ../qom/object.c:1324
#4  0x000055ef6b19385a in object_property_del_child (obj=0x55ef774fb900, child=0x55ef77ebbc30) at ../qom/object.c:629
#5  0x000055ef6b1944d1 in object_unparent (obj=<optimized out>) at ../qom/object.c:648
#6  0x000055ef6ad46c0e in qigvm_cleanup_memory (cfg=cfg@entry=0x55ef77506d80) at ../backends/igvm.c:1128
#7  0x000055ef6ad44c2b in igvm_reset_hold (obj=<optimized out>, type=RESET_TYPE_COLD) at ../backends/igvm-cfg.c:56

(gdb) n
memory_region_finalize (obj=<optimized out>) at ../system/memory.c:1733
1733        trace_memory_region_finalize(mr->name);
(gdb) p mr->name
$1 = 0x55ef7766dfa0 "igvm.23B"
(gdb)



> 
>>> Without owner, reference counting is disabled and we end up leaking memory regions on cleanup you introduce in patch 3 later.
>>> 
>>> 
>>> Alex