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