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