linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] Use TAP in some more KVM selftests
@ 2022-10-04  9:31 Thomas Huth
  2022-10-04  9:31 ` [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test Thomas Huth
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Thomas Huth @ 2022-10-04  9:31 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson, kvm
  Cc: Shuah Khan, linux-kselftest, linux-kernel

Many KVM selftests are completely silent. This has the disadvantage
for the users that they do not know what's going on here. For example,
some time ago, a tester asked me how to know whether a certain new
sub-test has been added to one of the s390x test binaries or not (which
he didn't compile on his own), which is hard to judge when there is no
output. So I finally went ahead and implemented TAP output in the
s390x-specific tests some months ago.

Now I wonder whether that could be a good strategy for the x86 and
generic tests, too? As a little RFC patch series, I've converted
three more KVM selftests to use TAP output. If we decide that this
is the right way to go, I can work on other tests later, too.

Thomas Huth (3):
  KVM: selftests: Use TAP interface in the kvm_binary_stats_test
  KVM: selftests: x86: Use TAP interface in the sync_regs test
  KVM: selftests: x86: Use TAP interface in the tsc_msrs_test

 .../selftests/kvm/kvm_binary_stats_test.c     |  11 +-
 .../selftests/kvm/x86_64/sync_regs_test.c     | 113 ++++++++++++++----
 .../selftests/kvm/x86_64/tsc_msrs_test.c      |  16 ++-
 3 files changed, 114 insertions(+), 26 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test
  2022-10-04  9:31 [RFC PATCH 0/3] Use TAP in some more KVM selftests Thomas Huth
@ 2022-10-04  9:31 ` Thomas Huth
  2022-10-05  8:33   ` Andrew Jones
  2022-10-04  9:31 ` [PATCH 2/3] KVM: selftests: x86: Use TAP interface in the sync_regs test Thomas Huth
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2022-10-04  9:31 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson, kvm
  Cc: Shuah Khan, linux-kselftest, linux-kernel

The kvm_binary_stats_test test currently does not have any output (unless
one of the TEST_ASSERT statement fails), so it's hard to say for a user
how far it did proceed already. Thus let's make this a little bit more
user-friendly and include some TAP output via the kselftest.h interface.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tools/testing/selftests/kvm/kvm_binary_stats_test.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index 0b45ac593387..894417c96f70 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -19,6 +19,7 @@
 #include "kvm_util.h"
 #include "asm/kvm.h"
 #include "linux/kvm.h"
+#include "kselftest.h"
 
 static void stats_test(int stats_fd)
 {
@@ -51,7 +52,7 @@ static void stats_test(int stats_fd)
 
 	/* Sanity check for other fields in header */
 	if (header.num_desc == 0) {
-		printf("No KVM stats defined!");
+		ksft_print_msg("No KVM stats defined!\n");
 		return;
 	}
 	/*
@@ -224,9 +225,13 @@ int main(int argc, char *argv[])
 			max_vcpu = DEFAULT_NUM_VCPU;
 	}
 
+	ksft_print_header();
+
 	/* Check the extension for binary stats */
 	TEST_REQUIRE(kvm_has_cap(KVM_CAP_BINARY_STATS_FD));
 
+	ksft_set_plan(max_vm);
+
 	/* Create VMs and VCPUs */
 	vms = malloc(sizeof(vms[0]) * max_vm);
 	TEST_ASSERT(vms, "Allocate memory for storing VM pointers");
@@ -245,10 +250,12 @@ int main(int argc, char *argv[])
 		vm_stats_test(vms[i]);
 		for (j = 0; j < max_vcpu; ++j)
 			vcpu_stats_test(vcpus[i * max_vcpu + j]);
+		ksft_test_result_pass("vm%i\n", i);
 	}
 
 	for (i = 0; i < max_vm; ++i)
 		kvm_vm_free(vms[i]);
 	free(vms);
-	return 0;
+
+	ksft_finished();	/* Print results and exit() accordingly */
 }
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] KVM: selftests: x86: Use TAP interface in the sync_regs test
  2022-10-04  9:31 [RFC PATCH 0/3] Use TAP in some more KVM selftests Thomas Huth
  2022-10-04  9:31 ` [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test Thomas Huth
@ 2022-10-04  9:31 ` Thomas Huth
  2022-10-04  9:31 ` [PATCH 3/3] KVM: selftests: x86: Use TAP interface in the tsc_msrs_test Thomas Huth
  2022-10-14 21:03 ` [RFC PATCH 0/3] Use TAP in some more KVM selftests Sean Christopherson
  3 siblings, 0 replies; 10+ messages in thread
From: Thomas Huth @ 2022-10-04  9:31 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson, kvm
  Cc: Shuah Khan, linux-kselftest, linux-kernel

The sync_regs test currently does not have any output (unless one
of the TEST_ASSERT statement fails), so it's hard to say for a user
whether a certain new sub-test has been included in the binary or
not. Let's make this a little bit more user-friendly and include
some TAP output via the kselftests.h interface.
To be able to distinguish the different sub-tests more easily, we
also break up the huge main() function here in more fine grained
parts (similar to what has been done to the s390 sync_regs_test
already).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .../selftests/kvm/x86_64/sync_regs_test.c     | 113 ++++++++++++++----
 1 file changed, 93 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
index 9b6db0b0b13e..8583b425a3e0 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -80,26 +80,11 @@ static void compare_vcpu_events(struct kvm_vcpu_events *left,
 #define TEST_SYNC_FIELDS   (KVM_SYNC_X86_REGS|KVM_SYNC_X86_SREGS|KVM_SYNC_X86_EVENTS)
 #define INVALID_SYNC_FIELD 0x80000000
 
-int main(int argc, char *argv[])
-{
-	struct kvm_vcpu *vcpu;
-	struct kvm_vm *vm;
-	struct kvm_run *run;
-	struct kvm_regs regs;
-	struct kvm_sregs sregs;
-	struct kvm_vcpu_events events;
-	int rv, cap;
-
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
-	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
-	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
-	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
-
-	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 
-	run = vcpu->run;
+void test_read_invalid(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	int rv;
 
 	/* Request reading invalid register set from VCPU. */
 	run->kvm_valid_regs = INVALID_SYNC_FIELD;
@@ -115,6 +100,12 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	run->kvm_valid_regs = 0;
+}
+
+void test_set_invalid(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	int rv;
 
 	/* Request setting invalid register set into VCPU. */
 	run->kvm_dirty_regs = INVALID_SYNC_FIELD;
@@ -130,11 +121,21 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	run->kvm_dirty_regs = 0;
+}
+
+void test_req_and_verify_all_valid_regs(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_vcpu_events events;
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Request and verify all valid register sets. */
 	/* TODO: BUILD TIME CHECK: TEST_ASSERT(KVM_SYNC_X86_NUM_FIELDS != 3); */
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
 	rv = _vcpu_run(vcpu);
+	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -148,6 +149,15 @@ int main(int argc, char *argv[])
 
 	vcpu_events_get(vcpu, &events);
 	compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+void test_set_and_verify_various_reg_values(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_vcpu_events events;
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Set and verify various register values. */
 	run->s.regs.regs.rbx = 0xBAD1DEA;
@@ -157,6 +167,7 @@ int main(int argc, char *argv[])
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
 	run->kvm_dirty_regs = KVM_SYNC_X86_REGS | KVM_SYNC_X86_SREGS;
 	rv = _vcpu_run(vcpu);
+	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -176,6 +187,12 @@ int main(int argc, char *argv[])
 
 	vcpu_events_get(vcpu, &events);
 	compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+void test_clear_kvm_dirty_regs_bits(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	int rv;
 
 	/* Clear kvm_dirty_regs bits, verify new s.regs values are
 	 * overwritten with existing guest values.
@@ -184,6 +201,7 @@ int main(int argc, char *argv[])
 	run->kvm_dirty_regs = 0;
 	run->s.regs.regs.rbx = 0xDEADBEEF;
 	rv = _vcpu_run(vcpu);
+	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -191,6 +209,13 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(run->s.regs.regs.rbx != 0xDEADBEEF,
 		    "rbx sync regs value incorrect 0x%llx.",
 		    run->s.regs.regs.rbx);
+}
+
+void test_clear_kvm_valid_and_dirty_regs(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Clear kvm_valid_regs bits and kvm_dirty_bits.
 	 * Verify s.regs values are not overwritten with existing guest values
@@ -199,9 +224,11 @@ int main(int argc, char *argv[])
 	run->kvm_valid_regs = 0;
 	run->kvm_dirty_regs = 0;
 	run->s.regs.regs.rbx = 0xAAAA;
+	vcpu_regs_get(vcpu, &regs);
 	regs.rbx = 0xBAC0;
 	vcpu_regs_set(vcpu, &regs);
 	rv = _vcpu_run(vcpu);
+	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -213,6 +240,13 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(regs.rbx == 0xBAC0 + 1,
 		    "rbx guest value incorrect 0x%llx.",
 		    regs.rbx);
+}
+
+void test_clear_kvm_valid_regs_bits(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Clear kvm_valid_regs bits. Verify s.regs values are not overwritten
 	 * with existing guest values but that guest values are overwritten
@@ -222,6 +256,7 @@ int main(int argc, char *argv[])
 	run->kvm_dirty_regs = TEST_SYNC_FIELDS;
 	run->s.regs.regs.rbx = 0xBBBB;
 	rv = _vcpu_run(vcpu);
+	TEST_ASSERT(rv == 0, "vcpu_run failed: %d\n", rv);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -233,8 +268,46 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(regs.rbx == 0xBBBB + 1,
 		    "rbx guest value incorrect 0x%llx.",
 		    regs.rbx);
+}
+
+struct testdef {
+	const char *name;
+	void (*test)(struct kvm_vcpu *vcpu);
+} testlist[] = {
+	{ "read invalid", test_read_invalid },
+	{ "set invalid", test_set_invalid },
+	{ "request+verify all valid regs", test_req_and_verify_all_valid_regs },
+	{ "set+verify various regs", test_set_and_verify_various_reg_values },
+	{ "clear kvm_dirty_regs bits", test_clear_kvm_dirty_regs_bits },
+	{ "clear kvm valid+dirty_regs", test_clear_kvm_valid_and_dirty_regs },
+	{ "clear kvm valid_regs bits", test_clear_kvm_valid_regs_bits },
+};
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int idx, cap;
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	ksft_print_header();
+
+	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
+	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
+	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
+
+	ksft_set_plan(ARRAY_SIZE(testlist));
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
+		testlist[idx].test(vcpu);
+		ksft_test_result_pass("%s\n", testlist[idx].name);
+	}
 
 	kvm_vm_free(vm);
 
-	return 0;
+	ksft_finished();	/* Print results and exit() accordingly */
 }
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] KVM: selftests: x86: Use TAP interface in the tsc_msrs_test
  2022-10-04  9:31 [RFC PATCH 0/3] Use TAP in some more KVM selftests Thomas Huth
  2022-10-04  9:31 ` [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test Thomas Huth
  2022-10-04  9:31 ` [PATCH 2/3] KVM: selftests: x86: Use TAP interface in the sync_regs test Thomas Huth
@ 2022-10-04  9:31 ` Thomas Huth
  2022-10-14 21:03 ` [RFC PATCH 0/3] Use TAP in some more KVM selftests Sean Christopherson
  3 siblings, 0 replies; 10+ messages in thread
From: Thomas Huth @ 2022-10-04  9:31 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson, kvm
  Cc: Shuah Khan, linux-kselftest, linux-kernel

Let's add some output here so that the user has some feedback
about what is being run.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .../testing/selftests/kvm/x86_64/tsc_msrs_test.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/tsc_msrs_test.c b/tools/testing/selftests/kvm/x86_64/tsc_msrs_test.c
index 22d366c697f7..c9f67702f657 100644
--- a/tools/testing/selftests/kvm/x86_64/tsc_msrs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/tsc_msrs_test.c
@@ -72,11 +72,16 @@ static void run_vcpu(struct kvm_vcpu *vcpu, int stage)
 
 	switch (get_ucall(vcpu, &uc)) {
 	case UCALL_SYNC:
-		TEST_ASSERT(!strcmp((const char *)uc.args[0], "hello") &&
-			    uc.args[1] == stage + 1, "Stage %d: Unexpected register values vmexit, got %lx",
-			    stage + 1, (ulong)uc.args[1]);
+		if (!strcmp((const char *)uc.args[0], "hello") &&
+		    uc.args[1] == stage + 1)
+			ksft_test_result_pass("stage %d passed\n", stage + 1);
+		else
+			ksft_test_result_fail(
+				"stage %d: Unexpected register values vmexit, got %lx",
+				stage + 1, (ulong)uc.args[1]);
 		return;
 	case UCALL_DONE:
+		ksft_test_result_pass("stage %d passed\n", stage + 1);
 		return;
 	case UCALL_ABORT:
 		REPORT_GUEST_ASSERT_2(uc, "values: %#lx, %#lx");
@@ -92,6 +97,9 @@ int main(void)
 	struct kvm_vm *vm;
 	uint64_t val;
 
+	ksft_print_header();
+	ksft_set_plan(5);
+
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 
 	val = 0;
@@ -149,5 +157,5 @@ int main(void)
 
 	kvm_vm_free(vm);
 
-	return 0;
+	ksft_finished();	/* Print results and exit() accordingly */
 }
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test
  2022-10-04  9:31 ` [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test Thomas Huth
@ 2022-10-05  8:33   ` Andrew Jones
  2022-10-07  1:22     ` Sean Christopherson
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Jones @ 2022-10-05  8:33 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Paolo Bonzini, Sean Christopherson, kvm, Shuah Khan,
	linux-kselftest, linux-kernel

On Tue, Oct 04, 2022 at 11:31:29AM +0200, Thomas Huth wrote:
> The kvm_binary_stats_test test currently does not have any output (unless
> one of the TEST_ASSERT statement fails), so it's hard to say for a user
> how far it did proceed already. Thus let's make this a little bit more
> user-friendly and include some TAP output via the kselftest.h interface.

I like the idea of switching the entire kvm selftests framework and all
tests to the ksft TAP interface. But, if we want to do that, then the
question is whether we should start by partially using it for some tests,
and then eventually switch over the framework, or whether we should try to
switch everything at once.

I think I prefer the latter, because without changing the framework we
can't provide full TAP anyway as TEST_ASSERT exits with 254 instead of 1
for a fail-exit and it doesn't output a final TAP test summary either.

All that said, this patch doesn't make the current situation worse and
it may help inspire the grand conversion, so

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks,
drew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test
  2022-10-05  8:33   ` Andrew Jones
@ 2022-10-07  1:22     ` Sean Christopherson
  0 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-10-07  1:22 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Thomas Huth, Paolo Bonzini, kvm, Shuah Khan, linux-kselftest,
	linux-kernel

On Wed, Oct 05, 2022, Andrew Jones wrote:
> On Tue, Oct 04, 2022 at 11:31:29AM +0200, Thomas Huth wrote:
> > The kvm_binary_stats_test test currently does not have any output (unless
> > one of the TEST_ASSERT statement fails), so it's hard to say for a user
> > how far it did proceed already. Thus let's make this a little bit more
> > user-friendly and include some TAP output via the kselftest.h interface.
> 
> I like the idea of switching the entire kvm selftests framework and all
> tests to the ksft TAP interface. But, if we want to do that, then the
> question is whether we should start by partially using it for some tests,
> and then eventually switch over the framework, or whether we should try to
> switch everything at once.
> 
> I think I prefer the latter, because without changing the framework we
> can't provide full TAP anyway as TEST_ASSERT exits with 254 instead of 1
> for a fail-exit and it doesn't output a final TAP test summary either.

I would much prefer the latter, e.g. with common entry/exit points[*], much of the
boilerplate can be done once in common code.  I bet we could even figure out a way
to have tests default to setting a plan of '1' so that simple tests don't need to
care about TAP at all.

[*] https://lore.kernel.org/all/20220915000448.1674802-2-vannapurve@google.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 0/3] Use TAP in some more KVM selftests
  2022-10-04  9:31 [RFC PATCH 0/3] Use TAP in some more KVM selftests Thomas Huth
                   ` (2 preceding siblings ...)
  2022-10-04  9:31 ` [PATCH 3/3] KVM: selftests: x86: Use TAP interface in the tsc_msrs_test Thomas Huth
@ 2022-10-14 21:03 ` Sean Christopherson
  2022-11-08  1:06   ` David Matlack
  3 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-10-14 21:03 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Paolo Bonzini, kvm, Shuah Khan, linux-kselftest, linux-kernel

On Tue, Oct 04, 2022, Thomas Huth wrote:
> Many KVM selftests are completely silent. This has the disadvantage
> for the users that they do not know what's going on here. For example,
> some time ago, a tester asked me how to know whether a certain new
> sub-test has been added to one of the s390x test binaries or not (which
> he didn't compile on his own), which is hard to judge when there is no
> output. So I finally went ahead and implemented TAP output in the
> s390x-specific tests some months ago.
> 
> Now I wonder whether that could be a good strategy for the x86 and
> generic tests, too?

Taking Andrew's thoughts a step further, I'm in favor of adding TAP output, but
only if we implement it in such a way that it reduces the burden on writing new
tests.  I _really_ like that sync_regs_test's subtests are split into consumable
chunks, but I worry that the amount of boilerplate needed will deter test writes
and increase the maintenance cost.

And my experience with KVM-unit-tests is that letting test writers specify strings
for test names is a bad idea, e.g. using an arbitrary string creates a disconnect
between what the user sees and what code is running, and makes it unnecessarily
difficult to connect a failure back to code.  And if we ever support running
specific testcases by name (I'm still not sure this is a net positive), arbitrary
strings get really annoying because inevitably an arbitrary string will contain
characters that need to be escaped in the shell.

Adding a macro or three to let tests define and run testscases with minimal effort
would more or less eliminate the boilerplate.  And in theory providing semi-rigid
macros would help force simple tests to conform to standard patterns, which should
reduce the cost of someone new understanding the test, and would likely let us do
more automagic things in the future.

E.g. something like this in the test:

	KVM_RUN_TESTCASES(vcpu,
		test_clear_kvm_dirty_regs_bits,
		test_set_invalid,
		test_req_and_verify_all_valid_regs,
		test_set_and_verify_various_reg_values,
		test_clear_kvm_dirty_regs_bits,
	);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 0/3] Use TAP in some more KVM selftests
  2022-10-14 21:03 ` [RFC PATCH 0/3] Use TAP in some more KVM selftests Sean Christopherson
@ 2022-11-08  1:06   ` David Matlack
  2022-11-09 19:22     ` Sean Christopherson
  2022-11-10  7:45     ` Thomas Huth
  0 siblings, 2 replies; 10+ messages in thread
From: David Matlack @ 2022-11-08  1:06 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Thomas Huth, Paolo Bonzini, kvm, Shuah Khan, linux-kselftest,
	linux-kernel

On Fri, Oct 14, 2022 at 09:03:59PM +0000, Sean Christopherson wrote:
> On Tue, Oct 04, 2022, Thomas Huth wrote:
> > Many KVM selftests are completely silent. This has the disadvantage
> > for the users that they do not know what's going on here. For example,
> > some time ago, a tester asked me how to know whether a certain new
> > sub-test has been added to one of the s390x test binaries or not (which
> > he didn't compile on his own), which is hard to judge when there is no
> > output. So I finally went ahead and implemented TAP output in the
> > s390x-specific tests some months ago.
> > 
> > Now I wonder whether that could be a good strategy for the x86 and
> > generic tests, too?
> 
> Taking Andrew's thoughts a step further, I'm in favor of adding TAP output, but
> only if we implement it in such a way that it reduces the burden on writing new
> tests.  I _really_ like that sync_regs_test's subtests are split into consumable
> chunks, but I worry that the amount of boilerplate needed will deter test writes
> and increase the maintenance cost.
> 
> And my experience with KVM-unit-tests is that letting test writers specify strings
> for test names is a bad idea, e.g. using an arbitrary string creates a disconnect
> between what the user sees and what code is running, and makes it unnecessarily
> difficult to connect a failure back to code.  And if we ever support running
> specific testcases by name (I'm still not sure this is a net positive), arbitrary
> strings get really annoying because inevitably an arbitrary string will contain
> characters that need to be escaped in the shell.
> 
> Adding a macro or three to let tests define and run testscases with minimal effort
> would more or less eliminate the boilerplate.  And in theory providing semi-rigid
> macros would help force simple tests to conform to standard patterns, which should
> reduce the cost of someone new understanding the test, and would likely let us do
> more automagic things in the future.
> 
> E.g. something like this in the test:
> 
> 	KVM_RUN_TESTCASES(vcpu,
> 		test_clear_kvm_dirty_regs_bits,
> 		test_set_invalid,
> 		test_req_and_verify_all_valid_regs,
> 		test_set_and_verify_various_reg_values,
> 		test_clear_kvm_dirty_regs_bits,
> 	);

There is an existing framework in
tools/testing/selftests/kselftest_harness.h that provides macros for
setting up and running tests cases. I converted sync_regs_test to use it
below as an example [1].

The harness runs each subtest in a child process, so sharing a VM/VCPU
across test cases is not possible. This means setting up and tearing
down a VM for every test case, but the harness makes this pretty easy
with FIXTURE_{SETUP,TEARDOWN}(). With this harness, we can keep using
TEST_ASSERT() as-is, and still run all test cases even if one fails.
Plus no need for the hard-coded ksft_*() calls in main().

[1]

diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
index 9b6db0b0b13e..11cf25d3e4a3 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -20,6 +20,8 @@
 #include "kvm_util.h"
 #include "processor.h"
 
+#include "../kselftest_harness.h"
+
 #define UCALL_PIO_PORT ((uint16_t)0x1000)
 
 struct ucall uc_none = {
@@ -80,26 +82,23 @@ static void compare_vcpu_events(struct kvm_vcpu_events *left,
 #define TEST_SYNC_FIELDS   (KVM_SYNC_X86_REGS|KVM_SYNC_X86_SREGS|KVM_SYNC_X86_EVENTS)
 #define INVALID_SYNC_FIELD 0x80000000
 
-int main(int argc, char *argv[])
-{
-	struct kvm_vcpu *vcpu;
+FIXTURE(sync_regs_test) {
 	struct kvm_vm *vm;
-	struct kvm_run *run;
-	struct kvm_regs regs;
-	struct kvm_sregs sregs;
-	struct kvm_vcpu_events events;
-	int rv, cap;
-
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
+	struct kvm_vcpu *vcpu;
+};
 
-	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
-	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
-	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
+FIXTURE_SETUP(sync_regs_test) {
+	self->vm = vm_create_with_one_vcpu(&self->vcpu, guest_code);
+}
 
-	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+FIXTURE_TEARDOWN(sync_regs_test) {
+	kvm_vm_free(self->vm);
+}
 
-	run = vcpu->run;
+TEST_F(sync_regs_test, read_invalid) {
+	struct kvm_run *run = self->vcpu->run;
+	struct kvm_vcpu *vcpu = self->vcpu;
+	int rv;
 
 	/* Request reading invalid register set from VCPU. */
 	run->kvm_valid_regs = INVALID_SYNC_FIELD;
@@ -115,6 +114,12 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	run->kvm_valid_regs = 0;
+}
+
+TEST_F(sync_regs_test, set_invalid) {
+	struct kvm_run *run = self->vcpu->run;
+	struct kvm_vcpu *vcpu = self->vcpu;
+	int rv;
 
 	/* Request setting invalid register set into VCPU. */
 	run->kvm_dirty_regs = INVALID_SYNC_FIELD;
@@ -130,6 +135,15 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	run->kvm_dirty_regs = 0;
+}
+
+TEST_F(sync_regs_test, req_and_verify_all_valid) {
+	struct kvm_run *run = self->vcpu->run;
+	struct kvm_vcpu *vcpu = self->vcpu;
+	struct kvm_vcpu_events events;
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Request and verify all valid register sets. */
 	/* TODO: BUILD TIME CHECK: TEST_ASSERT(KVM_SYNC_X86_NUM_FIELDS != 3); */
@@ -148,6 +162,22 @@ int main(int argc, char *argv[])
 
 	vcpu_events_get(vcpu, &events);
 	compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+TEST_F(sync_regs_test, set_and_verify_various) {
+	struct kvm_run *run = self->vcpu->run;
+	struct kvm_vcpu *vcpu = self->vcpu;
+	struct kvm_vcpu_events events;
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
+	int rv;
+
+	run->kvm_valid_regs = TEST_SYNC_FIELDS;
+	rv = _vcpu_run(vcpu);
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+		    "Unexpected exit reason: %u (%s),\n",
+		    run->exit_reason,
+		    exit_reason_str(run->exit_reason));
 
 	/* Set and verify various register values. */
 	run->s.regs.regs.rbx = 0xBAD1DEA;
@@ -176,6 +206,13 @@ int main(int argc, char *argv[])
 
 	vcpu_events_get(vcpu, &events);
 	compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+TEST_F(sync_regs_test, clear_kvm_valid_and_dirty) {
+	struct kvm_run *run = self->vcpu->run;
+	struct kvm_vcpu *vcpu = self->vcpu;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Clear kvm_dirty_regs bits, verify new s.regs values are
 	 * overwritten with existing guest values.
@@ -199,6 +236,7 @@ int main(int argc, char *argv[])
 	run->kvm_valid_regs = 0;
 	run->kvm_dirty_regs = 0;
 	run->s.regs.regs.rbx = 0xAAAA;
+	vcpu_regs_get(vcpu, &regs);
 	regs.rbx = 0xBAC0;
 	vcpu_regs_set(vcpu, &regs);
 	rv = _vcpu_run(vcpu);
@@ -213,6 +251,20 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(regs.rbx == 0xBAC0 + 1,
 		    "rbx guest value incorrect 0x%llx.",
 		    regs.rbx);
+}
+
+TEST_F(sync_regs_test, clear_kvm_valid_regs) {
+	struct kvm_run *run = self->vcpu->run;
+	struct kvm_vcpu *vcpu = self->vcpu;
+	struct kvm_regs regs;
+	int rv;
+
+	run->kvm_valid_regs = TEST_SYNC_FIELDS;
+	rv = _vcpu_run(vcpu);
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+		    "Unexpected exit reason: %u (%s),\n",
+		    run->exit_reason,
+		    exit_reason_str(run->exit_reason));
 
 	/* Clear kvm_valid_regs bits. Verify s.regs values are not overwritten
 	 * with existing guest values but that guest values are overwritten
@@ -233,8 +285,15 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(regs.rbx == 0xBBBB + 1,
 		    "rbx guest value incorrect 0x%llx.",
 		    regs.rbx);
+}
+
+int main(int argc, char **argv)
+{
+	int cap;
 
-	kvm_vm_free(vm);
+	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
+	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
+	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
 
-	return 0;
+	return test_harness_run(argc, argv);
 }

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 0/3] Use TAP in some more KVM selftests
  2022-11-08  1:06   ` David Matlack
@ 2022-11-09 19:22     ` Sean Christopherson
  2022-11-10  7:45     ` Thomas Huth
  1 sibling, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-11-09 19:22 UTC (permalink / raw)
  To: David Matlack
  Cc: Thomas Huth, Paolo Bonzini, kvm, Shuah Khan, linux-kselftest,
	linux-kernel

On Mon, Nov 07, 2022, David Matlack wrote:
> On Fri, Oct 14, 2022 at 09:03:59PM +0000, Sean Christopherson wrote:
> > On Tue, Oct 04, 2022, Thomas Huth wrote:
> > Adding a macro or three to let tests define and run testscases with minimal effort
> > would more or less eliminate the boilerplate.  And in theory providing semi-rigid
> > macros would help force simple tests to conform to standard patterns, which should
> > reduce the cost of someone new understanding the test, and would likely let us do
> > more automagic things in the future.
> > 
> > E.g. something like this in the test:
> > 
> > 	KVM_RUN_TESTCASES(vcpu,
> > 		test_clear_kvm_dirty_regs_bits,
> > 		test_set_invalid,
> > 		test_req_and_verify_all_valid_regs,
> > 		test_set_and_verify_various_reg_values,
> > 		test_clear_kvm_dirty_regs_bits,
> > 	);
> 
> There is an existing framework in
> tools/testing/selftests/kselftest_harness.h that provides macros for
> setting up and running tests cases. I converted sync_regs_test to use it
> below as an example [1].

Looks awesome!  Some thoughts to cut down on boilerplate below.  We'll also need
to deal with the ASSERT_EQ conflict.  Easiest thing there is to rename KVM's
version to TEST_ASSERT_EQ(), which IMO is an improvement irrespective of this
conversion.

> The harness runs each subtest in a child process, so sharing a VM/VCPU
> across test cases is not possible. This means setting up and tearing
> down a VM for every test case, 

This is a feature, not a bug.  My single biggest complaint about KVM-unit-tests
is the lack of isolation between sub-tests.  E.g. there have been far too many
bugs where sub-tests fail if run on their own due to sub-tests relying on setup
being done elsewhere.

> but the harness makes this pretty easy with FIXTURE_{SETUP,TEARDOWN}(). With
> this harness, we can keep using TEST_ASSERT() as-is, and still run all test
> cases even if one fails.  Plus no need for the hard-coded ksft_*() calls in
> main().
> +FIXTURE(sync_regs_test) {
>  	struct kvm_vm *vm;

A dedicated "vm" field isn't necessary, it's available in the vcpu.

> +	struct kvm_vcpu *vcpu;
> +};
>  
> -	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
> -	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
> -	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
> +FIXTURE_SETUP(sync_regs_test) {
> +	self->vm = vm_create_with_one_vcpu(&self->vcpu, guest_code);
> +}
>  
> -	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +FIXTURE_TEARDOWN(sync_regs_test) {
> +	kvm_vm_free(self->vm);

From above, this would be:

	kvm_vm_free(self->vcpu->vm);

> +}
>  
> -	run = vcpu->run;
> +TEST_F(sync_regs_test, read_invalid) {

Regardless of what other selftests do, IMO we should dress these up to look like
the functions they are, i.e. put the curly brace on its own line.

> +	struct kvm_run *run = self->vcpu->run;

I don't love the @self boilerplate, and the setup+teardown will be identical for
the vast majority of simple tests.  There will also be tests that want to run
different guest code.

What if we add our own wrappers (because one can never have enough macros) to
handle most of the boilerplate?  Sample conversion below (the wrapper macros would
obviously go in a common header).  And then to support per-testcase guest code, we
could add vcpu_arch_set_guest_code() and another wrapper, e.g.

#define KVM_ONE_VCPU_TEST_EX(suite, test, guest_code)				\
static void __suite##_##test(struct kvm_vcpu *vcpu);				\
										\
TEST_F(suite, test)								\
{										\
	vcpu_arch_set_guest_code(guest_code);					\
	__suite##_##test(self->vcpu);						\
}										\
static void __suite##_##test(struct kvm_vcpu *vcpu)


Alternatives to "suite" would be bundle, crate, cluster, etc...

---
 .../selftests/kvm/x86_64/sync_regs_test.c     | 119 ++++++++++++++----
 1 file changed, 93 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
index 9b6db0b0b13e..b805170980bb 100644
--- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
@@ -20,6 +20,9 @@
 #include "kvm_util.h"
 #include "processor.h"
 
+#undef ASSERT_EQ
+#include "../kselftest_harness.h"
+
 #define UCALL_PIO_PORT ((uint16_t)0x1000)
 
 struct ucall uc_none = {
@@ -80,26 +83,34 @@ static void compare_vcpu_events(struct kvm_vcpu_events *left,
 #define TEST_SYNC_FIELDS   (KVM_SYNC_X86_REGS|KVM_SYNC_X86_SREGS|KVM_SYNC_X86_EVENTS)
 #define INVALID_SYNC_FIELD 0x80000000
 
-int main(int argc, char *argv[])
+#define KVM_ONE_VCPU_TEST_SUITE(name, guest_code)				\
+	FIXTURE(name) {								\
+		struct kvm_vcpu *vcpu;						\
+	};									\
+										\
+	FIXTURE_SETUP(name) {							\
+		(void)vm_create_with_one_vcpu(&self->vcpu, guest_code);		\
+	}									\
+										\
+	FIXTURE_TEARDOWN(name) {						\
+		kvm_vm_free(self->vcpu->vm);					\
+	}
+
+#define KVM_ONE_VCPU_TEST(suite, test)						\
+static void __suite##_##test(struct kvm_vcpu *vcpu);				\
+										\
+TEST_F(suite, test)								\
+{										\
+	__suite##_##test(self->vcpu);						\
+}										\
+static void __suite##_##test(struct kvm_vcpu *vcpu)
+
+KVM_ONE_VCPU_TEST_SUITE(sync_regs_test, guest_code);
+
+KVM_ONE_VCPU_TEST(sync_regs_test, read_invalid)
 {
-	struct kvm_vcpu *vcpu;
-	struct kvm_vm *vm;
-	struct kvm_run *run;
-	struct kvm_regs regs;
-	struct kvm_sregs sregs;
-	struct kvm_vcpu_events events;
-	int rv, cap;
-
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
-	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
-	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
-	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
-
-	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
-
-	run = vcpu->run;
+	struct kvm_run *run = vcpu->run;
+	int rv;
 
 	/* Request reading invalid register set from VCPU. */
 	run->kvm_valid_regs = INVALID_SYNC_FIELD;
@@ -115,6 +126,12 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	run->kvm_valid_regs = 0;
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, set_invalid)
+{
+	struct kvm_run *run = vcpu->run;
+	int rv;
 
 	/* Request setting invalid register set into VCPU. */
 	run->kvm_dirty_regs = INVALID_SYNC_FIELD;
@@ -130,11 +147,19 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	run->kvm_dirty_regs = 0;
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, req_and_verify_all_valid)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_vcpu_events events;
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
 
 	/* Request and verify all valid register sets. */
 	/* TODO: BUILD TIME CHECK: TEST_ASSERT(KVM_SYNC_X86_NUM_FIELDS != 3); */
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
-	rv = _vcpu_run(vcpu);
+	vcpu_run(vcpu);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -148,6 +173,21 @@ int main(int argc, char *argv[])
 
 	vcpu_events_get(vcpu, &events);
 	compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, set_and_verify_various)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_vcpu_events events;
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
+
+	run->kvm_valid_regs = TEST_SYNC_FIELDS;
+	vcpu_run(vcpu);
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+		    "Unexpected exit reason: %u (%s),\n",
+		    run->exit_reason,
+		    exit_reason_str(run->exit_reason));
 
 	/* Set and verify various register values. */
 	run->s.regs.regs.rbx = 0xBAD1DEA;
@@ -156,7 +196,7 @@ int main(int argc, char *argv[])
 
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
 	run->kvm_dirty_regs = KVM_SYNC_X86_REGS | KVM_SYNC_X86_SREGS;
-	rv = _vcpu_run(vcpu);
+	vcpu_run(vcpu);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -176,6 +216,12 @@ int main(int argc, char *argv[])
 
 	vcpu_events_get(vcpu, &events);
 	compare_vcpu_events(&events, &run->s.regs.events);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, clear_kvm_valid_and_dirty)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_regs regs;
 
 	/* Clear kvm_dirty_regs bits, verify new s.regs values are
 	 * overwritten with existing guest values.
@@ -183,7 +229,7 @@ int main(int argc, char *argv[])
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
 	run->kvm_dirty_regs = 0;
 	run->s.regs.regs.rbx = 0xDEADBEEF;
-	rv = _vcpu_run(vcpu);
+	vcpu_run(vcpu);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -199,9 +245,10 @@ int main(int argc, char *argv[])
 	run->kvm_valid_regs = 0;
 	run->kvm_dirty_regs = 0;
 	run->s.regs.regs.rbx = 0xAAAA;
+	vcpu_regs_get(vcpu, &regs);
 	regs.rbx = 0xBAC0;
 	vcpu_regs_set(vcpu, &regs);
-	rv = _vcpu_run(vcpu);
+	vcpu_run(vcpu);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -213,6 +260,19 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(regs.rbx == 0xBAC0 + 1,
 		    "rbx guest value incorrect 0x%llx.",
 		    regs.rbx);
+}
+
+KVM_ONE_VCPU_TEST(sync_regs_test, clear_kvm_valid_regs)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_regs regs;
+
+	run->kvm_valid_regs = TEST_SYNC_FIELDS;
+	vcpu_run(vcpu);
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+		    "Unexpected exit reason: %u (%s),\n",
+		    run->exit_reason,
+		    exit_reason_str(run->exit_reason));
 
 	/* Clear kvm_valid_regs bits. Verify s.regs values are not overwritten
 	 * with existing guest values but that guest values are overwritten
@@ -221,7 +281,7 @@ int main(int argc, char *argv[])
 	run->kvm_valid_regs = 0;
 	run->kvm_dirty_regs = TEST_SYNC_FIELDS;
 	run->s.regs.regs.rbx = 0xBBBB;
-	rv = _vcpu_run(vcpu);
+	vcpu_run(vcpu);
 	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
 		    "Unexpected exit reason: %u (%s),\n",
 		    run->exit_reason,
@@ -233,8 +293,15 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(regs.rbx == 0xBBBB + 1,
 		    "rbx guest value incorrect 0x%llx.",
 		    regs.rbx);
+}
+
+int main(int argc, char **argv)
+{
+	int cap;
 
-	kvm_vm_free(vm);
+	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
+	TEST_REQUIRE((cap & TEST_SYNC_FIELDS) == TEST_SYNC_FIELDS);
+	TEST_REQUIRE(!(cap & INVALID_SYNC_FIELD));
 
-	return 0;
+	return test_harness_run(argc, argv);
 }

base-commit: d663b8a285986072428a6a145e5994bc275df994
-- 


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 0/3] Use TAP in some more KVM selftests
  2022-11-08  1:06   ` David Matlack
  2022-11-09 19:22     ` Sean Christopherson
@ 2022-11-10  7:45     ` Thomas Huth
  1 sibling, 0 replies; 10+ messages in thread
From: Thomas Huth @ 2022-11-10  7:45 UTC (permalink / raw)
  To: David Matlack, Sean Christopherson
  Cc: Paolo Bonzini, kvm, Shuah Khan, linux-kselftest, linux-kernel

On 08/11/2022 02.06, David Matlack wrote:
> On Fri, Oct 14, 2022 at 09:03:59PM +0000, Sean Christopherson wrote:
>> On Tue, Oct 04, 2022, Thomas Huth wrote:
>>> Many KVM selftests are completely silent. This has the disadvantage
>>> for the users that they do not know what's going on here. For example,
>>> some time ago, a tester asked me how to know whether a certain new
>>> sub-test has been added to one of the s390x test binaries or not (which
>>> he didn't compile on his own), which is hard to judge when there is no
>>> output. So I finally went ahead and implemented TAP output in the
>>> s390x-specific tests some months ago.
>>>
>>> Now I wonder whether that could be a good strategy for the x86 and
>>> generic tests, too?
>>
>> Taking Andrew's thoughts a step further, I'm in favor of adding TAP output, but
>> only if we implement it in such a way that it reduces the burden on writing new
>> tests.  I _really_ like that sync_regs_test's subtests are split into consumable
>> chunks, but I worry that the amount of boilerplate needed will deter test writes
>> and increase the maintenance cost.
>>
>> And my experience with KVM-unit-tests is that letting test writers specify strings
>> for test names is a bad idea, e.g. using an arbitrary string creates a disconnect
>> between what the user sees and what code is running, and makes it unnecessarily
>> difficult to connect a failure back to code.  And if we ever support running
>> specific testcases by name (I'm still not sure this is a net positive), arbitrary
>> strings get really annoying because inevitably an arbitrary string will contain
>> characters that need to be escaped in the shell.
>>
>> Adding a macro or three to let tests define and run testscases with minimal effort
>> would more or less eliminate the boilerplate.  And in theory providing semi-rigid
>> macros would help force simple tests to conform to standard patterns, which should
>> reduce the cost of someone new understanding the test, and would likely let us do
>> more automagic things in the future.
>>
>> E.g. something like this in the test:
>>
>> 	KVM_RUN_TESTCASES(vcpu,
>> 		test_clear_kvm_dirty_regs_bits,
>> 		test_set_invalid,
>> 		test_req_and_verify_all_valid_regs,
>> 		test_set_and_verify_various_reg_values,
>> 		test_clear_kvm_dirty_regs_bits,
>> 	);
> 
> There is an existing framework in
> tools/testing/selftests/kselftest_harness.h that provides macros for
> setting up and running tests cases. I converted sync_regs_test to use it
> below as an example [1].
> 
> The harness runs each subtest in a child process, so sharing a VM/VCPU
> across test cases is not possible. This means setting up and tearing
> down a VM for every test case, but the harness makes this pretty easy
> with FIXTURE_{SETUP,TEARDOWN}(). With this harness, we can keep using
> TEST_ASSERT() as-is, and still run all test cases even if one fails.
> Plus no need for the hard-coded ksft_*() calls in main().

  Hi!

Sorry for not getting back to this earlier - I'm pretty much busy with other 
stuff right now. But your suggestion looks really cool, I like it - so if 
you've got some spare time to work on the conversion, please go ahead (I 
won't have much time to work on this in the next weeks, I think)!

  Thomas


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-11-10  7:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-04  9:31 [RFC PATCH 0/3] Use TAP in some more KVM selftests Thomas Huth
2022-10-04  9:31 ` [PATCH 1/3] KVM: selftests: Use TAP interface in the kvm_binary_stats_test Thomas Huth
2022-10-05  8:33   ` Andrew Jones
2022-10-07  1:22     ` Sean Christopherson
2022-10-04  9:31 ` [PATCH 2/3] KVM: selftests: x86: Use TAP interface in the sync_regs test Thomas Huth
2022-10-04  9:31 ` [PATCH 3/3] KVM: selftests: x86: Use TAP interface in the tsc_msrs_test Thomas Huth
2022-10-14 21:03 ` [RFC PATCH 0/3] Use TAP in some more KVM selftests Sean Christopherson
2022-11-08  1:06   ` David Matlack
2022-11-09 19:22     ` Sean Christopherson
2022-11-10  7:45     ` Thomas Huth

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).