[for_v2?,v2,00/14] selftests/x86/sgx: Improve tests
mbox series

Message ID 20191017030340.18301-1-sean.j.christopherson@intel.com
Headers show
Series
  • selftests/x86/sgx: Improve tests
Related show

Message

Sean Christopherson Oct. 17, 2019, 3:03 a.m. UTC
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

Comments

Jarkko Sakkinen Oct. 18, 2019, 10:12 a.m. UTC | #1
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
Jarkko Sakkinen Oct. 18, 2019, 10:20 a.m. UTC | #2
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
Sean Christopherson Oct. 22, 2019, 10:41 p.m. UTC | #3
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...
Jarkko Sakkinen Oct. 23, 2019, 12:39 p.m. UTC | #4
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
Andy Lutomirski Oct. 26, 2019, 2:08 p.m. UTC | #5
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.