Thanks Zhao for your review! On 5/12/2025 4:07 PM, Zhao Liu wrote: > [snip] > >> --- >> include/exec/ramblock.h | 24 +++ >> system/meson.build | 1 + >> system/ram-block-attribute.c | 282 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 307 insertions(+) >> create mode 100644 system/ram-block-attribute.c > > checkpatch.pl complains a lot about code line length: > > total: 5 errors, 34 warnings, 324 lines checked
Thanks for reminder, I have adjusted indent locally and fixed most of them but still have some warnings for the function definition like: static uint64_t ram_block_attribute_rdm_get_min_granularity(const RamDiscardManager *rdm, const MemoryRegion *mr). The "rdm" argument in the same line with function name will exceed 80 width and I think it is acceptable to keep it. > >> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h >> index 0babd105c0..b8b5469db9 100644 >> --- a/include/exec/ramblock.h >> +++ b/include/exec/ramblock.h >> @@ -23,6 +23,10 @@ >> #include "cpu-common.h" >> #include "qemu/rcu.h" >> #include "exec/ramlist.h" >> +#include "system/hostmem.h" >> + >> +#define TYPE_RAM_BLOCK_ATTRIBUTE "ram-block-attribute" >> +OBJECT_DECLARE_TYPE(RamBlockAttribute, RamBlockAttributeClass, >> RAM_BLOCK_ATTRIBUTE) > > Could we use "OBJECT_DECLARE_SIMPLE_TYPE" here? Since I find class > doesn't have any virtual method. Yes, we can. Previously, I defined the state_change() method for the class (MemoryAttributeManagerClass) [1] instead of parent PrivateSharedManagerClass. And leave it unchanged in this version. In next version, I will drop PrivateShareManager and revert to use RamDiscardManager. Then, maybe I should also use OBJECT_DECLARE_SIMPLE_TYPE and make state_change() an exported function instead of a virtual method since no derived class for RamBlockAttribute. [1] https://lore.kernel.org/qemu-devel/20250310081837.13123-6-chenyi.qi...@intel.com/ > >> struct RAMBlock { >> struct rcu_head rcu; >> @@ -90,5 +94,25 @@ struct RAMBlock { >> */ >> ram_addr_t postcopy_length; >> }; >> + >> +struct RamBlockAttribute { >> + Object parent; >> + >> + MemoryRegion *mr; >> + >> + /* 1-setting of the bit represents the memory is populated (shared) */ >> + unsigned shared_bitmap_size; >> + unsigned long *shared_bitmap; >> + >> + QLIST_HEAD(, PrivateSharedListener) psl_list; >> +}; >> + >> +struct RamBlockAttributeClass { >> + ObjectClass parent_class; >> +}; > > With OBJECT_DECLARE_SIMPLE_TYPE, this class definition is not needed. > >> +int ram_block_attribute_realize(RamBlockAttribute *attr, MemoryRegion *mr); >> +void ram_block_attribute_unrealize(RamBlockAttribute *attr); >> + >> #endif >> #endif >> diff --git a/system/meson.build b/system/meson.build >> index 4952f4b2c7..50a5a64f1c 100644 >> --- a/system/meson.build >> +++ b/system/meson.build >> @@ -15,6 +15,7 @@ system_ss.add(files( >> 'dirtylimit.c', >> 'dma-helpers.c', >> 'globals.c', >> + 'ram-block-attribute.c', > > This new file is missing a MAINTAINERS entry. It is still uncertain to me if we need to introduce a new file or add the code in an existing one like system/physmem.c. Anyway, I can add the MAINTAINERS entry if no objection. > >> 'memory_mapping.c', >> 'qdev-monitor.c', >> 'qtest.c', > > [snip] > >> +static size_t ram_block_attribute_get_block_size(const RamBlockAttribute >> *attr) >> +{ >> + /* >> + * 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. >> + */ >> + g_assert(attr && attr->mr && attr->mr->ram_block); >> + g_assert(attr->mr->ram_block->page_size == qemu_real_host_page_size()); >> + return attr->mr->ram_block->page_size; > > What about using qemu_ram_pagesize() instead of accessing > ram_block->page_size directly? Make sense! > > Additionally, maybe we can add a simple helper to get page size from > RamBlockAttribute. Do you mean introduce a new field page_size and related helper? That was my first version and but suggested with current implementation (https://lore.kernel.org/qemu-devel/b55047fd-7b73-4669-b6d2-31653064f...@intel.com/) > >> +} >> + > > [snip] > >> +static void ram_block_attribute_psm_register_listener(GenericStateManager >> *gsm, >> + StateChangeListener >> *scl, >> + MemoryRegionSection >> *section) >> +{ >> + RamBlockAttribute *attr = RAM_BLOCK_ATTRIBUTE(gsm); >> + PrivateSharedListener *psl = container_of(scl, PrivateSharedListener, >> scl); >> + int ret; >> + >> + g_assert(section->mr == attr->mr); >> + scl->section = memory_region_section_new_copy(section); >> + >> + QLIST_INSERT_HEAD(&attr->psl_list, psl, next); >> + >> + ret = ram_block_attribute_for_each_shared_section(attr, section, scl, >> + >> ram_block_attribute_notify_shared_cb); >> + if (ret) { >> + error_report("%s: Failed to register RAM discard listener: %s", >> __func__, >> + strerror(-ret)); > > There will be 2 error messages: one is the above, and another is from > ram_block_attribute_for_each_shared_section(). > > Could we just exit to handle this error? Sure, will remove this message as well as the below one. > >> + } >> +} >> + >> +static void ram_block_attribute_psm_unregister_listener(GenericStateManager >> *gsm, >> + StateChangeListener >> *scl) >> +{ >> + RamBlockAttribute *attr = RAM_BLOCK_ATTRIBUTE(gsm); >> + PrivateSharedListener *psl = container_of(scl, PrivateSharedListener, >> scl); >> + int ret; >> + >> + g_assert(scl->section); >> + g_assert(scl->section->mr == attr->mr); >> + >> + ret = ram_block_attribute_for_each_shared_section(attr, scl->section, >> scl, >> + >> ram_block_attribute_notify_private_cb); >> + if (ret) { >> + error_report("%s: Failed to unregister RAM discard listener: %s", >> __func__, >> + strerror(-ret)); > > Ditto. > >> + } >> + >> + memory_region_section_free_copy(scl->section); >> + scl->section = NULL; >> + QLIST_REMOVE(psl, next); >> +} >> +
