Re: [PATCH 5/7] memory: Register the RamDiscardManager instance upon guest_memfd creation

[Alexey Kardashevskiy]

On 9/1/25 16:34, Chenyi Qiang wrote:
> On 1/8/2025 12:47 PM, Alexey Kardashevskiy wrote:
> > On 13/12/24 18:08, Chenyi Qiang wrote:
> > > Introduce the realize()/unrealize() callbacks to initialize/uninitialize
> > > the new guest_memfd_manager object and register/unregister it in the
> > > target MemoryRegion.
> > >
> > > Guest_memfd was initially set to shared until the commit bd3bcf6962
> > > ("kvm/memory: Make memory type private by default if it has guest memfd
> > > backend"). To align with this change, the default state in
> > > guest_memfd_manager is set to private. (The bitmap is cleared to 0).
> > > Additionally, setting the default to private can also reduce the
> > > overhead of mapping shared pages into IOMMU by VFIO during the bootup
> > > stage.
> > >
> > > Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> > > ---
> > >    include/sysemu/guest-memfd-manager.h | 27 +++++++++++++++++++++++++++
> > >    system/guest-memfd-manager.c         | 28 +++++++++++++++++++++++++++-
> > >    system/physmem.c                     |  7 +++++++
> > >    3 files changed, 61 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/sysemu/guest-memfd-manager.h b/include/sysemu/
> > > guest-memfd-manager.h
> > > index 9dc4e0346d..d1e7f698e8 100644
> > > --- a/include/sysemu/guest-memfd-manager.h
> > > +++ b/include/sysemu/guest-memfd-manager.h
> > > @@ -42,6 +42,8 @@ struct GuestMemfdManager {
> > >    struct GuestMemfdManagerClass {
> > >        ObjectClass parent_class;
> > >    +    void (*realize)(GuestMemfdManager *gmm, MemoryRegion *mr,
> > > uint64_t region_size);
> > > +    void (*unrealize)(GuestMemfdManager *gmm);
> > >        int (*state_change)(GuestMemfdManager *gmm, uint64_t offset,
> > > uint64_t size,
> > >                            bool shared_to_private);
> > >    };
> > > @@ -61,4 +63,29 @@ static inline int
> > > guest_memfd_manager_state_change(GuestMemfdManager *gmm, uint6
> > >        return 0;
> > >    }
> > >    +static inline void guest_memfd_manager_realize(GuestMemfdManager *gmm,
> > > +                                              MemoryRegion *mr,
> > > uint64_t region_size)
> > > +{
> > > +    GuestMemfdManagerClass *klass;
> > > +
> > > +    g_assert(gmm);
> > > +    klass = GUEST_MEMFD_MANAGER_GET_CLASS(gmm);
> > > +
> > > +    if (klass->realize) {
> > > +        klass->realize(gmm, mr, region_size);
> >
> > Ditch realize() hook and call guest_memfd_manager_realizefn() directly?
> > Not clear why these new hooks are needed.
>
> > > +    }
> > > +}
> > > +
> > > +static inline void guest_memfd_manager_unrealize(GuestMemfdManager *gmm)
> > > +{
> > > +    GuestMemfdManagerClass *klass;
> > > +
> > > +    g_assert(gmm);
> > > +    klass = GUEST_MEMFD_MANAGER_GET_CLASS(gmm);
> > > +
> > > +    if (klass->unrealize) {
> > > +        klass->unrealize(gmm);
> > > +    }
> > > +}
> >
> > guest_memfd_manager_unrealizefn()?
>
> Agree. Adding these wrappers seem unnecessary.
>
> > > +
> > >    #endif
> > > diff --git a/system/guest-memfd-manager.c b/system/guest-memfd-manager.c
> > > index 6601df5f3f..b6a32f0bfb 100644
> > > --- a/system/guest-memfd-manager.c
> > > +++ b/system/guest-memfd-manager.c
> > > @@ -366,6 +366,31 @@ static int
> > > guest_memfd_state_change(GuestMemfdManager *gmm, uint64_t offset,
> > >        return ret;
> > >    }
> > >    +static void guest_memfd_manager_realizefn(GuestMemfdManager *gmm,
> > > MemoryRegion *mr,
> > > +                                          uint64_t region_size)
> > > +{
> > > +    uint64_t bitmap_size;
> > > +
> > > +    gmm->block_size = qemu_real_host_page_size();
> > > +    bitmap_size = ROUND_UP(region_size, gmm->block_size) / gmm-
> > > > block_size;
> >
> > imho unaligned region_size should be an assert.
>
> There's no guarantee the region_size of the MemoryRegion is PAGE_SIZE
> aligned. So the ROUND_UP() is more appropriate.

It is all about DMA so the smallest you can map is PAGE_SIZE so even if 
you round up here, it is likely going to fail to DMA-map later anyway 
(or not?).


> > > +
> > > +    gmm->mr = mr;
> > > +    gmm->bitmap_size = bitmap_size;
> > > +    gmm->bitmap = bitmap_new(bitmap_size);
> > > +
> > > +    memory_region_set_ram_discard_manager(gmm->mr,
> > > RAM_DISCARD_MANAGER(gmm));
> > > +}
> >
> > This belongs to 2/7.
> >
> > > +
> > > +static void guest_memfd_manager_unrealizefn(GuestMemfdManager *gmm)
> > > +{
> > > +    memory_region_set_ram_discard_manager(gmm->mr, NULL);
> > > +
> > > +    g_free(gmm->bitmap);
> > > +    gmm->bitmap = NULL;
> > > +    gmm->bitmap_size = 0;
> > > +    gmm->mr = NULL;
> >
> > @gmm is being destroyed here, why bother zeroing?
>
> OK, will remove it.
>
> > > +}
> > > +
> >
> > This function belongs to 2/7.
>
> Will move both realizefn() and unrealizefn().

Yes.


> > >    static void guest_memfd_manager_init(Object *obj)
> > >    {
> > >        GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(obj);
> > > @@ -375,7 +400,6 @@ static void guest_memfd_manager_init(Object *obj)
> > >      static void guest_memfd_manager_finalize(Object *obj)
> > >    {
> > > -    g_free(GUEST_MEMFD_MANAGER(obj)->bitmap);
> > >    }
> > >      static void guest_memfd_manager_class_init(ObjectClass *oc, void
> > > *data)
> > > @@ -384,6 +408,8 @@ static void
> > > guest_memfd_manager_class_init(ObjectClass *oc, void *data)
> > >        RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(oc);
> > >          gmmc->state_change = guest_memfd_state_change;
> > > +    gmmc->realize = guest_memfd_manager_realizefn;
> > > +    gmmc->unrealize = guest_memfd_manager_unrealizefn;
> > >          rdmc->get_min_granularity = guest_memfd_rdm_get_min_granularity;
> > >        rdmc->register_listener = guest_memfd_rdm_register_listener;
> > > diff --git a/system/physmem.c b/system/physmem.c
> > > index dc1db3a384..532182a6dd 100644
> > > --- a/system/physmem.c
> > > +++ b/system/physmem.c
> > > @@ -53,6 +53,7 @@
> > >    #include "sysemu/hostmem.h"
> > >    #include "sysemu/hw_accel.h"
> > >    #include "sysemu/xen-mapcache.h"
> > > +#include "sysemu/guest-memfd-manager.h"
> > >    #include "trace.h"
> > >      #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> > > @@ -1885,6 +1886,9 @@ static void ram_block_add(RAMBlock *new_block,
> > > Error **errp)
> > >                qemu_mutex_unlock_ramlist();
> > >                goto out_free;
> > >            }
> > > +
> > > +        GuestMemfdManager *gmm =
> > > GUEST_MEMFD_MANAGER(object_new(TYPE_GUEST_MEMFD_MANAGER));
> > > +        guest_memfd_manager_realize(gmm, new_block->mr, new_block-
> > > > mr->size);
> >
> > Wow. Quite invasive.
>
> Yeah... It creates a manager object no matter whether the user wants to
> us      e shared passthru or not. We assume some fields like private/shared
> bitmap may also be helpful in other scenario for future usage, and if no
> passthru device, the listener would just return, so it is acceptable.

Explain these other scenarios in the commit log please as otherwise 
making this an interface of HostMemoryBackendMemfd looks way cleaner. 
Thanks,

> > >        }
> > >          ram_size = (new_block->offset + new_block->max_length) >>
> > > TARGET_PAGE_BITS;
> > > @@ -2139,6 +2143,9 @@ static void reclaim_ramblock(RAMBlock *block)
> > >          if (block->guest_memfd >= 0) {
> > >            close(block->guest_memfd);
> > > +        GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(block->mr->rdm);
> > > +        guest_memfd_manager_unrealize(gmm);
> > > +        object_unref(OBJECT(gmm));
> >
> > Likely don't matter but I'd do the cleanup before close() or do block-
> > > guest_memfd=-1 before the cleanup. Thanks,
> >
> >
> > >            ram_block_discard_require(false);
> > >        }
> > >    

--
Alexey