Re: [POC PATCH 3/5] memory/guest_memfd: Enable in-place conversion when available

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 7/17/2025 10:02 AM, Chenyi Qiang wrote:
> 
> 
> On 7/15/2025 11:31 AM, Xiaoyao Li wrote:
>> From: Yan Zhao <yan.y.zhao@intel.com>
>>
>> (This is just the POC code to use in-place conversion gmem.)
>>
>> Try to use in-place conversion gmem when it is supported.
>>
>> When in-place conversion is enabled, there is no need to discard memory
>> since it still needs to be used as the memory of opposite attribute
>> after conversion.
>>
>> For a upstreamable solution, we can introduce memory-backend-guestmemfd
>> for in-place conversion. With the non in-place conversion, it needs
>> seperate non-gmem memory to back the shared memory and gmem is created
>> implicitly and internally based on vm type. While with in-place
>> conversion, there is no need for seperate non-gmem memory because gmem
>> itself can be served as shared memory. So that we can introduce
>> memory-backend-guestmemfd as the specific backend for in-place
>> conversion gmem.
>>
>> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
>> Co-developed-by Xiaoyao Li <xiaoyao.li@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   accel/kvm/kvm-all.c       | 79 ++++++++++++++++++++++++++++-----------
>>   accel/stubs/kvm-stub.c    |  1 +
>>   include/system/kvm.h      |  1 +
>>   include/system/memory.h   |  2 +
>>   include/system/ramblock.h |  1 +
>>   system/memory.c           |  7 ++++
>>   system/physmem.c          | 21 ++++++++++-
>>   7 files changed, 90 insertions(+), 22 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index a106d1ba0f0b..609537738d38 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -105,6 +105,7 @@ static int kvm_sstep_flags;
>>   static bool kvm_immediate_exit;
>>   static uint64_t kvm_supported_memory_attributes;
>>   static bool kvm_guest_memfd_supported;
>> +bool kvm_guest_memfd_inplace_supported;
>>   static hwaddr kvm_max_slot_size = ~0;
>>   
>>   static const KVMCapabilityInfo kvm_required_capabilites[] = {
>> @@ -1487,6 +1488,30 @@ static int kvm_set_memory_attributes(hwaddr start, uint64_t size, uint64_t attr)
>>       return r;
>>   }
>>   
>> +static int kvm_set_guest_memfd_shareability(MemoryRegion *mr, ram_addr_t offset,
>> +                                            uint64_t size, bool shared)
>> +{
>> +    int guest_memfd = mr->ram_block->guest_memfd;
>> +    struct kvm_gmem_convert param = {
>> +                .offset = offset,
>> +                .size = size,
>> +                .error_offset = 0,
>> +    };
>> +    unsigned long op;
>> +    int r;
>> +
>> +    op = shared ? KVM_GMEM_CONVERT_SHARED : KVM_GMEM_CONVERT_PRIVATE;
>> +
>> +    r = ioctl(guest_memfd, op, &param);
>> +    if (r) {
>> +        error_report("failed to set guest_memfd offset 0x%lx size 0x%lx to %s  "
>> +                     "error '%s' error offset 0x%llx",
>> +                     offset, size, shared ? "shared" : "private",
>> +                     strerror(errno), param.error_offset);
>> +    }
>> +    return r;
>> +}
>> +
>>   int kvm_set_memory_attributes_private(hwaddr start, uint64_t size)
>>   {
>>       return kvm_set_memory_attributes(start, size, KVM_MEMORY_ATTRIBUTE_PRIVATE);
>> @@ -1604,7 +1629,8 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
>>               abort();
>>           }
>>   
>> -        if (memory_region_has_guest_memfd(mr)) {
>> +        if (memory_region_has_guest_memfd(mr) &&
>> +            !memory_region_guest_memfd_in_place_conversion(mr)) {
>>               err = kvm_set_memory_attributes_private(start_addr, slot_size);
>>               if (err) {
>>                   error_report("%s: failed to set memory attribute private: %s",
>> @@ -2779,6 +2805,9 @@ static int kvm_init(AccelState *as, MachineState *ms)
>>           kvm_check_extension(s, KVM_CAP_GUEST_MEMFD) &&
>>           kvm_check_extension(s, KVM_CAP_USER_MEMORY2) &&
>>           (kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE);
>> +    kvm_guest_memfd_inplace_supported =
>> +        kvm_check_extension(s, KVM_CAP_GMEM_SHARED_MEM) &&
>> +        kvm_check_extension(s, KVM_CAP_GMEM_CONVERSION);
>>       kvm_pre_fault_memory_supported = kvm_vm_check_extension(s, KVM_CAP_PRE_FAULT_MEMORY);
>>   
>>       if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) {
>> @@ -3056,6 +3085,7 @@ static void kvm_eat_signals(CPUState *cpu)
>>   
>>   int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private)
>>   {
>> +    bool in_place_conversion = false;
>>       MemoryRegionSection section;
>>       ram_addr_t offset;
>>       MemoryRegion *mr;
>> @@ -3112,18 +3142,23 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private)
>>           goto out_unref;
>>       }
>>   
>> -    if (to_private) {
>> -        ret = kvm_set_memory_attributes_private(start, size);
>> -    } else {
>> -        ret = kvm_set_memory_attributes_shared(start, size);
>> -    }
>> -    if (ret) {
>> -        goto out_unref;
>> -    }
>> -
>>       addr = memory_region_get_ram_ptr(mr) + section.offset_within_region;
>>       rb = qemu_ram_block_from_host(addr, false, &offset);
>>   
>> +    in_place_conversion = memory_region_guest_memfd_in_place_conversion(mr);
>> +    if (in_place_conversion) {
>> +        ret = kvm_set_guest_memfd_shareability(mr, offset, size, !to_private);
>> +    } else {
>> +        if (to_private) {
>> +            ret = kvm_set_memory_attributes_private(start, size);
>> +        } else {
>> +            ret = kvm_set_memory_attributes_shared(start, size);
>> +        }
>> +    }
>> +    if (ret) {
>> +        goto out_unref;
>> +    }
>> +
>>       ret = ram_block_attributes_state_change(RAM_BLOCK_ATTRIBUTES(mr->rdm),
>>                                               offset, size, to_private);
>>       if (ret) {
> 
> There's one thing required for shared device assignment with in-place conversion, we need to follow the
> sequence of unmap-before-conversion-to-private and map-after-conversion-to-shared. Maybe change it like:
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index a54e68e769..e9e62ae8f2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3146,6 +3146,17 @@ 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);
>   
> +    if (to_private) {
> +        ret = ram_block_attributes_state_change(RAM_BLOCK_ATTRIBUTES(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;
> +        }
> +    }
> +
>       in_place_conversion = memory_region_guest_memfd_in_place_conversion(mr);
>       if (in_place_conversion) {
>           ret = kvm_set_guest_memfd_shareability(mr, offset, size, !to_private);
> @@ -3160,13 +3171,15 @@ int kvm_convert_memory(hwaddr start, hwaddr size, bool to_private)
>           goto out_unref;
>       }
>   
> -    ret = ram_block_attributes_state_change(RAM_BLOCK_ATTRIBUTES(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) {
> +        ret = ram_block_attributes_state_change(RAM_BLOCK_ATTRIBUTES(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;
> +        }
>       }

(Sorry for forgetting to reply in the community)

Thanks for catching and reporting it. I have incorporated it to the 
internal branch.