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

[Sean Christopherson <seanjc@google.com>]

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
--