Many selftests use GUEST_DONE() to signal the end of guest code, which
is handled in userspace. In most tests, the test exits and there is no
need to clean up the ucall_pool->in_use bitmap.

If there are many guest code functions using GUEST_DONE(), or of guest
code functions are run many times, the ucall_pool->in_use bitmap will
fill up, causing later runs of the same guest code function to fail.

This patch allows ucall_free() to be called from userspace on uc.hva,
which will unset and free the correct struct ucall in the pool,
allowing ucalls to continue being used.

Change-Id: I2cb2aeed4b291b1bfb2bece001d09c509cd10446
Signed-off-by: Ackerley Tng <ackerleytng@google.com>
---
 .../testing/selftests/kvm/include/ucall_common.h |  1 +
 tools/testing/selftests/kvm/lib/ucall_common.c   | 16 ++++++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
index d9d6581b8d4f..b6b850d0319a 100644
--- a/tools/testing/selftests/kvm/include/ucall_common.h
+++ b/tools/testing/selftests/kvm/include/ucall_common.h
@@ -40,6 +40,7 @@ __printf(5, 6) void ucall_assert(uint64_t cmd, const char *exp,
 				 const char *fmt, ...);
 uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
 void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
+void ucall_free(struct ucall *uc);
 int ucall_nr_pages_required(uint64_t page_size);
 
 /*
diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index 42151e571953..9b6865c39ea7 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -21,24 +21,24 @@ int ucall_nr_pages_required(uint64_t page_size)
 
 /*
  * ucall_pool holds per-VM values (global data is duplicated by each VM), it
- * must not be accessed from host code.
+ * should generally not be accessed from host code other than via ucall_free(),
+ * to cleanup after using GUEST_DONE()
  */
 static struct ucall_header *ucall_pool;
 
 void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa)
 {
-	struct ucall_header *hdr;
 	struct ucall *uc;
 	vm_vaddr_t vaddr;
 	int i;
 
-	vaddr = vm_vaddr_alloc_shared(vm, sizeof(*hdr), KVM_UTIL_MIN_VADDR,
-				      MEM_REGION_DATA);
-	hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr);
-	memset(hdr, 0, sizeof(*hdr));
+	vaddr = vm_vaddr_alloc_shared(vm, sizeof(*ucall_pool),
+				      KVM_UTIL_MIN_VADDR, MEM_REGION_DATA);
+	ucall_pool = (struct ucall_header *)addr_gva2hva(vm, vaddr);
+	memset(ucall_pool, 0, sizeof(*ucall_pool));
 
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
-		uc = &hdr->ucalls[i];
+		uc = &ucall_pool->ucalls[i];
 		uc->hva = uc;
 	}
 
@@ -73,7 +73,7 @@ static struct ucall *ucall_alloc(void)
 	return NULL;
 }
 
-static void ucall_free(struct ucall *uc)
+void ucall_free(struct ucall *uc)
 {
 	/* Beware, here be pointer arithmetic.  */
 	clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
-- 
2.49.0.1045.g170613ef41-goog

On Wed, May 14, 2025, Ackerley Tng wrote:
> Many selftests use GUEST_DONE() to signal the end of guest code, which
> is handled in userspace. In most tests, the test exits and there is no
> need to clean up the ucall_pool->in_use bitmap.
> 
> If there are many guest code functions using GUEST_DONE(), or of guest
> code functions are run many times, the ucall_pool->in_use bitmap will
> fill up, causing later runs of the same guest code function to fail.
> 
> This patch allows ucall_free() to be called from userspace on uc.hva,
> which will unset and free the correct struct ucall in the pool,
> allowing ucalls to continue being used.

NAK.

The ucall thing isn't an issue with GUEST_DONE(), it's a general issue with not
completing a ucall.  The simple answer here is to not abuse GUEST_xxx().

I tried doing the same thing (jumping back to a guest's entry point) in what is
now the mmu_stress_test, and it didn't end well.  Restoring just registers mostly
works on x86, but it's not foolproof even there.  And on other architectures, the
approach is even less viable (IIRC).  E.g. if the guest code touches *anything*
that's not saved/restore, the test is hosed.

In short, while clever, the approach just doesn't work. Which is why I don't want
ucall_free() to exist: it's only useful for a pattern that is deeply flawed.

The easiest alternative is to use GUEST_SYNC(), have the guest code loop, and
use global variables to pass data.  It's ugly, but it works and is much less likely
to have arch specific quirks.   The worst of the ugliness can be mitigated by
using a struct to pass info, e.g. so that you only have to do one "sync global"
call.