diff mbox series

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

Message ID 20220419185857.128351-4-thuth@redhat.com (mailing list archive)
State New
Headers show
Series KVM: s390: selftests: Provide TAP output in tests | expand

Commit Message

Thomas Huth April 19, 2022, 6:58 p.m. UTC
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(-)

Comments

Janosch Frank April 20, 2022, 10:23 a.m. UTC | #1
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();
>   }
Janis Schoetterl-Glausch April 20, 2022, 11:38 a.m. UTC | #2
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?

[...]
Thomas Huth April 20, 2022, 11:46 a.m. UTC | #3
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
Janis Schoetterl-Glausch April 20, 2022, 12:06 p.m. UTC | #4
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
>
diff mbox series

Patch

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();
 }