Re: [PATCH v2 3/6] memory-attribute-manager: Introduce MemoryAttributeManager to manage RAMBLock with guest_memfd

[Alexey Kardashevskiy <aik@amd.com>]

On 19/2/25 12:20, Chenyi Qiang wrote:
> 
> 
> On 2/18/2025 5:19 PM, Alexey Kardashevskiy wrote:
>>
>>
> 
> [..]
> 
>>> diff --git a/include/system/memory-attribute-manager.h b/include/
>>> system/memory-attribute-manager.h
>>> new file mode 100644
>>> index 0000000000..72adc0028e
>>> --- /dev/null
>>> +++ b/include/system/memory-attribute-manager.h
>>> @@ -0,0 +1,42 @@
>>> +/*
>>> + * QEMU memory attribute manager
>>> + *
>>> + * Copyright Intel
>>> + *
>>> + * Author:
>>> + *      Chenyi Qiang <chenyi.qiang@intel.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> later.
>>> + * See the COPYING file in the top-level directory
>>> + *
>>> + */
>>> +
>>> +#ifndef SYSTEM_MEMORY_ATTRIBUTE_MANAGER_H
>>> +#define SYSTEM_MEMORY_ATTRIBUTE_MANAGER_H
>>> +
>>> +#include "system/hostmem.h"
>>> +
>>> +#define TYPE_MEMORY_ATTRIBUTE_MANAGER "memory-attribute-manager"
>>> +
>>> +OBJECT_DECLARE_TYPE(MemoryAttributeManager,
>>> MemoryAttributeManagerClass, MEMORY_ATTRIBUTE_MANAGER)
>>> +
>>> +struct MemoryAttributeManager {
>>> +    Object parent;
>>> +
>>> +    MemoryRegion *mr;
>>> +
>>> +    /* 1-setting of the bit represents the memory is populated
>>> (shared) */
>>> +    int32_t bitmap_size;
>>
>> unsigned.
>>
>> Also, do either s/bitmap_size/shared_bitmap_size/ or
>> s/shared_bitmap/bitmap/
> 
> Will change it. Thanks.
> 
>>
>>
>>
>>> +    unsigned long *shared_bitmap;
>>> +
>>> +    QLIST_HEAD(, RamDiscardListener) rdl_list;
>>> +};
>>> +
>>> +struct MemoryAttributeManagerClass {
>>> +    ObjectClass parent_class;
>>> +};
>>> +
>>> +int memory_attribute_manager_realize(MemoryAttributeManager *mgr,
>>> MemoryRegion *mr);
>>> +void memory_attribute_manager_unrealize(MemoryAttributeManager *mgr);
>>> +
>>> +#endif
>>> diff --git a/system/memory-attribute-manager.c b/system/memory-
>>> attribute-manager.c
>>> new file mode 100644
>>> index 0000000000..ed97e43dd0
>>> --- /dev/null
>>> +++ b/system/memory-attribute-manager.c
>>> @@ -0,0 +1,292 @@
>>> +/*
>>> + * QEMU memory attribute manager
>>> + *
>>> + * Copyright Intel
>>> + *
>>> + * Author:
>>> + *      Chenyi Qiang <chenyi.qiang@intel.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> later.
>>> + * See the COPYING file in the top-level directory
>>> + *
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/error-report.h"
>>> +#include "system/memory-attribute-manager.h"
>>> +
>>> +OBJECT_DEFINE_TYPE_WITH_INTERFACES(MemoryAttributeManager,
>>> +                                   memory_attribute_manager,
>>> +                                   MEMORY_ATTRIBUTE_MANAGER,
>>> +                                   OBJECT,
>>> +                                   { TYPE_RAM_DISCARD_MANAGER },
>>> +                                   { })
>>> +
>>> +static int memory_attribute_manager_get_block_size(const
>>> MemoryAttributeManager *mgr)
>>> +{
>>> +    /*
>>> +     * Because page conversion could be manipulated in the size of at
>>> least 4K or 4K aligned,
>>> +     * Use the host page size as the granularity to track the memory
>>> attribute.
>>> +     * TODO: if necessary, switch to get the page_size from RAMBlock.
>>> +     * i.e. mgr->mr->ram_block->page_size.
>>
>> I'd assume it is rather necessary already.
> 
> OK, Will return the page_size of RAMBlock directly.
> 
>>
>>> +     */
>>> +    return qemu_real_host_page_size();
>>> +}
>>> +
>>> +
>>> +static bool memory_attribute_rdm_is_populated(const RamDiscardManager
>>> *rdm,
>>> +                                              const
>>> MemoryRegionSection *section)
>>> +{
>>> +    const MemoryAttributeManager *mgr = MEMORY_ATTRIBUTE_MANAGER(rdm);
>>> +    int block_size = memory_attribute_manager_get_block_size(mgr);
>>> +    uint64_t first_bit = section->offset_within_region / block_size;
>>> +    uint64_t last_bit = first_bit + int128_get64(section->size) /
>>> block_size - 1;
>>> +    unsigned long first_discard_bit;
>>> +
>>> +    first_discard_bit = find_next_zero_bit(mgr->shared_bitmap,
>>> last_bit + 1, first_bit);
>>> +    return first_discard_bit > last_bit;
>>> +}
>>> +
>>> +typedef int (*memory_attribute_section_cb)(MemoryRegionSection *s,
>>> void *arg);
>>> +
>>> +static int memory_attribute_notify_populate_cb(MemoryRegionSection
>>> *section, void *arg)
>>> +{
>>> +    RamDiscardListener *rdl = arg;
>>> +
>>> +    return rdl->notify_populate(rdl, section);
>>> +}
>>> +
>>> +static int memory_attribute_notify_discard_cb(MemoryRegionSection
>>> *section, void *arg)
>>> +{
>>> +    RamDiscardListener *rdl = arg;
>>> +
>>> +    rdl->notify_discard(rdl, section);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int memory_attribute_for_each_populated_section(const
>>> MemoryAttributeManager *mgr,
>>> +
>>> MemoryRegionSection *section,
>>> +                                                       void *arg,
>>> +
>>> memory_attribute_section_cb cb)
>>> +{
>>> +    unsigned long first_one_bit, last_one_bit;
>>> +    uint64_t offset, size;
>>> +    int block_size = memory_attribute_manager_get_block_size(mgr);
>>> +    int ret = 0;
>>> +
>>> +    first_one_bit = section->offset_within_region / block_size;
>>> +    first_one_bit = find_next_bit(mgr->shared_bitmap, mgr-
>>>> bitmap_size, first_one_bit);
>>> +
>>> +    while (first_one_bit < mgr->bitmap_size) {
>>> +        MemoryRegionSection tmp = *section;
>>> +
>>> +        offset = first_one_bit * block_size;
>>> +        last_one_bit = find_next_zero_bit(mgr->shared_bitmap, mgr-
>>>> bitmap_size,
>>> +                                          first_one_bit + 1) - 1;
>>> +        size = (last_one_bit - first_one_bit + 1) * block_size;
>>
>>
>> What all this math is for if we stuck with VFIO doing 1 page at the
>> time? (I think I commented on this)
> 
> Sorry, I missed your previous comment. IMHO, as we track the status in
> bitmap and we want to call the cb() on the shared part within
> MemoryRegionSection. Here we do the calculation to find the expected
> sub-range.


You find a largest intersection here and call cb() on it which will call 
VFIO with 1 page at the time. So you could just call cb() for every page 
from here which will make the code simpler.


>>
>>> +
>>> +        if (!memory_region_section_intersect_range(&tmp, offset,
>>> size)) {
>>> +            break;
>>> +        }
>>> +
>>> +        ret = cb(&tmp, arg);
>>> +        if (ret) {
>>> +            error_report("%s: Failed to notify RAM discard listener:
>>> %s", __func__,
>>> +                         strerror(-ret));
>>> +            break;
>>> +        }
>>> +
>>> +        first_one_bit = find_next_bit(mgr->shared_bitmap, mgr-
>>>> bitmap_size,
>>> +                                      last_one_bit + 2);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
> 
> [..]
> 
>>> +
>>> +static void
>>> memory_attribute_rdm_unregister_listener(RamDiscardManager *rdm,
>>> +
>>> RamDiscardListener *rdl)
>>> +{
>>> +    MemoryAttributeManager *mgr = MEMORY_ATTRIBUTE_MANAGER(rdm);
>>> +    int ret;
>>> +
>>> +    g_assert(rdl->section);
>>> +    g_assert(rdl->section->mr == mgr->mr);
>>> +
>>> +    ret = memory_attribute_for_each_populated_section(mgr, rdl-
>>>> section, rdl,
>>> +
>>> memory_attribute_notify_discard_cb);
>>> +    if (ret) {
>>> +        error_report("%s: Failed to unregister RAM discard listener:
>>> %s", __func__,
>>> +                     strerror(-ret));
>>> +    }
>>> +
>>> +    memory_region_section_free_copy(rdl->section);
>>> +    rdl->section = NULL;
>>> +    QLIST_REMOVE(rdl, next);
>>> +
>>> +}
>>> +
>>> +typedef struct MemoryAttributeReplayData {
>>> +    void *fn;
>>
>> ReplayRamDiscard *fn, not void*.
> 
> We could cast the void *fn either to ReplayRamPopulate or
> ReplayRamDiscard (see below).


Hard to read, hard to maintain, and they take same parameters, only the 
return value is different (int/void) - if this is really important, have 
2 fn pointers in MemoryAttributeReplayData. It is already hard to follow 
this train on callbacks.


>>> +    void *opaque;
>>> +} MemoryAttributeReplayData;
>>> +
>>> +static int
>>> memory_attribute_rdm_replay_populated_cb(MemoryRegionSection *section,
>>> void *arg)
>>> +{
>>> +    MemoryAttributeReplayData *data = arg;
>>> +
>>> +    return ((ReplayRamPopulate)data->fn)(section, data->opaque);
>>> +}
>>> +
>>> +static int memory_attribute_rdm_replay_populated(const
>>> RamDiscardManager *rdm,
>>> +                                                 MemoryRegionSection
>>> *section,
>>> +                                                 ReplayRamPopulate
>>> replay_fn,
>>> +                                                 void *opaque)
>>> +{
>>> +    MemoryAttributeManager *mgr = MEMORY_ATTRIBUTE_MANAGER(rdm);
>>> +    MemoryAttributeReplayData data = { .fn = replay_fn, .opaque =
>>> opaque };
>>> +
>>> +    g_assert(section->mr == mgr->mr);
>>> +    return memory_attribute_for_each_populated_section(mgr, section,
>>> &data,
>>> +
>>> memory_attribute_rdm_replay_populated_cb);
>>> +}
>>> +
>>> +static int
>>> memory_attribute_rdm_replay_discarded_cb(MemoryRegionSection *section,
>>> void *arg)
>>> +{
>>> +    MemoryAttributeReplayData *data = arg;
>>> +
>>> +    ((ReplayRamDiscard)data->fn)(section, data->opaque);
>>> +    return 0;
>>> +}
>>> +
>>> +static void memory_attribute_rdm_replay_discarded(const
>>> RamDiscardManager *rdm,
>>> +                                                  MemoryRegionSection
>>> *section,
>>> +                                                  ReplayRamDiscard
>>> replay_fn,
>>> +                                                  void *opaque)
>>> +{
>>> +    MemoryAttributeManager *mgr = MEMORY_ATTRIBUTE_MANAGER(rdm);
>>> +    MemoryAttributeReplayData data = { .fn = replay_fn, .opaque =
>>> opaque };
>>> +
>>> +    g_assert(section->mr == mgr->mr);
>>> +    memory_attribute_for_each_discarded_section(mgr, section, &data,
>>> +
>>> memory_attribute_rdm_replay_discarded_cb);
>>> +}
>>> +
>>> +int memory_attribute_manager_realize(MemoryAttributeManager *mgr,
>>> MemoryRegion *mr)
>>> +{
>>> +    uint64_t bitmap_size;
>>> +    int block_size = memory_attribute_manager_get_block_size(mgr);
>>> +    int ret;
>>> +
>>> +    bitmap_size = ROUND_UP(mr->size, block_size) / block_size;
>>> +
>>> +    mgr->mr = mr;
>>> +    mgr->bitmap_size = bitmap_size;
>>> +    mgr->shared_bitmap = bitmap_new(bitmap_size);
>>> +
>>> +    ret = memory_region_set_ram_discard_manager(mgr->mr,
>>> RAM_DISCARD_MANAGER(mgr));
>>
>> Move it 3 lines up and avoid stale data in mgr->mr/bitmap_size/
>> shared_bitmap and avoid g_free below?
> 
> Make sense. I will move it up the same as patch 02 before bitmap_new().
> 
>>
>>> +    if (ret) {
>>> +        g_free(mgr->shared_bitmap);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +void memory_attribute_manager_unrealize(MemoryAttributeManager *mgr)
>>> +{
>>> +    memory_region_set_ram_discard_manager(mgr->mr, NULL);
>>> +
>>> +    g_free(mgr->shared_bitmap);
>>> +}
>>> +
>>> +static void memory_attribute_manager_init(Object *obj)
>>
>> Not used.
>>
>>> +{
>>> +    MemoryAttributeManager *mgr = MEMORY_ATTRIBUTE_MANAGER(obj);
>>> +
>>> +    QLIST_INIT(&mgr->rdl_list);
>>> +} > +
>>> +static void memory_attribute_manager_finalize(Object *obj)
>>
>> Not used either. Thanks,
> 
> I think it is OK to define it as a placeholder? Just some preference.

At very least gcc should warn on these (I am surprised it did not) and 
nobody likes this. Thanks,


>>
>>> +{
>>> +}
>>> +
>>> +static void memory_attribute_manager_class_init(ObjectClass *oc, void
>>> *data)
>>> +{
>>> +    RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(oc);
>>> +
>>> +    rdmc->get_min_granularity =
>>> memory_attribute_rdm_get_min_granularity;
>>> +    rdmc->register_listener = memory_attribute_rdm_register_listener;
>>> +    rdmc->unregister_listener =
>>> memory_attribute_rdm_unregister_listener;
>>> +    rdmc->is_populated = memory_attribute_rdm_is_populated;
>>> +    rdmc->replay_populated = memory_attribute_rdm_replay_populated;
>>> +    rdmc->replay_discarded = memory_attribute_rdm_replay_discarded;
>>> +}
>>> diff --git a/system/meson.build b/system/meson.build
>>> index 4952f4b2c7..ab07ff1442 100644
>>> --- a/system/meson.build
>>> +++ b/system/meson.build
>>> @@ -15,6 +15,7 @@ system_ss.add(files(
>>>      'dirtylimit.c',
>>>      'dma-helpers.c',
>>>      'globals.c',
>>> +  'memory-attribute-manager.c',
>>>      'memory_mapping.c',
>>>      'qdev-monitor.c',
>>>      'qtest.c',
>>
> 

-- 
Alexey