2022-04-14 14:41:13

by Thomas Huth

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

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.

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


2022-04-14 17:01:51

by Thomas Huth

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

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

2022-04-14 19:39:58

by Thomas Huth

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

On 14/04/2022 13.51, Claudio Imbrenda wrote:
> On Thu, 14 Apr 2022 12:53:21 +0200
> Thomas Huth <[email protected]> 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]>
>> ---
>> 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();
>> }
>

2022-04-16 00:35:58

by Thomas Huth

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

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

2022-04-16 01:09:53

by Claudio Imbrenda

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

On Thu, 14 Apr 2022 12:53:21 +0200
Thomas Huth <[email protected]> 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]>
> ---
> 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();
> }

2022-04-16 01:37:53

by Janis Schoetterl-Glausch

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

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 <[email protected]> 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]>
>>> ---
>>>   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();
>>>   }
>>
>

2022-04-16 01:56:05

by Thomas Huth

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

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 <[email protected]>
---
.../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

2022-04-22 19:14:32

by Thomas Huth

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

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 <[email protected]> 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]>
>>>> ---
>>>>   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