Re: [PATCH v4 07/13] ram-block-attribute: Introduce RamBlockAttribute to manage RAMBLock with guest_memfd

[Chenyi Qiang <chenyi.qiang@intel.com>]

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@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-31653064f27f@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
> 
>>>
>>>> +    }
>>>> +}
>>>> +