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

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?

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