This patch series is motivated by Shuah's suggestion here:
https://lore.kernel.org/kvm/[email protected]/
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(-)
--
2.27.0
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 <[email protected]>
---
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();
}
--
2.27.0
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 <[email protected]>
---
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();
}
--
2.27.0
Let's standardize the s390x KVM selftest output to the TAP output
generated via the kselftests.h interface.
Signed-off-by: Thomas Huth <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Janosch Frank <[email protected]>
> ---
> 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 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 <[email protected]>
Reviewed-by: Janosch Frank <[email protected]>
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:
> Let's standardize the s390x KVM selftest output to the TAP output
> generated via the kselftests.h interface.
>
> Signed-off-by: Thomas Huth <[email protected]>
> ---
Reviewed-by: Janosch Frank <[email protected]>
[...]
> - 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 <[email protected]>
>> ---
>
> Reviewed-by: Janosch Frank <[email protected]>
>
> [...]
>> - 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