[PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test

[PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test

[Thomas Huth]

Use the kselftest_harness.h interface in this test to get TAP
output, so that it is easier for the user to see what the test
is doing. (Note: We are not using the KVM_ONE_VCPU_TEST_SUITE()
macro here since these tests are creating their VMs with the
vm_create_barebones() function, not with vm_create_with_one_vcpu())

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2:
 - Rebase to linux-next branch
 - Make "loops" variable static
 - Added Andrew's Reviewed-by

 .../selftests/kvm/set_memory_region_test.c    | 86 +++++++++----------
 1 file changed, 42 insertions(+), 44 deletions(-)

diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index 68c899d27561..a5c9bee5235a 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -16,6 +16,7 @@
 #include <test_util.h>
 #include <kvm_util.h>
 #include <processor.h>
+#include "kselftest_harness.h"
 
 /*
  * s390x needs at least 1MB alignment, and the x86_64 MOVE/DELETE tests need a
@@ -38,6 +39,8 @@ extern const uint64_t final_rip_end;
 
 static sem_t vcpu_ready;
 
+static int loops;
+
 static inline uint64_t guest_spin_on_val(uint64_t spin_val)
 {
 	uint64_t val;
@@ -219,6 +222,13 @@ static void test_move_memory_region(void)
 	kvm_vm_free(vm);
 }
 
+TEST(move_in_use_region)
+{
+	ksft_print_msg("Testing MOVE of in-use region, %d loops\n", loops);
+	for (int i = 0; i < loops; i++)
+		test_move_memory_region();
+}
+
 static void guest_code_delete_memory_region(void)
 {
 	uint64_t val;
@@ -308,12 +318,19 @@ static void test_delete_memory_region(void)
 	kvm_vm_free(vm);
 }
 
-static void test_zero_memory_regions(void)
+TEST(delete_in_use_region)
+{
+	ksft_print_msg("Testing DELETE of in-use region, %d loops\n", loops);
+	for (int i = 0; i < loops; i++)
+		test_delete_memory_region();
+}
+
+TEST(zero_memory_regions)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 
-	pr_info("Testing KVM_RUN with zero added memory regions\n");
+	ksft_print_msg("Testing KVM_RUN with zero added memory regions\n");
 
 	vm = vm_create_barebones();
 	vcpu = __vm_vcpu_add(vm, 0);
@@ -326,7 +343,7 @@ static void test_zero_memory_regions(void)
 }
 #endif /* __x86_64__ */
 
-static void test_invalid_memory_region_flags(void)
+TEST(invalid_memory_region_flags)
 {
 	uint32_t supported_flags = KVM_MEM_LOG_DIRTY_PAGES;
 	const uint32_t v2_only_flags = KVM_MEM_GUEST_MEMFD;
@@ -389,7 +406,7 @@ static void test_invalid_memory_region_flags(void)
  * Test it can be added memory slots up to KVM_CAP_NR_MEMSLOTS, then any
  * tentative to add further slots should fail.
  */
-static void test_add_max_memory_regions(void)
+TEST(add_max_memory_regions)
 {
 	int ret;
 	struct kvm_vm *vm;
@@ -408,13 +425,13 @@ static void test_add_max_memory_regions(void)
 	max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
 	TEST_ASSERT(max_mem_slots > 0,
 		    "KVM_CAP_NR_MEMSLOTS should be greater than 0");
-	pr_info("Allowed number of memory slots: %i\n", max_mem_slots);
+	ksft_print_msg("Allowed number of memory slots: %i\n", max_mem_slots);
 
 	vm = vm_create_barebones();
 
 	/* Check it can be added memory slots up to the maximum allowed */
-	pr_info("Adding slots 0..%i, each memory region with %dK size\n",
-		(max_mem_slots - 1), MEM_REGION_SIZE >> 10);
+	ksft_print_msg("Adding slots 0..%i, each memory region with %dK size\n",
+		       (max_mem_slots - 1), MEM_REGION_SIZE >> 10);
 
 	mem = mmap(NULL, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment,
 		   PROT_READ | PROT_WRITE,
@@ -455,12 +472,21 @@ static void test_invalid_guest_memfd(struct kvm_vm *vm, int memfd,
 	TEST_ASSERT(r == -1 && errno == EINVAL, "%s", msg);
 }
 
-static void test_add_private_memory_region(void)
+static bool has_cap_guest_memfd(void)
+{
+	return kvm_has_cap(KVM_CAP_GUEST_MEMFD) &&
+	       (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM));
+}
+
+TEST(add_private_memory_region)
 {
 	struct kvm_vm *vm, *vm2;
 	int memfd, i;
 
-	pr_info("Testing ADD of KVM_MEM_GUEST_MEMFD memory regions\n");
+	if (!has_cap_guest_memfd())
+		SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");
+
+	ksft_print_msg("Testing ADD of KVM_MEM_GUEST_MEMFD memory regions\n");
 
 	vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM);
 
@@ -491,13 +517,16 @@ static void test_add_private_memory_region(void)
 	kvm_vm_free(vm);
 }
 
-static void test_add_overlapping_private_memory_regions(void)
+TEST(add_overlapping_private_memory_regions)
 {
 	struct kvm_vm *vm;
 	int memfd;
 	int r;
 
-	pr_info("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
+	if (!has_cap_guest_memfd())
+		SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");
+
+	ksft_print_msg("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
 
 	vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM);
 
@@ -536,46 +565,15 @@ static void test_add_overlapping_private_memory_regions(void)
 	close(memfd);
 	kvm_vm_free(vm);
 }
+
 #endif
 
 int main(int argc, char *argv[])
 {
-#ifdef __x86_64__
-	int i, loops;
-
-	/*
-	 * FIXME: the zero-memslot test fails on aarch64 and s390x because
-	 * KVM_RUN fails with ENOEXEC or EFAULT.
-	 */
-	test_zero_memory_regions();
-#endif
-
-	test_invalid_memory_region_flags();
-
-	test_add_max_memory_regions();
-
-#ifdef __x86_64__
-	if (kvm_has_cap(KVM_CAP_GUEST_MEMFD) &&
-	    (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))) {
-		test_add_private_memory_region();
-		test_add_overlapping_private_memory_regions();
-	} else {
-		pr_info("Skipping tests for KVM_MEM_GUEST_MEMFD memory regions\n");
-	}
-
 	if (argc > 1)
 		loops = atoi_positive("Number of iterations", argv[1]);
 	else
 		loops = 10;
 
-	pr_info("Testing MOVE of in-use region, %d loops\n", loops);
-	for (i = 0; i < loops; i++)
-		test_move_memory_region();
-
-	pr_info("Testing DELETE of in-use region, %d loops\n", loops);
-	for (i = 0; i < loops; i++)
-		test_delete_memory_region();
-#endif
-
-	return 0;
+	return test_harness_run(argc, argv);
 }
-- 
2.44.0

Re: [PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test

[Muhammad Usama Anjum]

On 4/26/24 4:45 PM, Thomas Huth wrote:
> Use the kselftest_harness.h interface in this test to get TAP
> output, so that it is easier for the user to see what the test
> is doing. (Note: We are not using the KVM_ONE_VCPU_TEST_SUITE()
> macro here since these tests are creating their VMs with the
> vm_create_barebones() function, not with vm_create_with_one_vcpu())
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
LGTM

Reviewed-by: Muhammad Usama Anjum <usama.anjum@colabora.com>
Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> ---
>  v2:
>  - Rebase to linux-next branch
>  - Make "loops" variable static
>  - Added Andrew's Reviewed-by
> 
>  .../selftests/kvm/set_memory_region_test.c    | 86 +++++++++----------
>  1 file changed, 42 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index 68c899d27561..a5c9bee5235a 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -16,6 +16,7 @@
>  #include <test_util.h>
>  #include <kvm_util.h>
>  #include <processor.h>
> +#include "kselftest_harness.h"
>  
>  /*
>   * s390x needs at least 1MB alignment, and the x86_64 MOVE/DELETE tests need a
> @@ -38,6 +39,8 @@ extern const uint64_t final_rip_end;
>  
>  static sem_t vcpu_ready;
>  
> +static int loops;
> +
>  static inline uint64_t guest_spin_on_val(uint64_t spin_val)
>  {
>  	uint64_t val;
> @@ -219,6 +222,13 @@ static void test_move_memory_region(void)
>  	kvm_vm_free(vm);
>  }
>  
> +TEST(move_in_use_region)
> +{
> +	ksft_print_msg("Testing MOVE of in-use region, %d loops\n", loops);
> +	for (int i = 0; i < loops; i++)
> +		test_move_memory_region();
> +}
> +
>  static void guest_code_delete_memory_region(void)
>  {
>  	uint64_t val;
> @@ -308,12 +318,19 @@ static void test_delete_memory_region(void)
>  	kvm_vm_free(vm);
>  }
>  
> -static void test_zero_memory_regions(void)
> +TEST(delete_in_use_region)
> +{
> +	ksft_print_msg("Testing DELETE of in-use region, %d loops\n", loops);
> +	for (int i = 0; i < loops; i++)
> +		test_delete_memory_region();
> +}
> +
> +TEST(zero_memory_regions)
>  {
>  	struct kvm_vcpu *vcpu;
>  	struct kvm_vm *vm;
>  
> -	pr_info("Testing KVM_RUN with zero added memory regions\n");
> +	ksft_print_msg("Testing KVM_RUN with zero added memory regions\n");
>  
>  	vm = vm_create_barebones();
>  	vcpu = __vm_vcpu_add(vm, 0);
> @@ -326,7 +343,7 @@ static void test_zero_memory_regions(void)
>  }
>  #endif /* __x86_64__ */
>  
> -static void test_invalid_memory_region_flags(void)
> +TEST(invalid_memory_region_flags)
>  {
>  	uint32_t supported_flags = KVM_MEM_LOG_DIRTY_PAGES;
>  	const uint32_t v2_only_flags = KVM_MEM_GUEST_MEMFD;
> @@ -389,7 +406,7 @@ static void test_invalid_memory_region_flags(void)
>   * Test it can be added memory slots up to KVM_CAP_NR_MEMSLOTS, then any
>   * tentative to add further slots should fail.
>   */
> -static void test_add_max_memory_regions(void)
> +TEST(add_max_memory_regions)
>  {
>  	int ret;
>  	struct kvm_vm *vm;
> @@ -408,13 +425,13 @@ static void test_add_max_memory_regions(void)
>  	max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
>  	TEST_ASSERT(max_mem_slots > 0,
>  		    "KVM_CAP_NR_MEMSLOTS should be greater than 0");
> -	pr_info("Allowed number of memory slots: %i\n", max_mem_slots);
> +	ksft_print_msg("Allowed number of memory slots: %i\n", max_mem_slots);
>  
>  	vm = vm_create_barebones();
>  
>  	/* Check it can be added memory slots up to the maximum allowed */
> -	pr_info("Adding slots 0..%i, each memory region with %dK size\n",
> -		(max_mem_slots - 1), MEM_REGION_SIZE >> 10);
> +	ksft_print_msg("Adding slots 0..%i, each memory region with %dK size\n",
> +		       (max_mem_slots - 1), MEM_REGION_SIZE >> 10);
>  
>  	mem = mmap(NULL, (size_t)max_mem_slots * MEM_REGION_SIZE + alignment,
>  		   PROT_READ | PROT_WRITE,
> @@ -455,12 +472,21 @@ static void test_invalid_guest_memfd(struct kvm_vm *vm, int memfd,
>  	TEST_ASSERT(r == -1 && errno == EINVAL, "%s", msg);
>  }
>  
> -static void test_add_private_memory_region(void)
> +static bool has_cap_guest_memfd(void)
> +{
> +	return kvm_has_cap(KVM_CAP_GUEST_MEMFD) &&
> +	       (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM));
> +}
> +
> +TEST(add_private_memory_region)
>  {
>  	struct kvm_vm *vm, *vm2;
>  	int memfd, i;
>  
> -	pr_info("Testing ADD of KVM_MEM_GUEST_MEMFD memory regions\n");
> +	if (!has_cap_guest_memfd())
> +		SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");
> +
> +	ksft_print_msg("Testing ADD of KVM_MEM_GUEST_MEMFD memory regions\n");
>  
>  	vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM);
>  
> @@ -491,13 +517,16 @@ static void test_add_private_memory_region(void)
>  	kvm_vm_free(vm);
>  }
>  
> -static void test_add_overlapping_private_memory_regions(void)
> +TEST(add_overlapping_private_memory_regions)
>  {
>  	struct kvm_vm *vm;
>  	int memfd;
>  	int r;
>  
> -	pr_info("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
> +	if (!has_cap_guest_memfd())
> +		SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");
> +
> +	ksft_print_msg("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
>  
>  	vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM);
>  
> @@ -536,46 +565,15 @@ static void test_add_overlapping_private_memory_regions(void)
>  	close(memfd);
>  	kvm_vm_free(vm);
>  }
> +
>  #endif
>  
>  int main(int argc, char *argv[])
>  {
> -#ifdef __x86_64__
> -	int i, loops;
> -
> -	/*
> -	 * FIXME: the zero-memslot test fails on aarch64 and s390x because
> -	 * KVM_RUN fails with ENOEXEC or EFAULT.
> -	 */
> -	test_zero_memory_regions();
> -#endif
> -
> -	test_invalid_memory_region_flags();
> -
> -	test_add_max_memory_regions();
> -
> -#ifdef __x86_64__
> -	if (kvm_has_cap(KVM_CAP_GUEST_MEMFD) &&
> -	    (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))) {
> -		test_add_private_memory_region();
> -		test_add_overlapping_private_memory_regions();
> -	} else {
> -		pr_info("Skipping tests for KVM_MEM_GUEST_MEMFD memory regions\n");
> -	}
> -
>  	if (argc > 1)
>  		loops = atoi_positive("Number of iterations", argv[1]);
>  	else
>  		loops = 10;
>  
> -	pr_info("Testing MOVE of in-use region, %d loops\n", loops);
> -	for (i = 0; i < loops; i++)
> -		test_move_memory_region();
> -
> -	pr_info("Testing DELETE of in-use region, %d loops\n", loops);
> -	for (i = 0; i < loops; i++)
> -		test_delete_memory_region();
> -#endif
> -
> -	return 0;
> +	return test_harness_run(argc, argv);
>  }

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test

[Sean Christopherson]

On Fri, Apr 26, 2024, Thomas Huth wrote:
> Use the kselftest_harness.h interface in this test to get TAP
> output, so that it is easier for the user to see what the test
> is doing. (Note: We are not using the KVM_ONE_VCPU_TEST_SUITE()
> macro here since these tests are creating their VMs with the
> vm_create_barebones() function, not with vm_create_with_one_vcpu())
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2:
>  - Rebase to linux-next branch
>  - Make "loops" variable static
>  - Added Andrew's Reviewed-by
> 
>  .../selftests/kvm/set_memory_region_test.c    | 86 +++++++++----------
>  1 file changed, 42 insertions(+), 44 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index 68c899d27561..a5c9bee5235a 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -16,6 +16,7 @@
>  #include <test_util.h>
>  #include <kvm_util.h>
>  #include <processor.h>
> +#include "kselftest_harness.h"
>  
>  /*
>   * s390x needs at least 1MB alignment, and the x86_64 MOVE/DELETE tests need a
> @@ -38,6 +39,8 @@ extern const uint64_t final_rip_end;
>  
>  static sem_t vcpu_ready;
>  
> +static int loops;

...

> -static void test_add_overlapping_private_memory_regions(void)
> +TEST(add_overlapping_private_memory_regions)
>  {
>  	struct kvm_vm *vm;
>  	int memfd;
>  	int r;
>  
> -	pr_info("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
> +	if (!has_cap_guest_memfd())
> +		SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");

I like that we can actually report sub-tests as being skipped, but I don't like
having multiple ways to express requirements.  And IMO, this is much less readable
than TEST_REQUIRE(has_cap_guest_memfd());

AIUI, each test runs in a child process, so TEST_REQUIRE() can simply exit(), it
just needs to avoid ksft_exit_skip() so that a sub-test doesn't spit out the full
test summary.

And if using exit() isn't an option, setjmp()+longjmp() will do the trick (I got
that working for KVM_ONE_VCPU_TEST() before I realized tests run as a child).

The below is lightly tested, but I think it does what we want?

I also think we would effectively forbid direct use of TEST().  Partly because
it's effectively necessary to use TEST_REQUIRE(), but also so that all tests will
have an existing single point of contact if we need/want to make similar changes
in the future.

Lastly, would using a fixture allow throwing "loops" into a structure that is
passed to each sub-test?  Having the global is obviously not a big deal, but it'd
be nice if the early conversions to the TAP-friendly framework demonstrate the
"right" way to do things, because they'll inevitably become the blueprint for all
future conversions.

---
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 2 May 2024 12:32:04 -0700
Subject: [PATCH] KVM: selftests: Allow using TEST_REQUIRE in kselftest harness
 testcases

TODO: write me

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/include/kvm_test_harness.h  |  4 ++++
 .../testing/selftests/kvm/include/test_util.h | 24 +++++++++++++++----
 tools/testing/selftests/kvm/lib/kvm_util.c    |  2 ++
 .../selftests/kvm/x86_64/vmx_pmu_caps_test.c  |  3 +--
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_test_harness.h b/tools/testing/selftests/kvm/include/kvm_test_harness.h
index 8f7c6858e8e2..eda1c08c7c2b 100644
--- a/tools/testing/selftests/kvm/include/kvm_test_harness.h
+++ b/tools/testing/selftests/kvm/include/kvm_test_harness.h
@@ -9,6 +9,7 @@
 #define SELFTEST_KVM_TEST_HARNESS_H
 
 #include "kselftest_harness.h"
+#include "test_util.h"
 
 #define KVM_ONE_VCPU_TEST_SUITE(name)					\
 	FIXTURE(name) {							\
@@ -29,7 +30,10 @@ static void __suite##_##test(struct kvm_vcpu *vcpu);			\
 TEST_F(suite, test)							\
 {									\
 	vcpu_arch_set_entry_point(self->vcpu, guestcode);		\
+									\
+	kvm_is_sub_test = true;						\
 	__suite##_##test(self->vcpu);					\
+	kvm_is_sub_test = NULL;						\
 }									\
 static void __suite##_##test(struct kvm_vcpu *vcpu)
 
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 3e473058849f..64c9f128fef4 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -36,10 +36,26 @@ static inline int _no_printf(const char *format, ...) { return 0; }
 #endif
 
 void __printf(1, 2) print_skip(const char *fmt, ...);
-#define __TEST_REQUIRE(f, fmt, ...)				\
-do {								\
-	if (!(f))						\
-		ksft_exit_skip("- " fmt "\n", ##__VA_ARGS__);	\
+
+extern bool kvm_is_sub_test;
+
+/*
+ * Skip the test if a required capability/feature/whatever is not available,
+ * e.g. due to lack of support in the underlying hardware, running against an
+ * older kernel/KVM, etc.  Use ksft_test_result_skip() for sub-tests to avoid
+ * spuriously printing the summary of the entire test suite.  Note, sub-tests
+ * run in a child process, and so can exit() directly, e.g. don't need to
+ * longjmp() out or do something similar to avoid killing the test as a whole.
+ */
+#define __TEST_REQUIRE(f, fmt, ...)						\
+do {										\
+	if (!(f)) {								\
+		if (kvm_is_sub_test) {						\
+			ksft_test_result_skip("- " fmt "\n", ##__VA_ARGS__);	\
+			exit(KSFT_SKIP);					\
+		}								\
+		ksft_exit_skip("- " fmt "\n", ##__VA_ARGS__);			\
+	}									\
 } while (0)
 
 #define TEST_REQUIRE(f) __TEST_REQUIRE(f, "Requirement not met: %s", #f)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 6b2158655baa..4b24c454fd33 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -19,6 +19,8 @@
 
 #define KVM_UTIL_MIN_PFN	2
 
+bool kvm_is_sub_test;
+
 uint32_t guest_random_seed;
 struct guest_random_state guest_rng;
 
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
index 7c92536551cc..a58e0b1c2ee5 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
@@ -195,8 +195,7 @@ KVM_ONE_VCPU_TEST(vmx_pmu_caps, lbr_perf_capabilities, guest_code)
 {
 	int r;
 
-	if (!host_cap.lbr_format)
-		return;
+	TEST_REQUIRE(host_cap.lbr_format);
 
 	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
 	vcpu_set_msr(vcpu, MSR_LBR_TOS, 7);

base-commit: 2489e6c9ebb57d6d0e98936479b5f586201379c7
-- 

Re: [PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test

[Thomas Huth]

On 02/05/2024 21.37, Sean Christopherson wrote:
> On Fri, Apr 26, 2024, Thomas Huth wrote:
>> Use the kselftest_harness.h interface in this test to get TAP
>> output, so that it is easier for the user to see what the test
>> is doing. (Note: We are not using the KVM_ONE_VCPU_TEST_SUITE()
>> macro here since these tests are creating their VMs with the
>> vm_create_barebones() function, not with vm_create_with_one_vcpu())
>>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   v2:
>>   - Rebase to linux-next branch
>>   - Make "loops" variable static
>>   - Added Andrew's Reviewed-by
>>
>>   .../selftests/kvm/set_memory_region_test.c    | 86 +++++++++----------
>>   1 file changed, 42 insertions(+), 44 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
>> index 68c899d27561..a5c9bee5235a 100644
>> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
>> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
>> @@ -16,6 +16,7 @@
>>   #include <test_util.h>
>>   #include <kvm_util.h>
>>   #include <processor.h>
>> +#include "kselftest_harness.h"
>>   
>>   /*
>>    * s390x needs at least 1MB alignment, and the x86_64 MOVE/DELETE tests need a
>> @@ -38,6 +39,8 @@ extern const uint64_t final_rip_end;
>>   
>>   static sem_t vcpu_ready;
>>   
>> +static int loops;
> 
> ...
> 
>> -static void test_add_overlapping_private_memory_regions(void)
>> +TEST(add_overlapping_private_memory_regions)
>>   {
>>   	struct kvm_vm *vm;
>>   	int memfd;
>>   	int r;
>>   
>> -	pr_info("Testing ADD of overlapping KVM_MEM_GUEST_MEMFD memory regions\n");
>> +	if (!has_cap_guest_memfd())
>> +		SKIP(return, "Missing KVM_MEM_GUEST_MEMFD / KVM_X86_SW_PROTECTED_VM");
> 
> I like that we can actually report sub-tests as being skipped, but I don't like
> having multiple ways to express requirements.  And IMO, this is much less readable
> than TEST_REQUIRE(has_cap_guest_memfd());
> 
> AIUI, each test runs in a child process, so TEST_REQUIRE() can simply exit(), it
> just needs to avoid ksft_exit_skip() so that a sub-test doesn't spit out the full
> test summary.
> 
> And if using exit() isn't an option, setjmp()+longjmp() will do the trick (I got
> that working for KVM_ONE_VCPU_TEST() before I realized tests run as a child).
> 
> The below is lightly tested, but I think it does what we want?

Not quite ... for example, if I force vmx_pmu_caps_test to skip the last 
test, I get:

TAP version 13
1..5
# Starting 5 tests from 1 test cases.
#  RUN           vmx_pmu_caps.guest_wrmsr_perf_capabilities ...
#            OK  vmx_pmu_caps.guest_wrmsr_perf_capabilities
ok 1 vmx_pmu_caps.guest_wrmsr_perf_capabilities
#  RUN           vmx_pmu_caps.basic_perf_capabilities ...
#            OK  vmx_pmu_caps.basic_perf_capabilities
ok 2 vmx_pmu_caps.basic_perf_capabilities
#  RUN           vmx_pmu_caps.fungible_perf_capabilities ...
#            OK  vmx_pmu_caps.fungible_perf_capabilities
ok 3 vmx_pmu_caps.fungible_perf_capabilities
#  RUN           vmx_pmu_caps.immutable_perf_capabilities ...
#            OK  vmx_pmu_caps.immutable_perf_capabilities
ok 4 vmx_pmu_caps.immutable_perf_capabilities
#  RUN           vmx_pmu_caps.lbr_perf_capabilities ...
ok 5 # SKIP - Requirement not met: host_cap.lbr_format && 0
#            OK  vmx_pmu_caps.lbr_perf_capabilities
ok 5 vmx_pmu_caps.lbr_perf_capabilities
# PASSED: 5 / 5 tests passed.
# Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0

As you can see, the "ok 5" line is duplicated now, once marked with "# SKIP" 
and once as successfull. I don't think that this is valid TAP anymore?

> I also think we would effectively forbid direct use of TEST().  Partly because
> it's effectively necessary to use TEST_REQUIRE(), but also so that all tests will
> have an existing single point of contact if we need/want to make similar changes
> in the future.

Ok, but I wrote in the patch description, KVM_ONE_VCPU_TEST_SUITE() does not 
work for the set_memory_region test since it does not like to have a 
pre-defined vcpu ... so if we want to forbid TEST(), I assume we'd need 
another macro like KVM_BAREBONE_TEST_SUITE() ?

Not sure whether I really like it, though, since I'd prefer if we could keep 
the possibility to use the original selftest macros (for people who are 
already used to those macros from other selftests).

  Thomas

Re: [PATCH v2] KVM: selftests: Use TAP interface in the set_memory_region test

[Sean Christopherson]

On Fri, May 03, 2024, Thomas Huth wrote:
> On 02/05/2024 21.37, Sean Christopherson wrote:
> > On Fri, Apr 26, 2024, Thomas Huth wrote:
> > I like that we can actually report sub-tests as being skipped, but I don't like
> > having multiple ways to express requirements.  And IMO, this is much less readable
> > than TEST_REQUIRE(has_cap_guest_memfd());
> > 
> > AIUI, each test runs in a child process, so TEST_REQUIRE() can simply exit(), it
> > just needs to avoid ksft_exit_skip() so that a sub-test doesn't spit out the full
> > test summary.
> > 
> > And if using exit() isn't an option, setjmp()+longjmp() will do the trick (I got
> > that working for KVM_ONE_VCPU_TEST() before I realized tests run as a child).
> > 
> > The below is lightly tested, but I think it does what we want?
> 
> Not quite ... for example, if I force vmx_pmu_caps_test to skip the last
> test, I get:

...

> As you can see, the "ok 5" line is duplicated now, once marked with "# SKIP"
> and once as successfull. I don't think that this is valid TAP anymore?

Ah, IIUC, it's because the test reports a SKIP, and then also eventually exits
with KSFT_SKIP too.

> > I also think we would effectively forbid direct use of TEST().  Partly because
> > it's effectively necessary to use TEST_REQUIRE(), but also so that all tests will
> > have an existing single point of contact if we need/want to make similar changes
> > in the future.
> 
> Ok, but I wrote in the patch description, KVM_ONE_VCPU_TEST_SUITE() does not
> work for the set_memory_region test since it does not like to have a
> pre-defined vcpu ... so if we want to forbid TEST(), I assume we'd need
> another macro like KVM_BAREBONE_TEST_SUITE() ?

Yeah, though we probably don't need BAREBONE, e.g. KVM_TEST_SUITE() would suffice.
The "barebones" terminology exists for VMs because the vanilla "create VM" helpers
do waay more than the bare minimum, whereas I don't think we'll have the same
issues here.

> Not sure whether I really like it, though, since I'd prefer if we could keep
> the possibility to use the original selftest macros (for people who are
> already used to those macros from other selftests).

The more I fiddle with the kselftests harness, the more I'm opposed to using it
directly.  The harness code heavily assumes a "simple" environment, i.e. a test
environment without libraries.  E.g. including kselftest_harness.h without invoking
test_harness_run() fails due to unused functions, and including it in multiple
compilation units, e.g. to allow using its macros in utilities, fails due to
duplicate symbols.

It's obviously possible to split kselftest_harness.h to get around the immediate
issues, but that doesn't help with SKIP() (and other macros) only being usable at
the top-level TEST().  And using the undersored globals and functions params,
i.e. the "private" stuff, directly seems like a bad idea, e.g. the odds of KVM
selftests being broken by changes to the common code would be too high for my
taste.

While I agree it would be nice to not diverge from the common kselftest framework,
as far as things like SKIP and ASSERT macros go, that ship sailed a long time ago,
as TEST_REQUIRE() and TEST_ASSERT() usage is ubiquitous throughout KVM selftests.
And given the limitations of the common framework versus what we have in KVM's
framework, I don't see us converging on the common framework.

It's not perfect, but the best idea I can come up with is to trampoline the skip
out through KVM's harness and on to the common harness.

---
 .../selftests/kvm/include/kvm_test_harness.h  | 11 ++++++-
 .../testing/selftests/kvm/include/test_util.h | 31 ++++++++++++++++++-
 tools/testing/selftests/kvm/lib/kvm_util.c    |  2 ++
 .../selftests/kvm/x86_64/vmx_pmu_caps_test.c  |  3 +-
 4 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_test_harness.h b/tools/testing/selftests/kvm/include/kvm_test_harness.h
index 8f7c6858e8e2..fa4b5f707135 100644
--- a/tools/testing/selftests/kvm/include/kvm_test_harness.h
+++ b/tools/testing/selftests/kvm/include/kvm_test_harness.h
@@ -9,6 +9,7 @@
 #define SELFTEST_KVM_TEST_HARNESS_H
 
 #include "kselftest_harness.h"
+#include "test_util.h"
 
 #define KVM_ONE_VCPU_TEST_SUITE(name)					\
 	FIXTURE(name) {							\
@@ -28,8 +29,16 @@ static void __suite##_##test(struct kvm_vcpu *vcpu);			\
 									\
 TEST_F(suite, test)							\
 {									\
+	struct kvm_selftests_subtest subtest;				\
+									\
 	vcpu_arch_set_entry_point(self->vcpu, guestcode);		\
-	__suite##_##test(self->vcpu);					\
+									\
+	kvm_subtest = &subtest;						\
+	if (!setjmp(subtest.buf))					\
+		__suite##_##test(self->vcpu);				\
+	else								\
+		SKIP(, "%s", subtest.reason);				\
+	kvm_subtest = NULL;						\
 }									\
 static void __suite##_##test(struct kvm_vcpu *vcpu)
 
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 3e473058849f..2fce7b2ed07c 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -8,6 +8,7 @@
 #ifndef SELFTEST_KVM_TEST_UTIL_H
 #define SELFTEST_KVM_TEST_UTIL_H
 
+#include <setjmp.h>
 #include <stdlib.h>
 #include <stdarg.h>
 #include <stdbool.h>
@@ -18,8 +19,22 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/mman.h>
+
 #include "kselftest.h"
 
+/*
+ * kvm_selftests_subtest is used to trampoline from __TEST_REQUIRE(), which can
+ * be called from any context, e.g. KVM selftests library code, out to the test
+ * being run, as the kselftests harness only allows calling SKIP() (and other
+ * macros/helpers) from the top-level TEST_*() context.
+ */
+struct kvm_selftests_subtest {
+	char reason[1024];
+	jmp_buf buf;
+};
+
+extern struct kvm_selftests_subtest *kvm_subtest;
+
 #define msecs_to_usecs(msec)    ((msec) * 1000ULL)
 
 static inline int _no_printf(const char *format, ...) { return 0; }
@@ -36,10 +51,24 @@ static inline int _no_printf(const char *format, ...) { return 0; }
 #endif
 
 void __printf(1, 2) print_skip(const char *fmt, ...);
+
+/*
+ * Skip the test if a required capability/feature/whatever is not available,
+ * e.g. due to lack of support in the underlying hardware, running against an
+ * older kernel/KVM, etc.  If a subtest is running, skip only the subtest,
+ * otherwise treat the requirement as applying to the entire test.
+ */
 #define __TEST_REQUIRE(f, fmt, ...)				\
 do {								\
-	if (!(f))						\
+	if (!(f)) {						\
+		if (kvm_subtest) {				\
+			snprintf(kvm_subtest->reason,		\
+				 sizeof(kvm_subtest->reason),	\
+				 fmt, ##__VA_ARGS__);		\
+			longjmp(kvm_subtest->buf, 1);		\
+		}						\
 		ksft_exit_skip("- " fmt "\n", ##__VA_ARGS__);	\
+	}							\
 } while (0)
 
 #define TEST_REQUIRE(f) __TEST_REQUIRE(f, "Requirement not met: %s", #f)
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 6b2158655baa..9601eed3ed57 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -19,6 +19,8 @@
 
 #define KVM_UTIL_MIN_PFN	2
 
+struct kvm_selftests_subtest *kvm_subtest;
+
 uint32_t guest_random_seed;
 struct guest_random_state guest_rng;
 
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
index 7c92536551cc..a58e0b1c2ee5 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
@@ -195,8 +195,7 @@ KVM_ONE_VCPU_TEST(vmx_pmu_caps, lbr_perf_capabilities, guest_code)
 {
 	int r;
 
-	if (!host_cap.lbr_format)
-		return;
+	TEST_REQUIRE(host_cap.lbr_format);
 
 	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
 	vcpu_set_msr(vcpu, MSR_LBR_TOS, 7);

base-commit: df8e458e70ce925b533ff33f1e8979650cf44e3e
--