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.qi...@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!
>
