This patch series is motivated by Shuah's suggestion here:
https://lore.kernel.org/kvm/d576d8f7-980f-3bc6-87ad-5a6ae45609b8@linuxfounda...
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.
v2: - Reworked the extension checking in the first patch - Make sure to always print the TAP 13 header in the second patch - Reworked the SKIP printing in the third patch
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 | 87 +++++++++++++----- tools/testing/selftests/kvm/s390x/tprot.c | 28 ++++-- 4 files changed, 192 insertions(+), 51 deletions(-)
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..ad9fe86c0592 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); + int cap; +} testlist[] = { + { + .name = "simple copy", + .test = test_copy, + }, + { + .name = "generic error checks", + .test = test_errors, + }, + { + .name = "copy with storage keys", + .test = test_copy_key, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, + { + .name = "copy with key storage protection override", + .test = test_copy_key_storage_prot_override, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, + { + .name = "copy with key fetch protection", + .test = test_copy_key_fetch_prot, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, + { + .name = "copy with key fetch protection override", + .test = test_copy_key_fetch_prot_override, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, + { + .name = "error checks with key", + .test = test_errors_key, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, + { + .name = "error checks with key storage protection override", + .test = test_errors_key_storage_prot_override, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, + { + .name = "error checks without key fetch prot override", + .test = test_errors_key_fetch_prot_override_not_enabled, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, + { + .name = "error checks with key fetch prot override", + .test = test_errors_key_fetch_prot_override_enabled, + .cap = KVM_CAP_S390_MEM_OP_EXTENSION, + }, +}; + 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].cap || (extension_cap && + testlist[idx].cap == KVM_CAP_S390_MEM_OP_EXTENSION)) { + testlist[idx].test(); + ksft_test_result_pass("%s\n", testlist[idx].name); + } else { + ksft_test_result_skip("%s - capability %d not supported\n", + testlist[idx].name, + testlist[idx].cap); + } } - test_errors();
- return 0; + ksft_finished(); }
On 4/19/22 20:58, 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
Reviewed-by: Janosch Frank frankja@linux.ibm.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..ad9fe86c0592 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);
- int cap;
+} testlist[] = {
- {
.name = "simple copy",.test = test_copy,- },
- {
.name = "generic error checks",.test = test_errors,- },
- {
.name = "copy with storage keys",.test = test_copy_key,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
- {
.name = "copy with key storage protection override",.test = test_copy_key_storage_prot_override,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
- {
.name = "copy with key fetch protection",.test = test_copy_key_fetch_prot,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
- {
.name = "copy with key fetch protection override",.test = test_copy_key_fetch_prot_override,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
- {
.name = "error checks with key",.test = test_errors_key,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
- {
.name = "error checks with key storage protection override",.test = test_errors_key_storage_prot_override,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
- {
.name = "error checks without key fetch prot override",.test = test_errors_key_fetch_prot_override_not_enabled,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
- {
.name = "error checks with key fetch prot override",.test = test_errors_key_fetch_prot_override_enabled,.cap = KVM_CAP_S390_MEM_OP_EXTENSION,- },
+};
- 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].cap || (extension_cap &&testlist[idx].cap == KVM_CAP_S390_MEM_OP_EXTENSION)) {testlist[idx].test();ksft_test_result_pass("%s\n", testlist[idx].name);} else {ksft_test_result_skip("%s - capability %d not supported\n",testlist[idx].name,testlist[idx].cap);}
- test_errors();
- return 0;
- ksft_finished(); }
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 | 87 ++++++++++++++----- 1 file changed, 66 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..c3719c92f4e8 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,46 @@ 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); + + ksft_print_header(); + + if (!kvm_check_cap(KVM_CAP_SYNC_REGS)) + ksft_exit_skip("CAP_SYNC_REGS not supported"); + + ksft_set_plan(ARRAY_SIZE(testlist)); + + /* Create VM */ + vm = vm_create_default(VCPU_ID, 0, guest_code); + + run = vcpu_state(vm, VCPU_ID); + + 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(); }
On 4/19/22 20:58, Thomas Huth 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
Acked-by: Janosch Frank frankja@linux.ibm.com
.../selftests/kvm/s390x/sync_regs_test.c | 87 ++++++++++++++----- 1 file changed, 66 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..c3719c92f4e8 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,46 @@ 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);
- ksft_print_header();
- if (!kvm_check_cap(KVM_CAP_SYNC_REGS))
ksft_exit_skip("CAP_SYNC_REGS not supported");- ksft_set_plan(ARRAY_SIZE(testlist));
- /* Create VM */
- vm = vm_create_default(VCPU_ID, 0, guest_code);
- run = vcpu_state(vm, VCPU_ID);
- 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(); }
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 | 28 +++++++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c index c097b9db495e..baba883d7a6d 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) @@ -63,12 +64,12 @@ static enum permission test_protection(void *addr, uint8_t key) }
enum stage { - STAGE_END, STAGE_INIT_SIMPLE, TEST_SIMPLE, STAGE_INIT_FETCH_PROT_OVERRIDE, TEST_FETCH_PROT_OVERRIDE, TEST_STORAGE_PROT_OVERRIDE, + STAGE_END /* this must be the last entry */ };
struct test { @@ -182,7 +183,7 @@ static void guest_code(void) GUEST_SYNC(perform_next_stage(&i, mapped_0)); }
-#define HOST_SYNC(vmp, stage) \ +#define HOST_SYNC_NO_TAP(vmp, stage) \ ({ \ struct kvm_vm *__vm = (vmp); \ struct ucall uc; \ @@ -198,12 +199,21 @@ static void guest_code(void) ASSERT_EQ(uc.args[1], __stage); \ })
+#define HOST_SYNC(vmp, stage) \ +{ \ + HOST_SYNC_NO_TAP(vmp, stage); \ + ksft_test_result_pass("" #stage "\n"); \ +} + int main(int argc, char *argv[]) { struct kvm_vm *vm; struct kvm_run *run; vm_vaddr_t guest_0_page;
+ ksft_print_header(); + ksft_set_plan(STAGE_END); + vm = vm_create_default(VCPU_ID, 0, guest_code); run = vcpu_state(vm, VCPU_ID);
@@ -212,9 +222,13 @@ int main(int argc, char *argv[]) HOST_SYNC(vm, TEST_SIMPLE);
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"); - HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE); + if (guest_0_page != 0) { + HOST_SYNC_NO_TAP(vm, STAGE_INIT_FETCH_PROT_OVERRIDE); + ksft_test_result_skip("STAGE_INIT_FETCH_PROT_OVERRIDE - " + "Did not allocate page at 0\n"); + } else { + 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); run->s.regs.crs[0] |= CR0_FETCH_PROTECTION_OVERRIDE; @@ -224,4 +238,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(); }
On 4/19/22 20:58, Thomas Huth 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
Reviewed-by: Janosch Frank frankja@linux.ibm.com
Some comments below.
tools/testing/selftests/kvm/s390x/tprot.c | 28 +++++++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c index c097b9db495e..baba883d7a6d 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) @@ -63,12 +64,12 @@ static enum permission test_protection(void *addr, uint8_t key) } enum stage {
- STAGE_END, STAGE_INIT_SIMPLE, TEST_SIMPLE, STAGE_INIT_FETCH_PROT_OVERRIDE, TEST_FETCH_PROT_OVERRIDE, TEST_STORAGE_PROT_OVERRIDE,
- STAGE_END /* this must be the last entry */
...so we can use it to calculate the test number
}; struct test { @@ -182,7 +183,7 @@ static void guest_code(void) GUEST_SYNC(perform_next_stage(&i, mapped_0)); }
@@ -212,9 +222,13 @@ int main(int argc, char *argv[]) HOST_SYNC(vm, TEST_SIMPLE); 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");- HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);
- if (guest_0_page != 0) {
Maybe add: /* Use no_tap so we don't get a PASS print */
HOST_SYNC_NO_TAP(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);ksft_test_result_skip("STAGE_INIT_FETCH_PROT_OVERRIDE - ""Did not allocate page at 0\n");- } else {
HOST_SYNC(vm, STAGE_INIT_FETCH_PROT_OVERRIDE);- }
Otherwise this would look weird.
if (guest_0_page == 0) mprotect(addr_gva2hva(vm, (vm_vaddr_t)0), PAGE_SIZE, PROT_READ); run->s.regs.crs[0] |= CR0_FETCH_PROTECTION_OVERRIDE; @@ -224,4 +238,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(); }
On 4/19/22 20:58, Thomas Huth 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 | 28 +++++++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c index c097b9db495e..baba883d7a6d 100644 --- a/tools/testing/selftests/kvm/s390x/tprot.c +++ b/tools/testing/selftests/kvm/s390x/tprot.c
We're not committing ourselves to any particular test output, are we? Your patch considers the stages used for test setup tests themselves, which I'm fine with, but would not want to commit to keeping that way forever.
[...]
+#define HOST_SYNC(vmp, stage) \ +{ \
- HOST_SYNC_NO_TAP(vmp, stage); \
- ksft_test_result_pass("" #stage "\n"); \
+}
It should not be a problem, but is there any reason you're not using do { ... } while(0) or ({ ... }) instead of just braces?
[...]
On 20/04/2022 13.38, Janis Schoetterl-Glausch wrote:
On 4/19/22 20:58, Thomas Huth 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 | 28 +++++++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c index c097b9db495e..baba883d7a6d 100644 --- a/tools/testing/selftests/kvm/s390x/tprot.c +++ b/tools/testing/selftests/kvm/s390x/tprot.c
We're not committing ourselves to any particular test output, are we? Your patch considers the stages used for test setup tests themselves, which I'm fine with, but would not want to commit to keeping that way forever.
No commitment - just somewhat more verbose output. If you don't like it, we can also drop this patch, or do it in another way, I don't mind too much.
+#define HOST_SYNC(vmp, stage) \ +{ \
- HOST_SYNC_NO_TAP(vmp, stage); \
- ksft_test_result_pass("" #stage "\n"); \
+}
It should not be a problem, but is there any reason you're not using do { ... } while(0) or ({ ... }) instead of just braces?
Yes, that would be better, indeed.
Thomas
On 4/20/22 13:46, Thomas Huth wrote:
On 20/04/2022 13.38, Janis Schoetterl-Glausch wrote:
On 4/19/22 20:58, Thomas Huth 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 | 28 +++++++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kvm/s390x/tprot.c b/tools/testing/selftests/kvm/s390x/tprot.c index c097b9db495e..baba883d7a6d 100644 --- a/tools/testing/selftests/kvm/s390x/tprot.c +++ b/tools/testing/selftests/kvm/s390x/tprot.c
We're not committing ourselves to any particular test output, are we? Your patch considers the stages used for test setup tests themselves, which I'm fine with, but would not want to commit to keeping that way forever.
No commitment - just somewhat more verbose output. If you don't like it, we can also drop this patch, or do it in another way, I don't mind too much.
I'm fine with it then. With the braces changed:
Reviewed-by: Janis Schoetterl-Glausch scgl@linux.ibm.com
+#define HOST_SYNC(vmp, stage) \ +{ \ + HOST_SYNC_NO_TAP(vmp, stage); \ + ksft_test_result_pass("" #stage "\n"); \ +}
It should not be a problem, but is there any reason you're not using do { ... } while(0) or ({ ... }) instead of just braces?
Yes, that would be better, indeed.
Thomas
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(); }
On 4/19/22 20:58, Thomas Huth wrote:
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
Reviewed-by: Janosch Frank frankja@linux.ibm.com
[...]
- return 0;
- ksft_finished();
main() is still int so it looks really weird, that we remove the return here. After reading the ksft_finished() code I know that we never return because we do an exit() but I'd like to have a comment, change to void or noreturn tag to make this clearer.
I'd guess that's true for all 4 patches.
}
On 20/04/2022 12.34, Janosch Frank wrote:
On 4/19/22 20:58, Thomas Huth wrote:
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
Reviewed-by: Janosch Frank frankja@linux.ibm.com
[...]
- return 0;
+ ksft_finished();
main() is still int so it looks really weird, that we remove the return here. After reading the ksft_finished() code I know that we never return because we do an exit() but I'd like to have a comment, change to void or noreturn tag to make this clearer.
Changing the return type of main() to void causes a compiler warning here, so I'll go with the comment.
Thomas
linux-kselftest-mirror@lists.linaro.org
-
Janis Schoetterl-Glausch -
Janosch Frank -
Thomas Huth