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.qi...@intel.com/
>>
>>>
>>>> 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-31653064f...@intel.com/)
>
> 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
>
>>>
>>>> + }
>>>> +}
>>>> +
