Re: [PATCH v5 06/10] memory: Attach RamBlockAttribute to guest_memfd-backed RAMBlocks

[David Hildenbrand <david@redhat.com>]

On 20.05.25 12:28, Chenyi Qiang wrote:
> A new field, ram_shared, was introduced in RAMBlock to link to a
> RamBlockAttribute object, which centralizes all guest_memfd state
> information (such as fd and shared_bitmap) within a RAMBlock.
> 
> Create and initialize the RamBlockAttribute object upon ram_block_add().
> Meanwhile, register the object in the target RAMBlock's MemoryRegion.
> After that, guest_memfd-backed RAMBlock is associated with the
> RamDiscardManager interface, and the users will execute
> RamDiscardManager specific handling. For example, VFIO will register the
> RamDiscardListener as expected. The live migration path needs to be
> avoided since it is not supported yet in confidential VMs.
> 
> Additionally, use the ram_block_attribute_state_change() helper to
> notify the registered RamDiscardListener of these changes.
> 
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
> Changes in v5:
>      - Revert to use RamDiscardManager interface.
>      - Move the object_new() into the ram_block_attribute_create()
>        helper.
>      - Add some check in migration path.
> 
> Changes in v4:
>      - Remove the replay operations for attribute changes which will be
>        handled in a listener in following patches.
>      - Add some comment in the error path of realize() to remind the
>        future development of the unified error path.
> 
> 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 |  9 +++++++++
>   migration/ram.c     | 28 ++++++++++++++++++++++++++++
>   system/physmem.c    | 14 ++++++++++++++
>   3 files changed, 51 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 51526d301b..2d7ecaeb6a 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3089,6 +3089,15 @@ 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 = ram_block_attribute_state_change(RAM_BLOCK_ATTRIBUTE(mr->rdm),
> +                                           offset, size, to_private);
> +    if (ret) {
> +        error_report("Failed to notify the listener the state change of "
> +                     "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s",
> +                     start, size, to_private ? "private" : "shared");
> +        goto out_unref;
> +    }
> +
>       if (to_private) {
>           if (rb->page_size != qemu_real_host_page_size()) {
>               /*
> diff --git a/migration/ram.c b/migration/ram.c
> index c004f37060..69c9a42f16 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -890,6 +890,13 @@ static uint64_t ramblock_dirty_bitmap_clear_discarded_pages(RAMBlock *rb)
>   
>       if (rb->mr && rb->bmap && memory_region_has_ram_discard_manager(rb->mr)) {
>           RamDiscardManager *rdm = memory_region_get_ram_discard_manager(rb->mr);
> +
> +        if (object_dynamic_cast(OBJECT(rdm), TYPE_RAM_BLOCK_ATTRIBUTE)) {
> +            error_report("%s: Live migration for confidential VM is not "
> +                         "supported yet.", __func__);
> +            exit(1);
> +        }
>

These checks seem conceptually wrong.

I think if we were to special-case specific implementations, we should 
do so using a different callback.

But why should we bother at all checking basic live migration handling 
("unsupported for confidential VMs") on this level, and even just 
exit()'ing the process?

All these object_dynamic_cast() checks in this patch should be dropped.

-- 
Cheers,

David / dhildenb