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

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 5/26/2025 5:06 PM, David Hildenbrand wrote:
> 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?

I thought these checks can act as some placeholder and provide
notifications when someone is trying to implement live migration in
Coco-VM. (As this series brought some confusion when developing the
related live migration handling internally). It is perfectly OK to drop
them. Sorry for confusion.

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

Will do. Thanks!

>