Re: [RFC PATCH v2 10/51] KVM: selftests: Refactor vm_mem_add to be more flexible

[Sean Christopherson <seanjc@google.com>]

On Wed, May 14, 2025, Ackerley Tng wrote:
> enum vm_mem_backing_src_type is encoding too many different
> possibilities on different axes of (1) whether to mmap from an fd, (2)
> granularity of mapping for THP, (3) size of hugetlb mapping, and has
> yet to be extended to support guest_memfd.

I don't disagree (I added the FIXME after all), but IMO what you've done here
doesn't improve the situation, it just punts the mess to tests, i.e. lets the
guest_memfd_conversions test do its thing, but doesn't actually improve anything.

A test has no business dealing with details about "installing" memory or even
mapping the memslot.  The whole point of the core library is to deal with those
details so they don't need to be copy+pasted.  Adding a gmem-backed memslot
shouldn't require this much effort:

	region = vm_mem_region_alloc(vm);

	guest_memfd = vm_mem_region_install_guest_memfd(region, guest_memfd);
	mem = vm_mem_region_mmap(region, memslot_size, MAP_SHARED, guest_memfd, 0);
	vm_mem_region_install_memory(region, memslot_size, PAGE_SIZE);

	region->region.slot = GUEST_MEMFD_SHARING_TEST_SLOT;
	region->region.flags = KVM_MEM_GUEST_MEMFD;
	region->region.guest_phys_addr = GUEST_MEMFD_SHARING_TEST_GPA;
	region->region.guest_memfd_offset = 0;

	vm_mem_region_add(vm, region);

Again, I agree vm_mem_add() is a mess.  But overall, this is a big step backwards.

> ---
>  .../testing/selftests/kvm/include/kvm_util.h  |  29 +-
>  .../testing/selftests/kvm/include/test_util.h |   2 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 429 +++++++++++-------
>  tools/testing/selftests/kvm/lib/test_util.c   |  25 +
>  4 files changed, 328 insertions(+), 157 deletions(-)

It's also not reviewable.  There is far, far too much going on here.  This is
quite obviously not one logical change.  The changelog is appropriate for the
cover letter of a decently large series, not for a patch that touches core code
of the selftests.

The massive refactoring is also unnecessary, at least for conversions.  With some
prep renames to shorten line lengths, all that is needed at this time is to dup()
the gmem_fd into region->fd when the gmem_fd can be mmap()'d.  It's not just a
clever hack, it's also how we want gmem to be used going forward; and for non-CoCo
VMs, it's the _only_ sane way to use gmem.

The only scenario where a test would want to shove a different userspace address
into the memslot is a test that deliberately tests that exact scenario.  And that
sort of thing belongs in a test; core code should only concern itself with the
scenario that 99 of usage will want.

With this, the test can do:

	vm_mem_add(vm, VM_MEM_SRC_SHMEM, gpa, slot, nr_pages,
		   KVM_MEM_GUEST_MEMFD, -1, 0, gmem_flags);

	t->gmem_fd = kvm_slot_to_fd(vm, slot);
	t->mem = addr_gpa2hva(vm, gpa);
	virt_map(vm, GUEST_MEMFD_SHARING_TEST_GVA, gpa, nr_pages);

---
 tools/testing/selftests/kvm/include/kvm_util.h           | 7 ++++++-
 tools/testing/selftests/kvm/lib/kvm_util.c               | 9 +++++----
 .../selftests/kvm/x86/private_mem_conversions_test.c     | 2 +-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 45159638d5dd..de8ae9be1906 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -678,7 +678,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
 				 uint32_t flags);
 void vm_mem_add(struct kvm_vm *vm, enum vm_mem_backing_src_type src_type,
 		uint64_t gpa, uint32_t slot, uint64_t npages, uint32_t flags,
-		int guest_memfd_fd, uint64_t guest_memfd_offset);
+		int gmem_fd, uint64_t gmem_offset, uint64_t gmem_flags);
 
 #ifndef vm_arch_has_protected_memory
 static inline bool vm_arch_has_protected_memory(struct kvm_vm *vm)
@@ -711,6 +711,11 @@ void *addr_gva2hva(struct kvm_vm *vm, vm_vaddr_t gva);
 vm_paddr_t addr_hva2gpa(struct kvm_vm *vm, void *hva);
 void *addr_gpa2alias(struct kvm_vm *vm, vm_paddr_t gpa);
 
+static inline int kvm_slot_to_fd(struct kvm_vm *vm, uint32_t slot)
+{
+	return memslot2region(vm, slot)->fd;
+}
+
 #ifndef vcpu_arch_put_guest
 #define vcpu_arch_put_guest(mem, val) do { (mem) = (val); } while (0)
 #endif
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 885a0dd58626..196af025e833 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -944,7 +944,7 @@ void vm_set_user_memory_region2(struct kvm_vm *vm, uint32_t slot, uint32_t flags
 /* FIXME: This thing needs to be ripped apart and rewritten. */
 void vm_mem_add(struct kvm_vm *vm, enum vm_mem_backing_src_type src_type,
 		uint64_t gpa, uint32_t slot, uint64_t npages, uint32_t flags,
-		int gmem_fd, uint64_t gmem_offset)
+		int gmem_fd, uint64_t gmem_offset, uint64_t gmem_flags)
 {
 	int ret;
 	struct userspace_mem_region *region;
@@ -1028,7 +1028,6 @@ void vm_mem_add(struct kvm_vm *vm, enum vm_mem_backing_src_type src_type,
 
 	if (flags & KVM_MEM_GUEST_MEMFD) {
 		if (gmem_fd < 0) {
-			uint32_t gmem_flags = 0;
 			TEST_ASSERT(!gmem_offset,
 				    "Offset must be zero when creating new guest_memfd");
 			gmem_fd = vm_create_guest_memfd(vm, mem_size, gmem_flags);
@@ -1049,7 +1048,9 @@ void vm_mem_add(struct kvm_vm *vm, enum vm_mem_backing_src_type src_type,
 	}
 
 	region->fd = -1;
-	if (backing_src_is_shared(src_type))
+	if (flags & KVM_MEM_GUEST_MEMFD && gmem_flags & GUEST_MEMFD_FLAG_MMAP)
+		region->fd = kvm_dup(gmem_fd);
+	else if (backing_src_is_shared(src_type))
 		region->fd = kvm_memfd_alloc(region->mmap_size,
 					     src_type == VM_MEM_SRC_SHARED_HUGETLB);
 
@@ -1116,7 +1117,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
 				 uint64_t gpa, uint32_t slot, uint64_t npages,
 				 uint32_t flags)
 {
-	vm_mem_add(vm, src_type, gpa, slot, npages, flags, -1, 0);
+	vm_mem_add(vm, src_type, gpa, slot, npages, flags, -1, 0, 0);
 }
 
 /*
diff --git a/tools/testing/selftests/kvm/x86/private_mem_conversions_test.c b/tools/testing/selftests/kvm/x86/private_mem_conversions_test.c
index 1969f4ab9b28..41f6b38f0407 100644
--- a/tools/testing/selftests/kvm/x86/private_mem_conversions_test.c
+++ b/tools/testing/selftests/kvm/x86/private_mem_conversions_test.c
@@ -399,7 +399,7 @@ static void test_mem_conversions(enum vm_mem_backing_src_type src_type, uint32_t
 	for (i = 0; i < nr_memslots; i++)
 		vm_mem_add(vm, src_type, BASE_DATA_GPA + slot_size * i,
 			   BASE_DATA_SLOT + i, slot_size / vm->page_size,
-			   KVM_MEM_GUEST_MEMFD, memfd, slot_size * i);
+			   KVM_MEM_GUEST_MEMFD, memfd, slot_size * i, 0);
 
 	for (i = 0; i < nr_vcpus; i++) {
 		uint64_t gpa =  BASE_DATA_GPA + i * per_cpu_size;

base-commit: 3572aeafd53394f4bf0a61d79c5c1b8cfccc3860
--