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

[Chenyi Qiang]

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.

> 
>> +
>> +    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().

> 
>>   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
use 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.

> 
>>       }
>>         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);
>>       }
>>   
>