On 5/13/2025 4:31 PM, Zhao Liu wrote: >>>> 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. > > Thank you! I see. I don't have an opinion on whether to add virtual > method or not, if you feel it's appropriate then adding class is fine. > (My comment may be outdated, it's just for the fact that there is no > need to add class in this patch.) Looking forward to your next version. > >> [1] >> https://lore.kernel.org/qemu-devel/20250310081837.13123-6-chenyi.qiang@xxxxxxxxx/ >> >>> >>>> struct RAMBlock { >>>> struct rcu_head rcu; >>>> @@ -90,5 +94,25 @@ struct RAMBlock { >>>> */ >>>> ram_addr_t postcopy_length; >>>> }; >>>> + > > [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-31653064f27f@xxxxxxxxx/) > > Yes, that's exactly my point. It's up to you if it's really necessary :-). > >>> >>>> +} >>>> + >>> >>> [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. > > if (ret) { > error_report("%s: Failed to register RAM discard listener: %s", __func__, > strerror(-ret); > exit(1); > } > > I mean adding a exit() here. When there's the error, if we expect it not to > break the QEMU, then perhaps warning is better. Otherwise, it's better to > handle this error. Direct exit() feels like an option. Sorry for my misunderstanding. You are right, only warning may cause unexpected behavior especially after adding a new listener for changing attribute. Will add a direct exit() here. > > Thanks, > Zhao > >>> >>>> + } >>>> +} >>>> +
