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

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

[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

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

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

>    '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?

Additionally, maybe we can add a simple helper to get page size from
RamBlockAttribute.

> +}
> +

[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?

> +    }
> +}
> +
> +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);
> +}
> +