Re: [PATCH 2/7] guest_memfd: Introduce an object to manage the guest-memfd with RamDiscardManager

Alexey Kardashevskiy
2025-01-08T04:48:13Z

On 13/12/24 18:08, Chenyi Qiang wrote:
> As the commit 852f0048f3 ("RAMBlock: make guest_memfd require
> uncoordinated discard") highlighted, some subsystems like VFIO might
> disable ram block discard. However, guest_memfd relies on the discard
> operation to perform page conversion between private and shared memory.
> This can lead to stale IOMMU mapping issue when assigning a hardware
> device to a confidential VM via shared memory (unprotected memory
> pages). Blocking shared page discard can solve this problem, but it
> could cause guests to consume twice the memory with VFIO, which is not
> acceptable in some cases. An alternative solution is to convey other
> systems like VFIO to refresh its outdated IOMMU mappings.
>
> RamDiscardManager is an existing concept (used by virtio-mem) to adjust
> VFIO mappings in relation to VM page assignment. Effectively page
> conversion is similar to hot-removing a page in one mode and adding it
> back in the other, so the similar work that needs to happen in response
> to virtio-mem changes needs to happen for page conversion events.
> Introduce the RamDiscardManager to guest_memfd to achieve it.
>
> However, guest_memfd is not an object so it cannot directly implement
> the RamDiscardManager interface.
>
> One solution is to implement the interface in HostMemoryBackend. Any

This sounds about right.

> guest_memfd-backed host memory backend can register itself in the target
> MemoryRegion. However, this solution doesn't cover the scenario where a
> guest_memfd MemoryRegion doesn't belong to the HostMemoryBackend, e.g.
> the virtual BIOS MemoryRegion.

What is this virtual BIOS MemoryRegion exactly? What does it look like
in "info mtree -f"? Do we really want this memory to be DMAable?

> Thus, choose the second option, i.e. define an object type named
> guest_memfd_manager with RamDiscardManager interface. Upon creation of
> guest_memfd, a new guest_memfd_manager object can be instantiated and
> registered to the managed guest_memfd MemoryRegion to handle the page
> conversion events.
>
> In the context of guest_memfd, the discarded state signifies that the
> page is private, while the populated state indicated that the page is
> shared. The state of the memory is tracked at the granularity of the
> host page size (i.e. block_size), as the minimum conversion size can be
> one page per request.
>
> In addition, VFIO expects the DMA mapping for a specific iova to be
> mapped and unmapped with the same granularity. However, the confidential
> VMs may do partial conversion, e.g. conversion happens on a small region
> within a large region. To prevent such invalid cases and before any
> potential optimization comes out, all operations are performed with 4K
> granularity.
>
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
> include/sysemu/guest-memfd-manager.h | 46 +++++
> system/guest-memfd-manager.c | 250 +++++++++++++++++++++++++++
> system/meson.build | 1 +
> 3 files changed, 297 insertions(+)
> create mode 100644 include/sysemu/guest-memfd-manager.h
> create mode 100644 system/guest-memfd-manager.c
>
> diff --git a/include/sysemu/guest-memfd-manager.h b/include/sysemu/guest-memfd-manager.h
> new file mode 100644
> index 0000000000..ba4a99b614
> --- /dev/null
> +++ b/include/sysemu/guest-memfd-manager.h
> @@ -0,0 +1,46 @@
> +/*
> + * QEMU guest memfd manager
> + *
> + * Copyright Intel
> + *
> + * Author:
> + * Chenyi Qiang <chenyi.qiang@intel.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory
> + *
> + */
> +
> +#ifndef SYSEMU_GUEST_MEMFD_MANAGER_H
> +#define SYSEMU_GUEST_MEMFD_MANAGER_H
> +
> +#include "sysemu/hostmem.h"
> +
> +#define TYPE_GUEST_MEMFD_MANAGER "guest-memfd-manager"
> +
> +OBJECT_DECLARE_TYPE(GuestMemfdManager, GuestMemfdManagerClass, GUEST_MEMFD_MANAGER)
> +
> +struct GuestMemfdManager {
> + Object parent;
> +
> + /* Managed memory region. */

Do not need this comment. And the period.

> + MemoryRegion *mr;
> +
> + /*
> + * 1-setting of the bit represents the memory is populated (shared).
> + */

Could be 1 line comment.

> + int32_t bitmap_size;

int or unsigned

> + unsigned long *bitmap;
> +
> + /* block size and alignment */
> + uint64_t block_size;

unsigned?

(u)int(32|64)_t make sense for migrations which is not the case (yet?).
Thanks,

> +
> + /* listeners to notify on populate/discard activity. */

Do not really need this comment either imho.

> + QLIST_HEAD(, RamDiscardListener) rdl_list;
> +};
> +
> +struct GuestMemfdManagerClass {
> + ObjectClass parent_class;
> +};
> +
> +#endif
> diff --git a/system/guest-memfd-manager.c b/system/guest-memfd-manager.c
> new file mode 100644
> index 0000000000..d7e105fead
> --- /dev/null
> +++ b/system/guest-memfd-manager.c
> @@ -0,0 +1,250 @@
> +/*
> + * QEMU guest memfd manager
> + *
> + * Copyright Intel
> + *
> + * Author:
> + * Chenyi Qiang <chenyi.qiang@intel.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/guest-memfd-manager.h"
> +
> +OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(GuestMemfdManager,
> + guest_memfd_manager,
> + GUEST_MEMFD_MANAGER,
> + OBJECT,
> + { TYPE_RAM_DISCARD_MANAGER },
> + { })
> +
> +static bool guest_memfd_rdm_is_populated(const RamDiscardManager *rdm,
> + const MemoryRegionSection *section)
> +{
> + const GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(rdm);
> + uint64_t first_bit = section->offset_within_region / gmm->block_size;
> + uint64_t last_bit = first_bit + int128_get64(section->size) / gmm->block_size - 1;
> + unsigned long first_discard_bit;
> +
> + first_discard_bit = find_next_zero_bit(gmm->bitmap, last_bit + 1, first_bit);
> + return first_discard_bit > last_bit;
> +}
> +
> +typedef int (*guest_memfd_section_cb)(MemoryRegionSection *s, void *arg);
> +
> +static int guest_memfd_notify_populate_cb(MemoryRegionSection *section, void *arg)
> +{
> + RamDiscardListener *rdl = arg;
> +
> + return rdl->notify_populate(rdl, section);
> +}
> +
> +static int guest_memfd_notify_discard_cb(MemoryRegionSection *section, void *arg)
> +{
> + RamDiscardListener *rdl = arg;
> +
> + rdl->notify_discard(rdl, section);
> +
> + return 0;
> +}
> +
> +static int guest_memfd_for_each_populated_section(const GuestMemfdManager *gmm,
> + MemoryRegionSection *section,
> + void *arg,
> + guest_memfd_section_cb cb)
> +{
> + unsigned long first_one_bit, last_one_bit;
> + uint64_t offset, size;
> + int ret = 0;
> +
> + first_one_bit = section->offset_within_region / gmm->block_size;
> + first_one_bit = find_next_bit(gmm->bitmap, gmm->bitmap_size, first_one_bit);
> +
> + while (first_one_bit < gmm->bitmap_size) {
> + MemoryRegionSection tmp = *section;
> +
> + offset = first_one_bit * gmm->block_size;
> + last_one_bit = find_next_zero_bit(gmm->bitmap, gmm->bitmap_size,
> + first_one_bit + 1) - 1;
> + size = (last_one_bit - first_one_bit + 1) * gmm->block_size;

This tries calling cb() on bigger chunks even though we say from the
beginning that only page size is supported?

May be simplify this for now and extend if/when VFIO learns to split
mappings, or just drop it when we get in-place page state convertion
(which will make this all irrelevant)?

> +
> + if (!memory_region_section_intersect_range(&tmp, offset, size)) {
> + break;
> + }
> +
> + ret = cb(&tmp, arg);
> + if (ret) {
> + break;
> + }
> +
> + first_one_bit = find_next_bit(gmm->bitmap, gmm->bitmap_size,
> + last_one_bit + 2);
> + }
> +
> + return ret;
> +}
> +
> +static int guest_memfd_for_each_discarded_section(const GuestMemfdManager *gmm,
> + MemoryRegionSection *section,
> + void *arg,
> + guest_memfd_section_cb cb)
> +{
> + unsigned long first_zero_bit, last_zero_bit;
> + uint64_t offset, size;
> + int ret = 0;
> +
> + first_zero_bit = section->offset_within_region / gmm->block_size;
> + first_zero_bit = find_next_zero_bit(gmm->bitmap, gmm->bitmap_size,
> + first_zero_bit);
> +
> + while (first_zero_bit < gmm->bitmap_size) {
> + MemoryRegionSection tmp = *section;
> +
> + offset = first_zero_bit * gmm->block_size;
> + last_zero_bit = find_next_bit(gmm->bitmap, gmm->bitmap_size,
> + first_zero_bit + 1) - 1;
> + size = (last_zero_bit - first_zero_bit + 1) * gmm->block_size;
> +
> + if (!memory_region_section_intersect_range(&tmp, offset, size)) {
> + break;
> + }
> +
> + ret = cb(&tmp, arg);
> + if (ret) {
> + break;
> + }
> +
> + first_zero_bit = find_next_zero_bit(gmm->bitmap, gmm->bitmap_size,
> + last_zero_bit + 2);
> + }
> +
> + return ret;
> +}
> +
> +static uint64_t guest_memfd_rdm_get_min_granularity(const RamDiscardManager *rdm,
> + const MemoryRegion *mr)
> +{
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(rdm);
> +
> + g_assert(mr == gmm->mr);
> + return gmm->block_size;
> +}
> +
> +static void guest_memfd_rdm_register_listener(RamDiscardManager *rdm,
> + RamDiscardListener *rdl,
> + MemoryRegionSection *section)
> +{
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(rdm);
> + int ret;
> +
> + g_assert(section->mr == gmm->mr);
> + rdl->section = memory_region_section_new_copy(section);
> +
> + QLIST_INSERT_HEAD(&gmm->rdl_list, rdl, next);
> +
> + ret = guest_memfd_for_each_populated_section(gmm, section, rdl,
> + guest_memfd_notify_populate_cb);
> + if (ret) {
> + error_report("%s: Failed to register RAM discard listener: %s", __func__,
> + strerror(-ret));
> + }
> +}
> +
> +static void guest_memfd_rdm_unregister_listener(RamDiscardManager *rdm,
> + RamDiscardListener *rdl)
> +{
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(rdm);
> + int ret;
> +
> + g_assert(rdl->section);
> + g_assert(rdl->section->mr == gmm->mr);
> +
> + ret = guest_memfd_for_each_populated_section(gmm, rdl->section, rdl,
> + guest_memfd_notify_discard_cb);
> + if (ret) {
> + error_report("%s: Failed to unregister RAM discard listener: %s", __func__,
> + strerror(-ret));
> + }
> +
> + memory_region_section_free_copy(rdl->section);
> + rdl->section = NULL;
> + QLIST_REMOVE(rdl, next);
> +
> +}
> +
> +typedef struct GuestMemfdReplayData {
> + void *fn;

s/void */ReplayRamPopulate/

> + void *opaque;
> +} GuestMemfdReplayData;
> +
> +static int guest_memfd_rdm_replay_populated_cb(MemoryRegionSection *section, void *arg)
> +{
> + struct GuestMemfdReplayData *data = arg;

Drop "struct" here and below.

> + ReplayRamPopulate replay_fn = data->fn;
> +
> + return replay_fn(section, data->opaque);
> +}
> +
> +static int guest_memfd_rdm_replay_populated(const RamDiscardManager *rdm,
> + MemoryRegionSection *section,
> + ReplayRamPopulate replay_fn,
> + void *opaque)
> +{
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(rdm);
> + struct GuestMemfdReplayData data = { .fn = replay_fn, .opaque = opaque };
> +
> + g_assert(section->mr == gmm->mr);
> + return guest_memfd_for_each_populated_section(gmm, section, &data,
> + guest_memfd_rdm_replay_populated_cb);
> +}
> +
> +static int guest_memfd_rdm_replay_discarded_cb(MemoryRegionSection *section, void *arg)
> +{
> + struct GuestMemfdReplayData *data = arg;
> + ReplayRamDiscard replay_fn = data->fn;
> +
> + replay_fn(section, data->opaque);

guest_memfd_rdm_replay_populated_cb() checks for errors though.

> +
> + return 0;
> +}
> +
> +static void guest_memfd_rdm_replay_discarded(const RamDiscardManager *rdm,
> + MemoryRegionSection *section,
> + ReplayRamDiscard replay_fn,
> + void *opaque)
> +{
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(rdm);
> + struct GuestMemfdReplayData data = { .fn = replay_fn, .opaque = opaque };
> +
> + g_assert(section->mr == gmm->mr);
> + guest_memfd_for_each_discarded_section(gmm, section, &data,
> + guest_memfd_rdm_replay_discarded_cb);
> +}
> +
> +static void guest_memfd_manager_init(Object *obj)
> +{
> + GuestMemfdManager *gmm = GUEST_MEMFD_MANAGER(obj);
> +
> + QLIST_INIT(&gmm->rdl_list);
> +}
> +
> +static void guest_memfd_manager_finalize(Object *obj)
> +{
> + g_free(GUEST_MEMFD_MANAGER(obj)->bitmap);

bitmap is not allocated though. And 5/7 removes this anyway. Thanks,

> +}
> +
> +static void guest_memfd_manager_class_init(ObjectClass *oc, void *data)
> +{
> + RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(oc);
> +
> + rdmc->get_min_granularity = guest_memfd_rdm_get_min_granularity;
> + rdmc->register_listener = guest_memfd_rdm_register_listener;
> + rdmc->unregister_listener = guest_memfd_rdm_unregister_listener;
> + rdmc->is_populated = guest_memfd_rdm_is_populated;
> + rdmc->replay_populated = guest_memfd_rdm_replay_populated;
> + rdmc->replay_discarded = guest_memfd_rdm_replay_discarded;
> +}
> diff --git a/system/meson.build b/system/meson.build
> index 4952f4b2c7..ed4e1137bd 100644
> --- a/system/meson.build
> +++ b/system/meson.build
> @@ -15,6 +15,7 @@ system_ss.add(files(
> 'dirtylimit.c',
> 'dma-helpers.c',
> 'globals.c',
> + 'guest-memfd-manager.c',
> 'memory_mapping.c',
> 'qdev-monitor.c',
> 'qtest.c',

--
Alexey