On 5/9/2025 5:03 PM, Baolu Lu wrote:
> On 4/7/2025 3:49 PM, Chenyi Qiang wrote:
>> With the introduction of the RamBlockAttribute object to manage
>> RAMBlocks with guest_memfd and the implementation of
>> PrivateSharedManager interface to convey page conversion events, it is
>> more elegant to move attribute changes into a PrivateSharedListener.
>>
>> The PrivateSharedListener is reigstered/unregistered for each memory
>> region section during kvm_region_add/del(), and listeners are stored in
>> a CVMPrivateSharedListener list for easy management. The listener
>> handler performs attribute changes upon receiving notifications from
>> private_shared_manager_state_change() calls. With this change, the
>> state changes operations in kvm_convert_memory() can be removed.
>>
>> Note that after moving attribute changes into a listener, errors can be
>> returned in ram_block_attribute_notify_to_private() if attribute changes
>> fail in corner cases (e.g. -ENOMEM). Since there is currently no rollback
>> operation for the to_private case, an assert is used to prevent the
>> guest from continuing with a partially changed attribute state.
>
> From the kernel IOMMU subsystem's perspective, this lack of rollback
> might not be a significant issue. Currently, converting memory pages
> from shared to private involves unpinning the pages and removing the
> mappings from the IOMMU page table, both of which are typically non-
> failing operations.
>
> But, in the future, when it comes to partial conversions, there might be
> a cut operation before the VFIO unmap. The kernel IOMMU subsystem cannot
> guarantee an always-successful cut operation.
Indeed. cut_mapping could fail and in-place conversion path would
change, which makes the error handling more complicated in the future.
At present, the basic convert memory handling does it in the simplest
way, i.e. QEMU quit if failed, which puzzled me a little bit: Should I
follow this simplest thought to just return without rollback, or keep
the rollback logic in case the future change requires it. Maybe I can
move the rollback handling in a individual patch for ease of management.
>
> Thanks,
> baolu
