[PATCH 0/4] KVM: s390: selftests: Provide TAP output in tests

[PATCH 0/4] KVM: s390: selftests: Provide TAP output in tests

[Thomas Huth]

This patch series is motivated by Shuah's suggestion here:

 https://lore.kernel.org/kvm/d576d8f7-980f-3bc6-87ad-5a6ae45609b8@linuxfoundation.org/

Many s390x KVM selftests do not output any information about which
tests have been run, so it's hard to say whether a test binary
contains a certain sub-test or not. To improve this situation let's
add some TAP output via the kselftest.h interface to these tests,
so that it easier to understand what has been executed or not.

Thomas Huth (4):
  KVM: s390: selftests: Use TAP interface in the memop test
  KVM: s390: selftests: Use TAP interface in the sync_regs test
  KVM: s390: selftests: Use TAP interface in the tprot test
  KVM: s390: selftests: Use TAP interface in the reset test

 tools/testing/selftests/kvm/s390x/memop.c     | 90 +++++++++++++++----
 tools/testing/selftests/kvm/s390x/resets.c    | 38 ++++++--
 .../selftests/kvm/s390x/sync_regs_test.c      | 86 +++++++++++++-----
 tools/testing/selftests/kvm/s390x/tprot.c     | 12 ++-
 4 files changed, 179 insertions(+), 47 deletions(-)

-- 
2.27.0

[PATCH 1/4] KVM: s390: selftests: Use TAP interface in the memop test

[Thomas Huth]

The memop 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.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tools/testing/selftests/kvm/s390x/memop.c | 90 ++++++++++++++++++-----
 1 file changed, 73 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
index b04c2c1b3c30..a2783d9afcac 100644
--- a/tools/testing/selftests/kvm/s390x/memop.c
+++ b/tools/testing/selftests/kvm/s390x/memop.c
@@ -12,6 +12,7 @@
 
 #include "test_util.h"
 #include "kvm_util.h"
+#include "kselftest.h"
 
 enum mop_target {
 	LOGICAL,
@@ -648,33 +649,88 @@ static void test_errors(void)
 	kvm_vm_free(t.kvm_vm);
 }
 
+struct testdef {
+	const char *name;
+	void (*test)(void);
+	bool needs_extension;
+} testlist[] = {
+	{
+		.name = "simple copy",
+		.test = test_copy,
+		.needs_extension = false,
+	},
+	{
+		.name = "copy with storage keys",
+		.test = test_copy_key,
+		.needs_extension = true,
+	},
+	{
+		.name = "copy with key storage protection override",
+		.test = test_copy_key_storage_prot_override,
+		.needs_extension = true,
+	},
+	{
+		.name = "copy with key fetch protection",
+		.test = test_copy_key_fetch_prot,
+		.needs_extension = true,
+	},
+	{
+		.name = "copy with key fetch protection override",
+		.test = test_copy_key_fetch_prot_override,
+		.needs_extension = true,
+	},
+	{
+		.name = "error checks with key",
+		.test = test_errors_key,
+		.needs_extension = true,
+	},
+	{
+		.name = "error checks with key storage protection override",
+		.test = test_errors_key_storage_prot_override,
+		.needs_extension = true,
+	},
+	{
+		.name = "error checks without key fetch prot override",
+		.test = test_errors_key_fetch_prot_override_not_enabled,
+		.needs_extension = true,
+	},
+	{
+		.name = "error checks with key fetch prot override",
+		.test = test_errors_key_fetch_prot_override_enabled,
+		.needs_extension = true,
+	},
+	{
+		.name = "generic error checks",
+		.test = test_errors,
+		.needs_extension = false,
+	},
+};
+
 int main(int argc, char *argv[])
 {
-	int memop_cap, extension_cap;
+	int memop_cap, extension_cap, idx;
 
 	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
 
+	ksft_print_header();
+
 	memop_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP);
 	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
 	if (!memop_cap) {
-		print_skip("CAP_S390_MEM_OP not supported");
-		exit(KSFT_SKIP);
+		ksft_exit_skip("CAP_S390_MEM_OP not supported.\n");
 	}
 
-	test_copy();
-	if (extension_cap > 0) {
-		test_copy_key();
-		test_copy_key_storage_prot_override();
-		test_copy_key_fetch_prot();
-		test_copy_key_fetch_prot_override();
-		test_errors_key();
-		test_errors_key_storage_prot_override();
-		test_errors_key_fetch_prot_override_not_enabled();
-		test_errors_key_fetch_prot_override_enabled();
-	} else {
-		print_skip("storage key memop extension not supported");
+	ksft_set_plan(ARRAY_SIZE(testlist));
+
+	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
+		if (!testlist[idx].needs_extension || extension_cap) {
+			testlist[idx].test();
+			ksft_test_result_pass("%s\n", testlist[idx].name);
+		} else {
+			ksft_test_result_skip("%s - storage key memop not supported\n",
+					      testlist[idx].name);
+		}
 	}
-	test_errors();
 
-	return 0;
+	ksft_finished();
 }
-- 
2.27.0

[PATCH 2/4] KVM: s390: selftests: Use TAP interface in the sync_regs test

[Thomas Huth]

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.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .../selftests/kvm/s390x/sync_regs_test.c      | 86 ++++++++++++++-----
 1 file changed, 65 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index caf7b8859a94..d5ddcbb82d12 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -21,6 +21,7 @@
 #include "test_util.h"
 #include "kvm_util.h"
 #include "diag318_test_handler.h"
+#include "kselftest.h"
 
 #define VCPU_ID 5
 
@@ -74,27 +75,9 @@ static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
 #define TEST_SYNC_FIELDS   (KVM_SYNC_GPRS|KVM_SYNC_ACRS|KVM_SYNC_CRS|KVM_SYNC_DIAG318)
 #define INVALID_SYNC_FIELD 0x80000000
 
-int main(int argc, char *argv[])
+void test_read_invalid(struct kvm_vm *vm, struct kvm_run *run)
 {
-	struct kvm_vm *vm;
-	struct kvm_run *run;
-	struct kvm_regs regs;
-	struct kvm_sregs sregs;
-	int rv, cap;
-
-	/* Tell stdout not to buffer its content */
-	setbuf(stdout, NULL);
-
-	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
-	if (!cap) {
-		print_skip("CAP_SYNC_REGS not supported");
-		exit(KSFT_SKIP);
-	}
-
-	/* Create VM */
-	vm = vm_create_default(VCPU_ID, 0, guest_code);
-
-	run = vcpu_state(vm, VCPU_ID);
+	int rv;
 
 	/* Request reading invalid register set from VCPU. */
 	run->kvm_valid_regs = INVALID_SYNC_FIELD;
@@ -110,6 +93,11 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
+}
+
+void test_set_invalid(struct kvm_vm *vm, struct kvm_run *run)
+{
+	int rv;
 
 	/* Request setting invalid register set into VCPU. */
 	run->kvm_dirty_regs = INVALID_SYNC_FIELD;
@@ -125,6 +113,13 @@ int main(int argc, char *argv[])
 		    "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
 		    rv);
 	vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
+}
+
+void test_req_and_verify_all_valid_regs(struct kvm_vm *vm, struct kvm_run *run)
+{
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Request and verify all valid register sets. */
 	run->kvm_valid_regs = TEST_SYNC_FIELDS;
@@ -146,6 +141,13 @@ int main(int argc, char *argv[])
 
 	vcpu_sregs_get(vm, VCPU_ID, &sregs);
 	compare_sregs(&sregs, &run->s.regs);
+}
+
+void test_set_and_verify_various_reg_values(struct kvm_vm *vm, struct kvm_run *run)
+{
+	struct kvm_sregs sregs;
+	struct kvm_regs regs;
+	int rv;
 
 	/* Set and verify various register values */
 	run->s.regs.gprs[11] = 0xBAD1DEA;
@@ -180,6 +182,11 @@ int main(int argc, char *argv[])
 
 	vcpu_sregs_get(vm, VCPU_ID, &sregs);
 	compare_sregs(&sregs, &run->s.regs);
+}
+
+void test_clear_kvm_dirty_regs_bits(struct kvm_vm *vm, struct kvm_run *run)
+{
+	int rv;
 
 	/* Clear kvm_dirty_regs bits, verify new s.regs values are
 	 * overwritten with existing guest values.
@@ -200,8 +207,45 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(run->s.regs.diag318 != 0x4B1D,
 		    "diag318 sync regs value incorrect 0x%llx.",
 		    run->s.regs.diag318);
+}
+
+struct testdef {
+	const char *name;
+	void (*test)(struct kvm_vm *vm, struct kvm_run *run);
+} 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 },
+};
+
+int main(int argc, char *argv[])
+{
+	static struct kvm_run *run;
+	static struct kvm_vm *vm;
+	int idx;
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	if (!kvm_check_cap(KVM_CAP_SYNC_REGS))
+		ksft_exit_skip("CAP_SYNC_REGS not supported");
+
+	/* Create VM */
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+
+	run = vcpu_state(vm, VCPU_ID);
+
+	ksft_print_header();
+	ksft_set_plan(ARRAY_SIZE(testlist));
+
+	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
+		testlist[idx].test(vm, run);
+		ksft_test_result_pass("%s\n", testlist[idx].name);
+	}
 
 	kvm_vm_free(vm);
 
-	return 0;
+	ksft_finished();
 }
-- 
2.27.0

[PATCH 3/4] KVM: s390: selftests: Use TAP interface in the tprot test

[Thomas Huth]

The tprot 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.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tools/testing/selftests/kvm/s390x/tprot.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
index c097b9db495e..a714b4206e95 100644
--- a/tools/testing/selftests/kvm/s390x/tprot.c
+++ b/tools/testing/selftests/kvm/s390x/tprot.c
@@ -8,6 +8,7 @@
 #include <sys/mman.h>
 #include "test_util.h"
 #include "kvm_util.h"
+#include "kselftest.h"
 
 #define PAGE_SHIFT 12
 #define PAGE_SIZE (1 << PAGE_SHIFT)
@@ -69,6 +70,7 @@ enum stage {
 	STAGE_INIT_FETCH_PROT_OVERRIDE,
 	TEST_FETCH_PROT_OVERRIDE,
 	TEST_STORAGE_PROT_OVERRIDE,
+	NUM_STAGES			/* this must be the last entry */
 };
 
 struct test {
@@ -196,6 +198,7 @@ static void guest_code(void)
 	}									\
 	ASSERT_EQ(uc.cmd, UCALL_SYNC);						\
 	ASSERT_EQ(uc.args[1], __stage);						\
+	ksft_test_result_pass("" #stage "\n");					\
 })
 
 int main(int argc, char *argv[])
@@ -204,6 +207,9 @@ int main(int argc, char *argv[])
 	struct kvm_run *run;
 	vm_vaddr_t guest_0_page;
 
+	ksft_print_header();
+	ksft_set_plan(NUM_STAGES - 1);	/* STAGE_END is not counted, thus - 1 */
+
 	vm = vm_create_default(VCPU_ID, 0, guest_code);
 	run = vcpu_state(vm, VCPU_ID);
 
@@ -213,7 +219,7 @@ int main(int argc, char *argv[])
 
 	guest_0_page = vm_vaddr_alloc(vm, PAGE_SIZE, 0);
 	if (guest_0_page != 0)
-		print_skip("Did not allocate page at 0 for fetch protection override tests");
+		ksft_print_msg("Did not allocate page at 0 for fetch protection override tests\n");
 	HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
 	if (guest_0_page == 0)
 		mprotect(addr_gva2hva(vm, (vm_vaddr_t)0), PAGE_SIZE, PROT_READ);
@@ -224,4 +230,8 @@ int main(int argc, char *argv[])
 	run->s.regs.crs[0] |= CR0_STORAGE_PROTECTION_OVERRIDE;
 	run->kvm_dirty_regs = KVM_SYNC_CRS;
 	HOST_SYNC(vm, TEST_STORAGE_PROT_OVERRIDE);
+
+	kvm_vm_free(vm);
+
+	ksft_finished();
 }
-- 
2.27.0

[PATCH 4/4] KVM: s390: selftests: Use TAP interface in the reset test

[Thomas Huth]

Let's standardize the s390x KVM selftest output to the TAP output
generated via the kselftests.h interface.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tools/testing/selftests/kvm/s390x/resets.c | 38 +++++++++++++++++-----
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
index b143db6d8693..1d649ec77260 100644
--- a/tools/testing/selftests/kvm/s390x/resets.c
+++ b/tools/testing/selftests/kvm/s390x/resets.c
@@ -12,6 +12,7 @@
 
 #include "test_util.h"
 #include "kvm_util.h"
+#include "kselftest.h"
 
 #define VCPU_ID 3
 #define LOCAL_IRQS 32
@@ -202,7 +203,7 @@ static void inject_irq(int cpu_id)
 
 static void test_normal(void)
 {
-	pr_info("Testing normal reset\n");
+	ksft_print_msg("Testing normal reset\n");
 	/* Create VM */
 	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
 	run = vcpu_state(vm, VCPU_ID);
@@ -225,7 +226,7 @@ static void test_normal(void)
 
 static void test_initial(void)
 {
-	pr_info("Testing initial reset\n");
+	ksft_print_msg("Testing initial reset\n");
 	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
 	run = vcpu_state(vm, VCPU_ID);
 	sync_regs = &run->s.regs;
@@ -247,7 +248,7 @@ static void test_initial(void)
 
 static void test_clear(void)
 {
-	pr_info("Testing clear reset\n");
+	ksft_print_msg("Testing clear reset\n");
 	vm = vm_create_default(VCPU_ID, 0, guest_code_initial);
 	run = vcpu_state(vm, VCPU_ID);
 	sync_regs = &run->s.regs;
@@ -266,14 +267,35 @@ static void test_clear(void)
 	kvm_vm_free(vm);
 }
 
+struct testdef {
+	const char *name;
+	void (*test)(void);
+	bool needs_cap;
+} testlist[] = {
+	{ "initial", test_initial, false },
+	{ "normal", test_normal, true },
+	{ "clear", test_clear, true },
+};
+
 int main(int argc, char *argv[])
 {
+	bool has_s390_vcpu_resets = kvm_check_cap(KVM_CAP_S390_VCPU_RESETS);
+	int idx;
+
 	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
 
-	test_initial();
-	if (kvm_check_cap(KVM_CAP_S390_VCPU_RESETS)) {
-		test_normal();
-		test_clear();
+	ksft_print_header();
+	ksft_set_plan(ARRAY_SIZE(testlist));
+
+	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
+		if (!testlist[idx].needs_cap || has_s390_vcpu_resets) {
+			testlist[idx].test();
+			ksft_test_result_pass("%s\n", testlist[idx].name);
+		} else {
+			ksft_test_result_skip("%s - no VCPU_RESETS capability\n",
+					      testlist[idx].name);
+		}
 	}
-	return 0;
+
+	ksft_finished();
 }
-- 
2.27.0

Re: [PATCH 2/4] KVM: s390: selftests: Use TAP interface in the sync_regs test

[Claudio Imbrenda]

On Thu, 14 Apr 2022 12:53:20 +0200
Thomas Huth <thuth@redhat.com> wrote:

> 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  .../selftests/kvm/s390x/sync_regs_test.c      | 86 ++++++++++++++-----
>  1 file changed, 65 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> index caf7b8859a94..d5ddcbb82d12 100644
> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
> @@ -21,6 +21,7 @@
>  #include "test_util.h"
>  #include "kvm_util.h"
>  #include "diag318_test_handler.h"
> +#include "kselftest.h"
>  
>  #define VCPU_ID 5
>  
> @@ -74,27 +75,9 @@ static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
>  #define TEST_SYNC_FIELDS   (KVM_SYNC_GPRS|KVM_SYNC_ACRS|KVM_SYNC_CRS|KVM_SYNC_DIAG318)
>  #define INVALID_SYNC_FIELD 0x80000000
>  
> -int main(int argc, char *argv[])
> +void test_read_invalid(struct kvm_vm *vm, struct kvm_run *run)
>  {
> -	struct kvm_vm *vm;
> -	struct kvm_run *run;
> -	struct kvm_regs regs;
> -	struct kvm_sregs sregs;
> -	int rv, cap;
> -
> -	/* Tell stdout not to buffer its content */
> -	setbuf(stdout, NULL);
> -
> -	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
> -	if (!cap) {
> -		print_skip("CAP_SYNC_REGS not supported");
> -		exit(KSFT_SKIP);
> -	}
> -
> -	/* Create VM */
> -	vm = vm_create_default(VCPU_ID, 0, guest_code);
> -
> -	run = vcpu_state(vm, VCPU_ID);
> +	int rv;
>  
>  	/* Request reading invalid register set from VCPU. */
>  	run->kvm_valid_regs = INVALID_SYNC_FIELD;
> @@ -110,6 +93,11 @@ int main(int argc, char *argv[])
>  		    "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
>  		    rv);
>  	vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
> +}
> +
> +void test_set_invalid(struct kvm_vm *vm, struct kvm_run *run)
> +{
> +	int rv;
>  
>  	/* Request setting invalid register set into VCPU. */
>  	run->kvm_dirty_regs = INVALID_SYNC_FIELD;
> @@ -125,6 +113,13 @@ int main(int argc, char *argv[])
>  		    "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
>  		    rv);
>  	vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
> +}
> +
> +void test_req_and_verify_all_valid_regs(struct kvm_vm *vm, struct kvm_run *run)
> +{
> +	struct kvm_sregs sregs;
> +	struct kvm_regs regs;
> +	int rv;
>  
>  	/* Request and verify all valid register sets. */
>  	run->kvm_valid_regs = TEST_SYNC_FIELDS;
> @@ -146,6 +141,13 @@ int main(int argc, char *argv[])
>  
>  	vcpu_sregs_get(vm, VCPU_ID, &sregs);
>  	compare_sregs(&sregs, &run->s.regs);
> +}
> +
> +void test_set_and_verify_various_reg_values(struct kvm_vm *vm, struct kvm_run *run)
> +{
> +	struct kvm_sregs sregs;
> +	struct kvm_regs regs;
> +	int rv;
>  
>  	/* Set and verify various register values */
>  	run->s.regs.gprs[11] = 0xBAD1DEA;
> @@ -180,6 +182,11 @@ int main(int argc, char *argv[])
>  
>  	vcpu_sregs_get(vm, VCPU_ID, &sregs);
>  	compare_sregs(&sregs, &run->s.regs);
> +}
> +
> +void test_clear_kvm_dirty_regs_bits(struct kvm_vm *vm, struct kvm_run *run)
> +{
> +	int rv;
>  
>  	/* Clear kvm_dirty_regs bits, verify new s.regs values are
>  	 * overwritten with existing guest values.
> @@ -200,8 +207,45 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(run->s.regs.diag318 != 0x4B1D,
>  		    "diag318 sync regs value incorrect 0x%llx.",
>  		    run->s.regs.diag318);
> +}
> +
> +struct testdef {
> +	const char *name;
> +	void (*test)(struct kvm_vm *vm, struct kvm_run *run);
> +} 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 },
> +};
> +
> +int main(int argc, char *argv[])
> +{
> +	static struct kvm_run *run;
> +	static struct kvm_vm *vm;
> +	int idx;
> +
> +	/* Tell stdout not to buffer its content */
> +	setbuf(stdout, NULL);
> +
> +	if (!kvm_check_cap(KVM_CAP_SYNC_REGS))
> +		ksft_exit_skip("CAP_SYNC_REGS not supported");

I'm not an expert on the TAP format, but wouldn't it be more meaningful
to print the header first? (like you do in the previous patch)

> +
> +	/* Create VM */
> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
> +
> +	run = vcpu_state(vm, VCPU_ID);
> +
> +	ksft_print_header();
> +	ksft_set_plan(ARRAY_SIZE(testlist));
> +
> +	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
> +		testlist[idx].test(vm, run);
> +		ksft_test_result_pass("%s\n", testlist[idx].name);
> +	}
>  
>  	kvm_vm_free(vm);
>  
> -	return 0;
> +	ksft_finished();
>  }

Re: [PATCH 3/4] KVM: s390: selftests: Use TAP interface in the tprot test

[Claudio Imbrenda]

On Thu, 14 Apr 2022 12:53:21 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The tprot 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tools/testing/selftests/kvm/s390x/tprot.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
> index c097b9db495e..a714b4206e95 100644
> --- a/tools/testing/selftests/kvm/s390x/tprot.c
> +++ b/tools/testing/selftests/kvm/s390x/tprot.c
> @@ -8,6 +8,7 @@
>  #include <sys/mman.h>
>  #include "test_util.h"
>  #include "kvm_util.h"
> +#include "kselftest.h"
>  
>  #define PAGE_SHIFT 12
>  #define PAGE_SIZE (1 << PAGE_SHIFT)
> @@ -69,6 +70,7 @@ enum stage {
>  	STAGE_INIT_FETCH_PROT_OVERRIDE,
>  	TEST_FETCH_PROT_OVERRIDE,
>  	TEST_STORAGE_PROT_OVERRIDE,
> +	NUM_STAGES			/* this must be the last entry */
>  };
>  
>  struct test {
> @@ -196,6 +198,7 @@ static void guest_code(void)
>  	}									\
>  	ASSERT_EQ(uc.cmd, UCALL_SYNC);						\
>  	ASSERT_EQ(uc.args[1], __stage);						\
> +	ksft_test_result_pass("" #stage "\n");					\
>  })
>  
>  int main(int argc, char *argv[])
> @@ -204,6 +207,9 @@ int main(int argc, char *argv[])
>  	struct kvm_run *run;
>  	vm_vaddr_t guest_0_page;
>  
> +	ksft_print_header();
> +	ksft_set_plan(NUM_STAGES - 1);	/* STAGE_END is not counted, thus - 1 */
> +
>  	vm = vm_create_default(VCPU_ID, 0, guest_code);
>  	run = vcpu_state(vm, VCPU_ID);
>  
> @@ -213,7 +219,7 @@ int main(int argc, char *argv[])
>  
>  	guest_0_page = vm_vaddr_alloc(vm, PAGE_SIZE, 0);
>  	if (guest_0_page != 0)
> -		print_skip("Did not allocate page at 0 for fetch protection override tests");
> +		ksft_print_msg("Did not allocate page at 0 for fetch protection override tests\n");

will this print a skip, though?

or you don't want to print a skip because then the numbering in the
planning doesn't match anymore? in which case, is there an easy way to
fix it?

>  	HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
>  	if (guest_0_page == 0)
>  		mprotect(addr_gva2hva(vm, (vm_vaddr_t)0), PAGE_SIZE, PROT_READ);
> @@ -224,4 +230,8 @@ int main(int argc, char *argv[])
>  	run->s.regs.crs[0] |= CR0_STORAGE_PROTECTION_OVERRIDE;
>  	run->kvm_dirty_regs = KVM_SYNC_CRS;
>  	HOST_SYNC(vm, TEST_STORAGE_PROT_OVERRIDE);
> +
> +	kvm_vm_free(vm);
> +
> +	ksft_finished();
>  }

Re: [PATCH 2/4] KVM: s390: selftests: Use TAP interface in the sync_regs test

[Thomas Huth]

On 14/04/2022 13.39, Claudio Imbrenda wrote:
> On Thu, 14 Apr 2022 12:53:20 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> 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.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   .../selftests/kvm/s390x/sync_regs_test.c      | 86 ++++++++++++++-----
>>   1 file changed, 65 insertions(+), 21 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>> index caf7b8859a94..d5ddcbb82d12 100644
>> --- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>> +++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
>> @@ -21,6 +21,7 @@
>>   #include "test_util.h"
>>   #include "kvm_util.h"
>>   #include "diag318_test_handler.h"
>> +#include "kselftest.h"
>>   
>>   #define VCPU_ID 5
>>   
>> @@ -74,27 +75,9 @@ static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
>>   #define TEST_SYNC_FIELDS   (KVM_SYNC_GPRS|KVM_SYNC_ACRS|KVM_SYNC_CRS|KVM_SYNC_DIAG318)
>>   #define INVALID_SYNC_FIELD 0x80000000
>>   
>> -int main(int argc, char *argv[])
>> +void test_read_invalid(struct kvm_vm *vm, struct kvm_run *run)
>>   {
>> -	struct kvm_vm *vm;
>> -	struct kvm_run *run;
>> -	struct kvm_regs regs;
>> -	struct kvm_sregs sregs;
>> -	int rv, cap;
>> -
>> -	/* Tell stdout not to buffer its content */
>> -	setbuf(stdout, NULL);
>> -
>> -	cap = kvm_check_cap(KVM_CAP_SYNC_REGS);
>> -	if (!cap) {
>> -		print_skip("CAP_SYNC_REGS not supported");
>> -		exit(KSFT_SKIP);
>> -	}
>> -
>> -	/* Create VM */
>> -	vm = vm_create_default(VCPU_ID, 0, guest_code);
>> -
>> -	run = vcpu_state(vm, VCPU_ID);
>> +	int rv;
>>   
>>   	/* Request reading invalid register set from VCPU. */
>>   	run->kvm_valid_regs = INVALID_SYNC_FIELD;
>> @@ -110,6 +93,11 @@ int main(int argc, char *argv[])
>>   		    "Invalid kvm_valid_regs did not cause expected KVM_RUN error: %d\n",
>>   		    rv);
>>   	vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
>> +}
>> +
>> +void test_set_invalid(struct kvm_vm *vm, struct kvm_run *run)
>> +{
>> +	int rv;
>>   
>>   	/* Request setting invalid register set into VCPU. */
>>   	run->kvm_dirty_regs = INVALID_SYNC_FIELD;
>> @@ -125,6 +113,13 @@ int main(int argc, char *argv[])
>>   		    "Invalid kvm_dirty_regs did not cause expected KVM_RUN error: %d\n",
>>   		    rv);
>>   	vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
>> +}
>> +
>> +void test_req_and_verify_all_valid_regs(struct kvm_vm *vm, struct kvm_run *run)
>> +{
>> +	struct kvm_sregs sregs;
>> +	struct kvm_regs regs;
>> +	int rv;
>>   
>>   	/* Request and verify all valid register sets. */
>>   	run->kvm_valid_regs = TEST_SYNC_FIELDS;
>> @@ -146,6 +141,13 @@ int main(int argc, char *argv[])
>>   
>>   	vcpu_sregs_get(vm, VCPU_ID, &sregs);
>>   	compare_sregs(&sregs, &run->s.regs);
>> +}
>> +
>> +void test_set_and_verify_various_reg_values(struct kvm_vm *vm, struct kvm_run *run)
>> +{
>> +	struct kvm_sregs sregs;
>> +	struct kvm_regs regs;
>> +	int rv;
>>   
>>   	/* Set and verify various register values */
>>   	run->s.regs.gprs[11] = 0xBAD1DEA;
>> @@ -180,6 +182,11 @@ int main(int argc, char *argv[])
>>   
>>   	vcpu_sregs_get(vm, VCPU_ID, &sregs);
>>   	compare_sregs(&sregs, &run->s.regs);
>> +}
>> +
>> +void test_clear_kvm_dirty_regs_bits(struct kvm_vm *vm, struct kvm_run *run)
>> +{
>> +	int rv;
>>   
>>   	/* Clear kvm_dirty_regs bits, verify new s.regs values are
>>   	 * overwritten with existing guest values.
>> @@ -200,8 +207,45 @@ int main(int argc, char *argv[])
>>   	TEST_ASSERT(run->s.regs.diag318 != 0x4B1D,
>>   		    "diag318 sync regs value incorrect 0x%llx.",
>>   		    run->s.regs.diag318);
>> +}
>> +
>> +struct testdef {
>> +	const char *name;
>> +	void (*test)(struct kvm_vm *vm, struct kvm_run *run);
>> +} 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 },
>> +};
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	static struct kvm_run *run;
>> +	static struct kvm_vm *vm;
>> +	int idx;
>> +
>> +	/* Tell stdout not to buffer its content */
>> +	setbuf(stdout, NULL);
>> +
>> +	if (!kvm_check_cap(KVM_CAP_SYNC_REGS))
>> +		ksft_exit_skip("CAP_SYNC_REGS not supported");
> 
> I'm not an expert on the TAP format, but wouldn't it be more meaningful
> to print the header first? (like you do in the previous patch)

It shouldn't matter much, without the header, TAP version 12 will be used:

  https://testanything.org/tap-specification.html

With header, it switches to version 13:

  https://testanything.org/tap-version-13-specification.html

But the "1..0" lines (which signal a complete skip) are part of both 
versions, so we should be fine here.

(but I can also move it in case I have to respin anyway)

  Thomas

Re: [PATCH 3/4] KVM: s390: selftests: Use TAP interface in the tprot test

[Thomas Huth]

On 14/04/2022 13.51, Claudio Imbrenda wrote:
> On Thu, 14 Apr 2022 12:53:21 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> The tprot 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.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tools/testing/selftests/kvm/s390x/tprot.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
>> index c097b9db495e..a714b4206e95 100644
>> --- a/tools/testing/selftests/kvm/s390x/tprot.c
>> +++ b/tools/testing/selftests/kvm/s390x/tprot.c
>> @@ -8,6 +8,7 @@
>>   #include <sys/mman.h>
>>   #include "test_util.h"
>>   #include "kvm_util.h"
>> +#include "kselftest.h"
>>   
>>   #define PAGE_SHIFT 12
>>   #define PAGE_SIZE (1 << PAGE_SHIFT)
>> @@ -69,6 +70,7 @@ enum stage {
>>   	STAGE_INIT_FETCH_PROT_OVERRIDE,
>>   	TEST_FETCH_PROT_OVERRIDE,
>>   	TEST_STORAGE_PROT_OVERRIDE,
>> +	NUM_STAGES			/* this must be the last entry */
>>   };
>>   
>>   struct test {
>> @@ -196,6 +198,7 @@ static void guest_code(void)
>>   	}									\
>>   	ASSERT_EQ(uc.cmd, UCALL_SYNC);						\
>>   	ASSERT_EQ(uc.args[1], __stage);						\
>> +	ksft_test_result_pass("" #stage "\n");					\
>>   })
>>   
>>   int main(int argc, char *argv[])
>> @@ -204,6 +207,9 @@ int main(int argc, char *argv[])
>>   	struct kvm_run *run;
>>   	vm_vaddr_t guest_0_page;
>>   
>> +	ksft_print_header();
>> +	ksft_set_plan(NUM_STAGES - 1);	/* STAGE_END is not counted, thus - 1 */
>> +
>>   	vm = vm_create_default(VCPU_ID, 0, guest_code);
>>   	run = vcpu_state(vm, VCPU_ID);
>>   
>> @@ -213,7 +219,7 @@ int main(int argc, char *argv[])
>>   
>>   	guest_0_page = vm_vaddr_alloc(vm, PAGE_SIZE, 0);
>>   	if (guest_0_page != 0)
>> -		print_skip("Did not allocate page at 0 for fetch protection override tests");
>> +		ksft_print_msg("Did not allocate page at 0 for fetch protection override tests\n");
> 
> will this print a skip, though?

No, it's now only a message.

> or you don't want to print a skip because then the numbering in the
> planning doesn't match anymore?

Right.

> in which case, is there an easy way to fix it?

Honestly, this part of the code is a little bit of a riddle to me - I wonder 
why this was using "print_skip()" at all, since the HOST_SYNC below is 
executed anyway... so this sounds rather like a warning message to me that 
says that the following test might not work as expected, instead of a real 
test-is-skipped message?

Janis, could you please clarify the intention here?

  Thomas

>>   	HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
>>   	if (guest_0_page == 0)
>>   		mprotect(addr_gva2hva(vm, (vm_vaddr_t)0), PAGE_SIZE, PROT_READ);
>> @@ -224,4 +230,8 @@ int main(int argc, char *argv[])
>>   	run->s.regs.crs[0] |= CR0_STORAGE_PROTECTION_OVERRIDE;
>>   	run->kvm_dirty_regs = KVM_SYNC_CRS;
>>   	HOST_SYNC(vm, TEST_STORAGE_PROT_OVERRIDE);
>> +
>> +	kvm_vm_free(vm);
>> +
>> +	ksft_finished();
>>   }
> 

Re: [PATCH 3/4] KVM: s390: selftests: Use TAP interface in the tprot test

[Janis Schoetterl-Glausch]

On 4/14/22 14:08, Thomas Huth wrote:
> On 14/04/2022 13.51, Claudio Imbrenda wrote:
>> On Thu, 14 Apr 2022 12:53:21 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> The tprot 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.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   tools/testing/selftests/kvm/s390x/tprot.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
>>> index c097b9db495e..a714b4206e95 100644
>>> --- a/tools/testing/selftests/kvm/s390x/tprot.c
>>> +++ b/tools/testing/selftests/kvm/s390x/tprot.c
>>> @@ -8,6 +8,7 @@
>>>   #include <sys/mman.h>
>>>   #include "test_util.h"
>>>   #include "kvm_util.h"
>>> +#include "kselftest.h"
>>>     #define PAGE_SHIFT 12
>>>   #define PAGE_SIZE (1 << PAGE_SHIFT)
>>> @@ -69,6 +70,7 @@ enum stage {
>>>       STAGE_INIT_FETCH_PROT_OVERRIDE,
>>>       TEST_FETCH_PROT_OVERRIDE,
>>>       TEST_STORAGE_PROT_OVERRIDE,
>>> +    NUM_STAGES            /* this must be the last entry */

You could move STAGE_END down and use that instead.

>>>   };
>>>     struct test {
>>> @@ -196,6 +198,7 @@ static void guest_code(void)
>>>       }                                    \
>>>       ASSERT_EQ(uc.cmd, UCALL_SYNC);                        \
>>>       ASSERT_EQ(uc.args[1], __stage);                        \
>>> +    ksft_test_result_pass("" #stage "\n");                    \
>>>   })
>>>     int main(int argc, char *argv[])
>>> @@ -204,6 +207,9 @@ int main(int argc, char *argv[])
>>>       struct kvm_run *run;
>>>       vm_vaddr_t guest_0_page;
>>>   +    ksft_print_header();
>>> +    ksft_set_plan(NUM_STAGES - 1);    /* STAGE_END is not counted, thus - 1 */
>>> +
>>>       vm = vm_create_default(VCPU_ID, 0, guest_code);
>>>       run = vcpu_state(vm, VCPU_ID);
>>>   @@ -213,7 +219,7 @@ int main(int argc, char *argv[])
>>>         guest_0_page = vm_vaddr_alloc(vm, PAGE_SIZE, 0);
>>>       if (guest_0_page != 0)
>>> -        print_skip("Did not allocate page at 0 for fetch protection override tests");
>>> +        ksft_print_msg("Did not allocate page at 0 for fetch protection override tests\n");
>>
>> will this print a skip, though?
> 
> No, it's now only a message.
> 
>> or you don't want to print a skip because then the numbering in the
>> planning doesn't match anymore?
> 
> Right.
> 
>> in which case, is there an easy way to fix it?
> 
> Honestly, this part of the code is a little bit of a riddle to me - I wonder why this was using "print_skip()" at all, since the HOST_SYNC below is executed anyway... so this sounds rather like a warning message to me that says that the following test might not work as expected, instead of a real test-is-skipped message?
> 
> Janis, could you please clarify the intention here?

Both the host and the guest check the same condition independently, the host just to print the message,
then the guest is run and skips those stages.
> 
>  Thomas
> 
>>>       HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
>>>       if (guest_0_page == 0)
>>>           mprotect(addr_gva2hva(vm, (vm_vaddr_t)0), PAGE_SIZE, PROT_READ);
>>> @@ -224,4 +230,8 @@ int main(int argc, char *argv[])
>>>       run->s.regs.crs[0] |= CR0_STORAGE_PROTECTION_OVERRIDE;
>>>       run->kvm_dirty_regs = KVM_SYNC_CRS;
>>>       HOST_SYNC(vm, TEST_STORAGE_PROT_OVERRIDE);
>>> +
>>> +    kvm_vm_free(vm);
>>> +
>>> +    ksft_finished();
>>>   }
>>
> 

Re: [PATCH 1/4] KVM: s390: selftests: Use TAP interface in the memop test

[Janis Schoetterl-Glausch]

On 4/14/22 12:53, Thomas Huth wrote:
> The memop 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tools/testing/selftests/kvm/s390x/memop.c | 90 ++++++++++++++++++-----
>  1 file changed, 73 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
> index b04c2c1b3c30..a2783d9afcac 100644
> --- a/tools/testing/selftests/kvm/s390x/memop.c
> +++ b/tools/testing/selftests/kvm/s390x/memop.c
> @@ -12,6 +12,7 @@
>  
>  #include "test_util.h"
>  #include "kvm_util.h"
> +#include "kselftest.h"
>  
>  enum mop_target {
>  	LOGICAL,
> @@ -648,33 +649,88 @@ static void test_errors(void)
>  	kvm_vm_free(t.kvm_vm);
>  }
>  
> +struct testdef {
> +	const char *name;
> +	void (*test)(void);
> +	bool needs_extension;

Please make this numeric. You could also rename it to required_extension or similar.

> +} testlist[] = {
> +	{
> +		.name = "simple copy",
> +		.test = test_copy,
> +		.needs_extension = false,
> +	},
> +	{
> +		.name = "copy with storage keys",
> +		.test = test_copy_key,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "copy with key storage protection override",
> +		.test = test_copy_key_storage_prot_override,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "copy with key fetch protection",
> +		.test = test_copy_key_fetch_prot,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "copy with key fetch protection override",
> +		.test = test_copy_key_fetch_prot_override,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "error checks with key",
> +		.test = test_errors_key,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "error checks with key storage protection override",
> +		.test = test_errors_key_storage_prot_override,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "error checks without key fetch prot override",
> +		.test = test_errors_key_fetch_prot_override_not_enabled,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "error checks with key fetch prot override",
> +		.test = test_errors_key_fetch_prot_override_enabled,
> +		.needs_extension = true,
> +	},
> +	{
> +		.name = "generic error checks",
> +		.test = test_errors,
> +		.needs_extension = false,
> +	},
> +};
> +
>  int main(int argc, char *argv[])
>  {
> -	int memop_cap, extension_cap;
> +	int memop_cap, extension_cap, idx;
>  
>  	setbuf(stdout, NULL);	/* Tell stdout not to buffer its content */
>  
> +	ksft_print_header();
> +
>  	memop_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP);
>  	extension_cap = kvm_check_cap(KVM_CAP_S390_MEM_OP_EXTENSION);
>  	if (!memop_cap) {
> -		print_skip("CAP_S390_MEM_OP not supported");
> -		exit(KSFT_SKIP);
> +		ksft_exit_skip("CAP_S390_MEM_OP not supported.\n");
>  	}
>  
> -	test_copy();
> -	if (extension_cap > 0) {
> -		test_copy_key();
> -		test_copy_key_storage_prot_override();
> -		test_copy_key_fetch_prot();
> -		test_copy_key_fetch_prot_override();
> -		test_errors_key();
> -		test_errors_key_storage_prot_override();
> -		test_errors_key_fetch_prot_override_not_enabled();
> -		test_errors_key_fetch_prot_override_enabled();
> -	} else {
> -		print_skip("storage key memop extension not supported");
> +	ksft_set_plan(ARRAY_SIZE(testlist));
> +
> +	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
> +		if (!testlist[idx].needs_extension || extension_cap) {

Then check here that extension_cap >= the required extension.
This way the test can easily be adapted in case of future extensions.
> +			testlist[idx].test();
> +			ksft_test_result_pass("%s\n", testlist[idx].name);
> +		} else {
> +			ksft_test_result_skip("%s - storage key memop not supported\n",
> +					      testlist[idx].name);
> +		}
>  	}
> -	test_errors();
>  
> -	return 0;
> +	ksft_finished();
>  }

Re: [PATCH 1/4] KVM: s390: selftests: Use TAP interface in the memop test

[Thomas Huth]

On 14/04/2022 14.48, Janis Schoetterl-Glausch wrote:
> On 4/14/22 12:53, Thomas Huth wrote:
>> The memop 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.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tools/testing/selftests/kvm/s390x/memop.c | 90 ++++++++++++++++++-----
>>   1 file changed, 73 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
>> index b04c2c1b3c30..a2783d9afcac 100644
>> --- a/tools/testing/selftests/kvm/s390x/memop.c
>> +++ b/tools/testing/selftests/kvm/s390x/memop.c
>> @@ -12,6 +12,7 @@
>>   
>>   #include "test_util.h"
>>   #include "kvm_util.h"
>> +#include "kselftest.h"
>>   
>>   enum mop_target {
>>   	LOGICAL,
>> @@ -648,33 +649,88 @@ static void test_errors(void)
>>   	kvm_vm_free(t.kvm_vm);
>>   }
>>   
>> +struct testdef {
>> +	const char *name;
>> +	void (*test)(void);
>> +	bool needs_extension;
> 
> Please make this numeric. You could also rename it to required_extension or similar.
[...]
>> +
>> +	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
>> +		if (!testlist[idx].needs_extension || extension_cap) {
> 
> Then check here that extension_cap >= the required extension.
> This way the test can easily be adapted in case of future extensions.

Not sure whether a ">=" will really be safe, since a future extension does 
not necessarily assert that previous extensions are available at the same time.

But I can still turn the bool into a numeric to make it a little bit more 
flexible for future use.

  Thomas

Re: [PATCH 3/4] KVM: s390: selftests: Use TAP interface in the tprot test

[Thomas Huth]

On 14/04/2022 14.33, Janis Schoetterl-Glausch wrote:
> On 4/14/22 14:08, Thomas Huth wrote:
>> On 14/04/2022 13.51, Claudio Imbrenda wrote:
>>> On Thu, 14 Apr 2022 12:53:21 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> The tprot 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.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    tools/testing/selftests/kvm/s390x/tprot.c | 12 +++++++++++-
>>>>    1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c
>>>> index c097b9db495e..a714b4206e95 100644
>>>> --- a/tools/testing/selftests/kvm/s390x/tprot.c
>>>> +++ b/tools/testing/selftests/kvm/s390x/tprot.c
>>>> @@ -8,6 +8,7 @@
>>>>    #include <sys/mman.h>
>>>>    #include "test_util.h"
>>>>    #include "kvm_util.h"
>>>> +#include "kselftest.h"
>>>>      #define PAGE_SHIFT 12
>>>>    #define PAGE_SIZE (1 << PAGE_SHIFT)
>>>> @@ -69,6 +70,7 @@ enum stage {
>>>>        STAGE_INIT_FETCH_PROT_OVERRIDE,
>>>>        TEST_FETCH_PROT_OVERRIDE,
>>>>        TEST_STORAGE_PROT_OVERRIDE,
>>>> +    NUM_STAGES            /* this must be the last entry */
> 
> You could move STAGE_END down and use that instead.
> 
>>>>    };
>>>>      struct test {
>>>> @@ -196,6 +198,7 @@ static void guest_code(void)
>>>>        }                                    \
>>>>        ASSERT_EQ(uc.cmd, UCALL_SYNC);                        \
>>>>        ASSERT_EQ(uc.args[1], __stage);                        \
>>>> +    ksft_test_result_pass("" #stage "\n");                    \
>>>>    })
>>>>      int main(int argc, char *argv[])
>>>> @@ -204,6 +207,9 @@ int main(int argc, char *argv[])
>>>>        struct kvm_run *run;
>>>>        vm_vaddr_t guest_0_page;
>>>>    +    ksft_print_header();
>>>> +    ksft_set_plan(NUM_STAGES - 1);    /* STAGE_END is not counted, thus - 1 */
>>>> +
>>>>        vm = vm_create_default(VCPU_ID, 0, guest_code);
>>>>        run = vcpu_state(vm, VCPU_ID);
>>>>    @@ -213,7 +219,7 @@ int main(int argc, char *argv[])
>>>>          guest_0_page = vm_vaddr_alloc(vm, PAGE_SIZE, 0);
>>>>        if (guest_0_page != 0)
>>>> -        print_skip("Did not allocate page at 0 for fetch protection override tests");
>>>> +        ksft_print_msg("Did not allocate page at 0 for fetch protection override tests\n");
>>>
>>> will this print a skip, though?
>>
>> No, it's now only a message.
>>
>>> or you don't want to print a skip because then the numbering in the
>>> planning doesn't match anymore?
>>
>> Right.
>>
>>> in which case, is there an easy way to fix it?
>>
>> Honestly, this part of the code is a little bit of a riddle to me - I wonder why this was using "print_skip()" at all, since the HOST_SYNC below is executed anyway... so this sounds rather like a warning message to me that says that the following test might not work as expected, instead of a real test-is-skipped message?
>>
>> Janis, could you please clarify the intention here?
> 
> Both the host and the guest check the same condition independently, the host just to print the message,
> then the guest is run and skips those stages.

Ok.

However, I'm not sure how to make this use ksft_test_result_skip() in a nice 
way now, though, without makeing the macro way uglier ...
I'll have a try, but if that does not work out I'd suggest to simply keep 
the ksft_print_msg() here instead.

  Thomas

Re: [PATCH 1/4] KVM: s390: selftests: Use TAP interface in the memop test

[Janis Schoetterl-Glausch]

On 4/19/22 19:40, Thomas Huth wrote:
> On 14/04/2022 14.48, Janis Schoetterl-Glausch wrote:
>> On 4/14/22 12:53, Thomas Huth wrote:
>>> The memop 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.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   tools/testing/selftests/kvm/s390x/memop.c | 90 ++++++++++++++++++-----
>>>   1 file changed, 73 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c
>>> index b04c2c1b3c30..a2783d9afcac 100644
>>> --- a/tools/testing/selftests/kvm/s390x/memop.c
>>> +++ b/tools/testing/selftests/kvm/s390x/memop.c
>>> @@ -12,6 +12,7 @@
>>>     #include "test_util.h"
>>>   #include "kvm_util.h"
>>> +#include "kselftest.h"
>>>     enum mop_target {
>>>       LOGICAL,
>>> @@ -648,33 +649,88 @@ static void test_errors(void)
>>>       kvm_vm_free(t.kvm_vm);
>>>   }
>>>   +struct testdef {
>>> +    const char *name;
>>> +    void (*test)(void);
>>> +    bool needs_extension;
>>
>> Please make this numeric. You could also rename it to required_extension or similar.
> [...]
>>> +
>>> +    for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
>>> +        if (!testlist[idx].needs_extension || extension_cap) {
>>
>> Then check here that extension_cap >= the required extension.
>> This way the test can easily be adapted in case of future extensions.
> 
> Not sure whether a ">=" will really be safe, since a future extension does not necessarily assert that previous extensions are available at the same time.

Hmm, I intend for that to hold. In any case, for the existing extension we have committed to it, e.g.
the documentation says:

Absolute accesses are permitted for the VM ioctl if KVM_CAP_S390_MEM_OP_EXTENSION
is > 0.

So, if we introduce an extension and allow for it to be removed with a higher extension number,
when we add testing support for that extension we'd have to change the capability check,
but the existing test case would not break.

I guess the most flexible way would be to initialize the array in the middle of main, then
you could do .skip = !extension_cap and in the future whatever expression makes sense, but
it's kinda ugly and should not be necessary anyway.
> 
> But I can still turn the bool into a numeric to make it a little bit more flexible for future use.
> 
>  Thomas
>