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

[Zhao Liu <zhao1.liu@intel.com>]

> >> 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.

Thanks,
Zhao

> > 
> >> +    }
> >> +}
> >> +