| Message ID | 20191017030340.18301-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | selftests/x86/sgx: Improve tests | expand |
On Wed, Oct 16, 2019 at 08:03:26PM -0700, Sean Christopherson wrote: > The bulk of this series is comprised of the selftest portion of the vDSO > cleanup[*]. The big difference from the full vDSO series is to reuse as > much of the existing selftest code as possible. There is still a bit of > homebrew code in defining the low level assertion macro, but much less so > than in the previous from-scratch version. > > Cc'd Andy, who also happens to be a reviewer for the harness code, on the > off chance he has bandwidth to weigh in. > > Tagged for_v2? to make it clear that this doesn't need to be rushed into > v23. No need for harness right now. Open coded tests are better for initial upstreaming. /Jarkko
On Fri, Oct 18, 2019 at 01:12:52PM +0300, Jarkko Sakkinen wrote: > On Wed, Oct 16, 2019 at 08:03:26PM -0700, Sean Christopherson wrote: > > The bulk of this series is comprised of the selftest portion of the vDSO > > cleanup[*]. The big difference from the full vDSO series is to reuse as > > much of the existing selftest code as possible. There is still a bit of > > homebrew code in defining the low level assertion macro, but much less so > > than in the previous from-scratch version. > > > > Cc'd Andy, who also happens to be a reviewer for the harness code, on the > > off chance he has bandwidth to weigh in. > > > > Tagged for_v2? to make it clear that this doesn't need to be rushed into > > v23. > > No need for harness right now. Open coded tests are better for initial > upstreaming. Macros make code more productive to write but harder to read and debug. With only maybe three tests the cost of using them does not pay. /Jarkko
On Fri, Oct 18, 2019 at 01:20:59PM +0300, Jarkko Sakkinen wrote: > On Fri, Oct 18, 2019 at 01:12:52PM +0300, Jarkko Sakkinen wrote: > > On Wed, Oct 16, 2019 at 08:03:26PM -0700, Sean Christopherson wrote: > > > The bulk of this series is comprised of the selftest portion of the vDSO > > > cleanup[*]. The big difference from the full vDSO series is to reuse as > > > much of the existing selftest code as possible. There is still a bit of > > > homebrew code in defining the low level assertion macro, but much less so > > > than in the previous from-scratch version. > > > > > > Cc'd Andy, who also happens to be a reviewer for the harness code, on the > > > off chance he has bandwidth to weigh in. > > > > > > Tagged for_v2? to make it clear that this doesn't need to be rushed into > > > v23. > > > > No need for harness right now. Open coded tests are better for initial > > upstreaming. > > Macros make code more productive to write but harder to read and > debug. With only maybe three tests the cost of using them does > not pay. Harder to read is debatable, personally I find this static void test_sgx_basic(struct sgx_secs *secs) { uint64_t result = 0; sgx_call_eenter((void *)&MAGIC, &result, (void *)secs->base); EXPECT_EQ(result, MAGIC); } to be more intuitive than static void test_sgx_basic(struct sgx_secs *secs) { uint64_t result = 0; printf("Input: 0x%lx\n", MAGIC); sgx_call_eenter((void *)&MAGIC, &result, (void *)secs->base); if (result != MAGIC) { fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC); exit(1); } printf("Output: 0x%lx\n", result); } but to each his own. The debug argument I just don't buy. How is this $ ./test_sgx TAP version 13 1..4 not ok 1 Expected 'result (0) == MAGIC (1234605616436508552)' at main.c:325 ok 2 test_sgx_vdso: Passed ok 3 test_sgx_vdso_exit_handler: Passed ok 4 test_sgx_vdso_exception_handler: Passed # Pass 3 Fail 1 Xfail 0 Xpass 0 Skip 0 Error 0 harder to debug than this? $ ./test_sgx Input: 0x1122334455667788 0x0 != 0x1122334455667788 Except for the error status in the shell there's not even an explicit indicator that something went wrong, let alone any information about what test was being run, what exact check failed, etc...
On Tue, Oct 22, 2019 at 03:41:58PM -0700, Sean Christopherson wrote: > On Fri, Oct 18, 2019 at 01:20:59PM +0300, Jarkko Sakkinen wrote: > > On Fri, Oct 18, 2019 at 01:12:52PM +0300, Jarkko Sakkinen wrote: > > > On Wed, Oct 16, 2019 at 08:03:26PM -0700, Sean Christopherson wrote: > > > > The bulk of this series is comprised of the selftest portion of the vDSO > > > > cleanup[*]. The big difference from the full vDSO series is to reuse as > > > > much of the existing selftest code as possible. There is still a bit of > > > > homebrew code in defining the low level assertion macro, but much less so > > > > than in the previous from-scratch version. > > > > > > > > Cc'd Andy, who also happens to be a reviewer for the harness code, on the > > > > off chance he has bandwidth to weigh in. > > > > > > > > Tagged for_v2? to make it clear that this doesn't need to be rushed into > > > > v23. > > > > > > No need for harness right now. Open coded tests are better for initial > > > upstreaming. > > > > Macros make code more productive to write but harder to read and > > debug. With only maybe three tests the cost of using them does > > not pay. > > Harder to read is debatable, personally I find this > > static void test_sgx_basic(struct sgx_secs *secs) > { > uint64_t result = 0; > > sgx_call_eenter((void *)&MAGIC, &result, (void *)secs->base); > EXPECT_EQ(result, MAGIC); > } > > to be more intuitive than > > static void test_sgx_basic(struct sgx_secs *secs) > { > uint64_t result = 0; > > printf("Input: 0x%lx\n", MAGIC); > > sgx_call_eenter((void *)&MAGIC, &result, (void *)secs->base); > if (result != MAGIC) { > fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC); > exit(1); > } > > printf("Output: 0x%lx\n", result); > } > > but to each his own. > > The debug argument I just don't buy. How is this > > $ ./test_sgx > TAP version 13 > 1..4 > not ok 1 Expected 'result (0) == MAGIC (1234605616436508552)' at main.c:325 > ok 2 test_sgx_vdso: Passed > ok 3 test_sgx_vdso_exit_handler: Passed > ok 4 test_sgx_vdso_exception_handler: Passed > # Pass 3 Fail 1 Xfail 0 Xpass 0 Skip 0 Error 0 > > > harder to debug than this? > > $ ./test_sgx > Input: 0x1122334455667788 > 0x0 != 0x1122334455667788 > > Except for the error status in the shell there's not even an explicit > indicator that something went wrong, let alone any information about > what test was being run, what exact check failed, etc... Lets consider this post upstreaming. For me it comes in the end not to add new twists to the patch set unless there is life and death reason to do so. /Jarkko
On Wed, Oct 23, 2019 at 5:39 AM Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > On Tue, Oct 22, 2019 at 03:41:58PM -0700, Sean Christopherson wrote: > > On Fri, Oct 18, 2019 at 01:20:59PM +0300, Jarkko Sakkinen wrote: > > > On Fri, Oct 18, 2019 at 01:12:52PM +0300, Jarkko Sakkinen wrote: > > > > On Wed, Oct 16, 2019 at 08:03:26PM -0700, Sean Christopherson wrote: > > > > > The bulk of this series is comprised of the selftest portion of the vDSO > > > > > cleanup[*]. The big difference from the full vDSO series is to reuse as > > > > > much of the existing selftest code as possible. There is still a bit of > > > > > homebrew code in defining the low level assertion macro, but much less so > > > > > than in the previous from-scratch version. > > > > > > > > > > Cc'd Andy, who also happens to be a reviewer for the harness code, on the > > > > > off chance he has bandwidth to weigh in. > > > > > > > > > > Tagged for_v2? to make it clear that this doesn't need to be rushed into > > > > > v23. > > > > > > > > No need for harness right now. Open coded tests are better for initial > > > > upstreaming. > > > > > > Macros make code more productive to write but harder to read and > > > debug. With only maybe three tests the cost of using them does > > > not pay. > > > > Harder to read is debatable, personally I find this > > > > static void test_sgx_basic(struct sgx_secs *secs) > > { > > uint64_t result = 0; > > > > sgx_call_eenter((void *)&MAGIC, &result, (void *)secs->base); > > EXPECT_EQ(result, MAGIC); > > } > > > > to be more intuitive than > > > > static void test_sgx_basic(struct sgx_secs *secs) > > { > > uint64_t result = 0; > > > > printf("Input: 0x%lx\n", MAGIC); > > > > sgx_call_eenter((void *)&MAGIC, &result, (void *)secs->base); > > if (result != MAGIC) { > > fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC); > > exit(1); > > } > > > > printf("Output: 0x%lx\n", result); > > } > > > > but to each his own. > > > > The debug argument I just don't buy. How is this > > > > $ ./test_sgx > > TAP version 13 > > 1..4 > > not ok 1 Expected 'result (0) == MAGIC (1234605616436508552)' at main.c:325 > > ok 2 test_sgx_vdso: Passed > > ok 3 test_sgx_vdso_exit_handler: Passed > > ok 4 test_sgx_vdso_exception_handler: Passed > > # Pass 3 Fail 1 Xfail 0 Xpass 0 Skip 0 Error 0 > > > > > > harder to debug than this? > > > > $ ./test_sgx > > Input: 0x1122334455667788 > > 0x0 != 0x1122334455667788 > > > > Except for the error status in the shell there's not even an explicit > > indicator that something went wrong, let alone any information about > > what test was being run, what exact check failed, etc... > > Lets consider this post upstreaming. For me it comes in the end > not to add new twists to the patch set unless there is life and > death reason to do so. > I tend to agree. For what it's worth, the main reason that I don't use any test harness in most of the x86 selftests is that I wrote them so early in the history of kselftests that there wasn't any infrastructure. I haven't looked to see what the best practice is now.
The bulk of this series is comprised of the selftest portion of the vDSO cleanup[*]. The big difference from the full vDSO series is to reuse as much of the existing selftest code as possible. There is still a bit of homebrew code in defining the low level assertion macro, but much less so than in the previous from-scratch version. Cc'd Andy, who also happens to be a reviewer for the harness code, on the off chance he has bandwidth to weigh in. Tagged for_v2? to make it clear that this doesn't need to be rushed into v23. Passing case: TAP version 13 1..4 ok 1 test_sgx_basic: Passed ok 2 test_sgx_vdso: Passed ok 3 test_sgx_vdso_exit_handler: Passed ok 4 test_sgx_vdso_exception_handler: Passed # Pass 4 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0 Failing case: TAP version 13 1..4 not ok 1 Expected 'result (1234605616436508552) != MAGIC (1234605616436508552)' at main.c:324 ok 2 test_sgx_vdso: Passed ok 3 test_sgx_vdso_exit_handler: Passed ok 4 test_sgx_vdso_exception_handler: Passed # Pass 3 Fail 1 Xfail 0 Xpass 0 Skip 0 Error 0 [*] https://patchwork.kernel.org/cover/11178771/ Sean Christopherson (14): selftests/x86/sgx: Fix a benign linker warning selftests/x86/sgx: Use getauxval() to retrieve the vDSO base address selftests/x86/sgx: Sanitize the types for sgx_vdso_call()'s input params selftests/x86/sgx: Mark helper functions as static selftests/x86/sgx: Move vDSO setup to a helper function selftests/x86/sgx: Move individual tests into helper functions selftests/x86/sgx: Use standard helper function to signal pass/fail selftests/harness: Move operator macros to their own header file selftests/x86/sgx: Use kselftest operators to check test results selftests/x86/sgx: Handle setup failures via kselftest assertions selftests/x86/sgx: Add a check on the vDSO exception reporting mechanism selftests/x86/sgx: Add test of vDSO with basic exit handler selftests/x86/sgx: Add check to verify exit handler stack alignment selftests/x86/sgx: Add test for exception behavior with exit handler Documentation/dev-tools/kselftest.rst | 9 +- MAINTAINERS | 1 + tools/testing/selftests/kselftest_harness.h | 246 +--------- tools/testing/selftests/kselftest_operators.h | 255 +++++++++++ tools/testing/selftests/x86/sgx/Makefile | 2 +- tools/testing/selftests/x86/sgx/defines.h | 7 + tools/testing/selftests/x86/sgx/main.c | 426 +++++++++++------- tools/testing/selftests/x86/sgx/sgx_call.h | 3 +- 8 files changed, 541 insertions(+), 408 deletions(-) create mode 100644 tools/testing/selftests/kselftest_operators.h