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

> On 6 Aug 2026, at 12:59 PM, Ani Sinha <[email protected]> wrote:
> 
> 
> 
>> 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;
          ^^^^^^^^^^^^^^^^
I suppose that is unnecessary as imr is getting freed right away.

>         g_free(imr);
>     }
> }