Hi David & Alexey,
To keep the bitmap aligned, I add the undo operation for
set_memory_attributes() and use the bitmap + replay callback to do
set_memory_attributes(). Does this change make sense?
Alexey, I didn't add your Reivewed-by since this patch introduced some
new changes.
On 3/10/2025 4:18 PM, Chenyi Qiang wrote:
> Introduce a new field, memory_attribute_manager, in RAMBlock to link to
> an MemoryAttributeManager object. This change centralizes all
> guest_memfd state information (like fd and shared_bitmap) within a
> RAMBlock, making it easier to manage.
>
> Use the realize()/unrealize() helpers to initialize/uninitialize the
> MemoryAttributeManager object. Register/unregister the object in the
> target RAMBlock's MemoryRegion when creating guest_memfd.
>
> In the kvm_convert_memory() function, manage memory state changes by
> using the shared_bitmap to call set_attribute() only on the specific
> memory range. Additionally, use the
> memory_attribute_manager_state_change() helper to notify the reigstered
> RamDiscardListener of these changes.
>
> Signed-off-by: Chenyi Qiang <chenyi.qiang@xxxxxxxxx>
> ---
> Changes in v3:
> - Use ram_discard_manager_reply_populated/discarded() to set the
> memory attribute and add the undo support if state_change()
> failed.
> - Didn't add Reviewed-by from Alexey due to the new changes in this
> commit.
>
> Changes in v2:
> - Introduce a new field memory_attribute_manager in RAMBlock.
> - Move the state_change() handling during page conversion in this patch.
> - Undo what we did if it fails to set.
> - Change the order of close(guest_memfd) and memory_attribute_manager cleanup.
> ---
> accel/kvm/kvm-all.c | 50 +++++++++++++++++++++++++++++++++++++++--
> include/exec/ramblock.h | 2 ++
> system/physmem.c | 13 +++++++++++
> 3 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c1fea69d58..a89c5655e8 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -48,6 +48,7 @@
> #include "kvm-cpus.h"
> #include "system/dirtylimit.h"
> #include "qemu/range.h"
> +#include "system/memory-attribute-manager.h"
>
> #include "hw/boards.h"
> #include "system/stats.h"
> @@ -3018,6 +3019,25 @@ static void kvm_eat_signals(CPUState *cpu)
> } while (sigismember(&chkset, SIG_IPI));
> }
>
> +typedef struct SetMemoryAttribute {
> + bool to_private;
> +} SetMemoryAttribute;
> +
> +static int kvm_set_memory_attributes_cb(MemoryRegionSection *section,
> + void *opaque)
> +{
> + hwaddr start = section->offset_within_address_space;
> + hwaddr size = section->size;
> + SetMemoryAttribute *args = opaque;
> + bool to_private = args->to_private;
> +
> + if (to_private) {
> + return kvm_set_memory_attributes_private(start, size);
> + } else {
> + return kvm_set_memory_attributes_shared(start, size);
> + }
> +}
> +
> int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private)
> {
> MemoryRegionSection section;
> @@ -3026,6 +3046,7 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private)
> RAMBlock *rb;
> void *addr;
> int ret = -EINVAL;
> + SetMemoryAttribute args = { .to_private = to_private };
>
> trace_kvm_convert_memory(start, size, to_private ? "shared_to_private" : "private_to_shared");
>
> @@ -3077,9 +3098,13 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private)
> }
>
> if (to_private) {
> - ret = kvm_set_memory_attributes_private(start, size);
> + ret = ram_discard_manager_replay_populated(mr->rdm, §ion,
> + kvm_set_memory_attributes_cb,
> + &args);
> } else {
> - ret = kvm_set_memory_attributes_shared(start, size);
> + ret = ram_discard_manager_replay_discarded(mr->rdm, §ion,
> + kvm_set_memory_attributes_cb,
> + &args);
> }
> if (ret) {
> goto out_unref;
> @@ -3088,6 +3113,27 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private)
> addr = memory_region_get_ram_ptr(mr) + section.offset_within_region;
> rb = qemu_ram_block_from_host(addr, false, &offset);
>
> + ret = memory_attribute_manager_state_change(MEMORY_ATTRIBUTE_MANAGER(mr->rdm),
> + offset, size, to_private);
> + if (ret) {
> + warn_report("Failed to notify the listener the state change of "
> + "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s",
> + start, size, to_private ? "private" : "shared");
> + args.to_private = !to_private;
> + if (to_private) {
> + ret = ram_discard_manager_replay_populated(mr->rdm, §ion,
> + kvm_set_memory_attributes_cb,
> + &args);
> + } else {
> + ret = ram_discard_manager_replay_discarded(mr->rdm, §ion,
> + kvm_set_memory_attributes_cb,
> + &args);
> + }
> + if (ret) {
> + goto out_unref;
> + }
> + }
> +
> if (to_private) {
> if (rb->page_size != qemu_real_host_page_size()) {
> /*
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> index 0babd105c0..06fd365326 100644
> --- a/include/exec/ramblock.h
> +++ b/include/exec/ramblock.h
> @@ -23,6 +23,7 @@
> #include "cpu-common.h"
> #include "qemu/rcu.h"
> #include "exec/ramlist.h"
> +#include "system/memory-attribute-manager.h"
>
> struct RAMBlock {
> struct rcu_head rcu;
> @@ -42,6 +43,7 @@ struct RAMBlock {
> int fd;
> uint64_t fd_offset;
> int guest_memfd;
> + MemoryAttributeManager *memory_attribute_manager;
> size_t page_size;
> /* dirty bitmap used during migration */
> unsigned long *bmap;
> diff --git a/system/physmem.c b/system/physmem.c
> index c76503aea8..0ed394c5d2 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -54,6 +54,7 @@
> #include "system/hostmem.h"
> #include "system/hw_accel.h"
> #include "system/xen-mapcache.h"
> +#include "system/memory-attribute-manager.h"
> #include "trace.h"
>
> #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> @@ -1885,6 +1886,16 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> qemu_mutex_unlock_ramlist();
> goto out_free;
> }
> +
> + new_block->memory_attribute_manager = MEMORY_ATTRIBUTE_MANAGER(object_new(TYPE_MEMORY_ATTRIBUTE_MANAGER));
> + if (memory_attribute_manager_realize(new_block->memory_attribute_manager, new_block->mr)) {
> + error_setg(errp, "Failed to realize memory attribute manager");
> + object_unref(OBJECT(new_block->memory_attribute_manager));
> + close(new_block->guest_memfd);
> + ram_block_discard_require(false);
> + qemu_mutex_unlock_ramlist();
> + goto out_free;
> + }
> }
>
> ram_size = (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS;
> @@ -2138,6 +2149,8 @@ static void reclaim_ramblock(RAMBlock *block)
> }
>
> if (block->guest_memfd >= 0) {
> + memory_attribute_manager_unrealize(block->memory_attribute_manager);
> + object_unref(OBJECT(block->memory_attribute_manager));
> close(block->guest_memfd);
> ram_block_discard_require(false);
> }
