[PATCH for_v2? v2 00/14] selftests/x86/sgx: Improve tests

[PATCH for_v2? v2 00/14] selftests/x86/sgx: Improve tests

[Sean Christopherson]

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

-- 
2.22.0

[PATCH for_v2? v2 01/14] selftests/x86/sgx: Fix a benign linker warning

[Sean Christopherson]

Pass a build id of "none" to the linker to suppress a warning about the
build id being ignored:

  /usr/bin/ld: warning: .note.gnu.build-id section discarded, --build-id
  ignored.

Co-developed-by: Cedric Xing <cedric.xing@intel.com>
Signed-off-by: Cedric Xing <cedric.xing@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sgx/Makefile b/tools/testing/selftests/x86/sgx/Makefile
index a09ef5f965dc..90da0de41504 100644
--- a/tools/testing/selftests/x86/sgx/Makefile
+++ b/tools/testing/selftests/x86/sgx/Makefile
@@ -27,7 +27,7 @@ $(OUTPUT)/encl.bin: $(OUTPUT)/encl.elf $(OUTPUT)/sgxsign
 	$(OBJCOPY) -O binary $< $@
 
 $(OUTPUT)/encl.elf: encl.lds encl.c encl_bootstrap.S
-	$(CC) $(ENCL_CFLAGS) -T $^ -o $@
+	$(CC) $(ENCL_CFLAGS) -T $^ -o $@ -Wl,--build-id=none
 
 $(OUTPUT)/encl.ss: $(OUTPUT)/encl.bin
 	$(OUTPUT)/sgxsign signing_key.pem $(OUTPUT)/encl.bin $(OUTPUT)/encl.ss
-- 
2.22.0

[PATCH for_v2? v2 02/14] selftests/x86/sgx: Use getauxval() to retrieve the vDSO base address

[Sean Christopherson]

Replace the open coded ELF fun with a simple getauxval() call.

Suggested-by: Cedric Xing <cedric.xing@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 74b3b1aa1f8c..ad284539d418 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -15,6 +15,8 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/types.h>
+#include <sys/auxv.h>
+
 #include "defines.h"
 #include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
 #include "../../../../../arch/x86/include/uapi/asm/sgx.h"
@@ -31,22 +33,9 @@ struct vdso_symtab {
 	Elf64_Word *elf_hashtab;
 };
 
-static void *vdso_get_base_addr(char *envp[])
+static void *vdso_get_base_addr(void)
 {
-	Elf64_auxv_t *auxv;
-	int i;
-
-	for (i = 0; envp[i]; i++)
-		;
-
-	auxv = (Elf64_auxv_t *)&envp[i + 1];
-
-	for (i = 0; auxv[i].a_type != AT_NULL; i++) {
-		if (auxv[i].a_type == AT_SYSINFO_EHDR)
-			return (void *)auxv[i].a_un.a_val;
-	}
-
-	return NULL;
+	return (void *)getauxval(AT_SYSINFO_EHDR);
 }
 
 static Elf64_Dyn *vdso_get_dyntab(void *addr)
@@ -353,7 +342,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	memset(&exception, 0, sizeof(exception));
 
-	addr = vdso_get_base_addr(envp);
+	addr = vdso_get_base_addr();
 	if (!addr)
 		exit(1);
 
-- 
2.22.0

[PATCH for_v2? v2 03/14] selftests/x86/sgx: Sanitize the types for sgx_vdso_call()'s input params

[Sean Christopherson]

Convert the @rdx parameter for sgx_vdso_call() from 'long' to 'void *'
to make it consistent with all other register params.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c     | 2 +-
 tools/testing/selftests/x86/sgx/sgx_call.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index ad284539d418..fa93521ed4d3 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -356,7 +356,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	printf("Input: 0x%lx\n", MAGIC);
 
-	sgx_call_vdso((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
+	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
 		      (void *)secs.base, &exception, NULL);
 	if (result != MAGIC) {
 		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
diff --git a/tools/testing/selftests/x86/sgx/sgx_call.h b/tools/testing/selftests/x86/sgx/sgx_call.h
index a4072c5ecce7..2bf239aff0a8 100644
--- a/tools/testing/selftests/x86/sgx/sgx_call.h
+++ b/tools/testing/selftests/x86/sgx/sgx_call.h
@@ -8,7 +8,8 @@
 
 void sgx_call_eenter(void *rdi, void *rsi, void *entry);
 
-int sgx_call_vdso(void *rdi, void *rsi, long rdx, void *rcx, void *r8, void *r9,
+
+int sgx_call_vdso(void *rdi, void *rsi, void *rdx, void *rcx, void *r8, void *r9,
 		  void *tcs, struct sgx_enclave_exception *ei, void *cb);
 
 #endif /* SGX_CALL_H */
-- 
2.22.0

[PATCH for_v2? v2 04/14] selftests/x86/sgx: Mark helper functions as static

[Sean Christopherson]

Tag a handful of local helper functions as static.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index fa93521ed4d3..e87e445f6de1 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -241,7 +241,7 @@ static bool encl_build(struct sgx_secs *secs, void *bin,
 	return false;
 }
 
-bool get_file_size(const char *path, off_t *bin_size)
+static bool get_file_size(const char *path, off_t *bin_size)
 {
 	struct stat sb;
 	int ret;
@@ -261,7 +261,7 @@ bool get_file_size(const char *path, off_t *bin_size)
 	return true;
 }
 
-bool encl_data_map(const char *path, void **bin, off_t *bin_size)
+static bool encl_data_map(const char *path, void **bin, off_t *bin_size)
 {
 	int fd;
 
@@ -288,7 +288,7 @@ bool encl_data_map(const char *path, void **bin, off_t *bin_size)
 	return false;
 }
 
-bool load_sigstruct(const char *path, void *sigstruct)
+static bool load_sigstruct(const char *path, void *sigstruct)
 {
 	int fd;
 
-- 
2.22.0

[PATCH for_v2? v2 05/14] selftests/x86/sgx: Move vDSO setup to a helper function

[Sean Christopherson]

Move the vDSO setup code to a helper function so that it is more obvious
that it's run-once setup, and that setting eenter is the end goal of the
code.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 41 ++++++++++++++++----------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index e87e445f6de1..2510cacb5154 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -309,16 +309,36 @@ static bool load_sigstruct(const char *path, void *sigstruct)
 	return true;
 }
 
+static bool setup_vdso(void)
+{
+	struct vdso_symtab symtab;
+	Elf64_Sym *eenter_sym;
+	void *addr;
+
+	addr = vdso_get_base_addr();
+	if (!addr)
+		return false;
+
+	if (!vdso_get_symtab(addr, &symtab))
+		return false;
+
+	eenter_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave");
+	if (!eenter_sym)
+		return false;
+
+	/* eenter is used by sgx_call_vdso() to call into the vDSO. */
+	eenter = addr + eenter_sym->st_value;
+
+	return true;
+}
+
 int main(int argc, char *argv[], char *envp[])
 {
 	struct sgx_enclave_exception exception;
 	struct sgx_sigstruct sigstruct;
-	struct vdso_symtab symtab;
-	Elf64_Sym *eenter_sym;
 	struct sgx_secs secs;
 	uint64_t result = 0;
 	off_t bin_size;
-	void *addr;
 	void *bin;
 
 	if (!encl_data_map("encl.bin", &bin, &bin_size))
@@ -340,20 +360,11 @@ int main(int argc, char *argv[], char *envp[])
 
 	printf("Output: 0x%lx\n", result);
 
+	if (!setup_vdso())
+		exit(1);
+
 	memset(&exception, 0, sizeof(exception));
 
-	addr = vdso_get_base_addr();
-	if (!addr)
-		exit(1);
-
-	if (!vdso_get_symtab(addr, &symtab))
-		exit(1);
-
-	eenter_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave");
-	if (!eenter_sym)
-		exit(1);
-	eenter = addr + eenter_sym->st_value;
-
 	printf("Input: 0x%lx\n", MAGIC);
 
 	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
-- 
2.22.0

[PATCH for_v2? v2 06/14] selftests/x86/sgx: Move individual tests into helper functions

[Sean Christopherson]

Move the basic and vDSO tests to their own helper functions to improve
readability and prepare for using the kselftest helpers to signal
pass/fail.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 59 ++++++++++++++++----------
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 2510cacb5154..140dffe9c765 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -332,12 +332,44 @@ static bool setup_vdso(void)
 	return true;
 }
 
-int main(int argc, char *argv[], char *envp[])
+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);
+}
+
+static void test_sgx_vdso(struct sgx_secs *secs)
 {
 	struct sgx_enclave_exception exception;
+	uint64_t result = 0;
+
+	memset(&exception, 0, sizeof(exception));
+
+	printf("Input: 0x%lx\n", MAGIC);
+
+	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
+		      (void *)secs->base, &exception, NULL);
+	if (result != MAGIC) {
+		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
+		exit(1);
+	}
+
+	printf("Output: 0x%lx\n", result);
+}
+
+int main(int argc, char *argv[], char *envp[])
+{
 	struct sgx_sigstruct sigstruct;
 	struct sgx_secs secs;
-	uint64_t result = 0;
 	off_t bin_size;
 	void *bin;
 
@@ -350,31 +382,12 @@ int main(int argc, char *argv[], char *envp[])
 	if (!encl_build(&secs, bin, bin_size, &sigstruct))
 		exit(1);
 
-	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);
+	test_sgx_basic(&secs);
 
 	if (!setup_vdso())
 		exit(1);
 
-	memset(&exception, 0, sizeof(exception));
-
-	printf("Input: 0x%lx\n", MAGIC);
-
-	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
-		      (void *)secs.base, &exception, NULL);
-	if (result != MAGIC) {
-		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
-		exit(1);
-	}
-
-	printf("Output: 0x%lx\n", result);
+	test_sgx_vdso(&secs);
 
 	exit(0);
 }
-- 
2.22.0

[PATCH for_v2? v2 07/14] selftests/x86/sgx: Use standard helper function to signal pass/fail

[Sean Christopherson]

Use the various helpers provided by kselftest.h to configure the test
and signal/pass failure.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 140dffe9c765..664a2ed98915 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -17,6 +17,8 @@
 #include <sys/types.h>
 #include <sys/auxv.h>
 
+#include "../../kselftest.h"
+
 #include "defines.h"
 #include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
 #include "../../../../../arch/x86/include/uapi/asm/sgx.h"
@@ -336,15 +338,12 @@ 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);
+		ksft_test_result_error("0x%lx != 0x%lx\n", result, MAGIC);
+		return;
 	}
-
-	printf("Output: 0x%lx\n", result);
+	ksft_test_result_pass("%s: Passed\n", __func__);
 }
 
 static void test_sgx_vdso(struct sgx_secs *secs)
@@ -354,16 +353,13 @@ static void test_sgx_vdso(struct sgx_secs *secs)
 
 	memset(&exception, 0, sizeof(exception));
 
-	printf("Input: 0x%lx\n", MAGIC);
-
 	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
 		      (void *)secs->base, &exception, NULL);
 	if (result != MAGIC) {
-		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
-		exit(1);
+		ksft_test_result_error("0x%lx != 0x%lx\n", result, MAGIC);
+		return;
 	}
-
-	printf("Output: 0x%lx\n", result);
+	ksft_test_result_pass("%s: Passed\n", __func__);
 }
 
 int main(int argc, char *argv[], char *envp[])
@@ -373,6 +369,9 @@ int main(int argc, char *argv[], char *envp[])
 	off_t bin_size;
 	void *bin;
 
+	ksft_print_header();
+	ksft_set_plan(2);
+
 	if (!encl_data_map("encl.bin", &bin, &bin_size))
 		exit(1);
 
@@ -389,5 +388,5 @@ int main(int argc, char *argv[], char *envp[])
 
 	test_sgx_vdso(&secs);
 
-	exit(0);
+	return ksft_exit_pass();
 }
-- 
2.22.0

[PATCH for_v2? v2 08/14] selftests/harness: Move operator macros to their own header file

[Sean Christopherson]

Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
so that they can be reused by other selftests without pulling in the
full harness framework, which is cumbersome to use for testing features
that require a substantial amount of setup, need callbacks, etc...

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 Documentation/dev-tools/kselftest.rst         |   9 +-
 MAINTAINERS                                   |   1 +
 tools/testing/selftests/kselftest_harness.h   | 246 +----------------
 tools/testing/selftests/kselftest_operators.h | 255 ++++++++++++++++++
 4 files changed, 259 insertions(+), 252 deletions(-)
 create mode 100644 tools/testing/selftests/kselftest_operators.h

diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
index 25604904fa6e..09dbeb8ab502 100644
--- a/Documentation/dev-tools/kselftest.rst
+++ b/Documentation/dev-tools/kselftest.rst
@@ -290,12 +290,5 @@ Helpers
 Operators
 ---------
 
-.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
-    :doc: operators
+.. kernel-doc:: tools/testing/selftests/kselftest_operators.h
 
-.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
-    :functions: ASSERT_EQ ASSERT_NE ASSERT_LT ASSERT_LE ASSERT_GT ASSERT_GE
-                ASSERT_NULL ASSERT_TRUE ASSERT_NULL ASSERT_TRUE ASSERT_FALSE
-                ASSERT_STREQ ASSERT_STRNE EXPECT_EQ EXPECT_NE EXPECT_LT
-                EXPECT_LE EXPECT_GT EXPECT_GE EXPECT_NULL EXPECT_TRUE
-                EXPECT_FALSE EXPECT_STREQ EXPECT_STRNE
diff --git a/MAINTAINERS b/MAINTAINERS
index 1eb065d3c209..71d680dff071 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14514,6 +14514,7 @@ F:	include/uapi/linux/seccomp.h
 F:	include/linux/seccomp.h
 F:	tools/testing/selftests/seccomp/*
 F:	tools/testing/selftests/kselftest_harness.h
+F:	tools/testing/selftests/kselftest_operators.h
 F:	Documentation/userspace-api/seccomp_filter.rst
 K:	\bsecure_computing
 K:	\bTIF_SECCOMP\b
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5336b26506ab..89af5a7bfd65 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -62,6 +62,8 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include "kselftest_operators.h"
+
 #define TEST_TIMEOUT_DEFAULT 30
 
 /* Utilities exposed to the test definitions */
@@ -343,250 +345,6 @@
 		return test_harness_run(argc, argv); \
 	}
 
-/**
- * DOC: operators
- *
- * Operators for use in TEST() and TEST_F().
- * ASSERT_* calls will stop test execution immediately.
- * EXPECT_* calls will emit a failure warning, note it, and continue.
- */
-
-/**
- * ASSERT_EQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_EQ(expected, measured): expected == measured
- */
-#define ASSERT_EQ(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, ==, 1)
-
-/**
- * ASSERT_NE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_NE(expected, measured): expected != measured
- */
-#define ASSERT_NE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, !=, 1)
-
-/**
- * ASSERT_LT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_LT(expected, measured): expected < measured
- */
-#define ASSERT_LT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <, 1)
-
-/**
- * ASSERT_LE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_LE(expected, measured): expected <= measured
- */
-#define ASSERT_LE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <=, 1)
-
-/**
- * ASSERT_GT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_GT(expected, measured): expected > measured
- */
-#define ASSERT_GT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >, 1)
-
-/**
- * ASSERT_GE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_GE(expected, measured): expected >= measured
- */
-#define ASSERT_GE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >=, 1)
-
-/**
- * ASSERT_NULL(seen)
- *
- * @seen: measured value
- *
- * ASSERT_NULL(measured): NULL == measured
- */
-#define ASSERT_NULL(seen) \
-	__EXPECT(NULL, "NULL", seen, #seen, ==, 1)
-
-/**
- * ASSERT_TRUE(seen)
- *
- * @seen: measured value
- *
- * ASSERT_TRUE(measured): measured != 0
- */
-#define ASSERT_TRUE(seen) \
-	__EXPECT(0, "0", seen, #seen, !=, 1)
-
-/**
- * ASSERT_FALSE(seen)
- *
- * @seen: measured value
- *
- * ASSERT_FALSE(measured): measured == 0
- */
-#define ASSERT_FALSE(seen) \
-	__EXPECT(0, "0", seen, #seen, ==, 1)
-
-/**
- * ASSERT_STREQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_STREQ(expected, measured): !strcmp(expected, measured)
- */
-#define ASSERT_STREQ(expected, seen) \
-	__EXPECT_STR(expected, seen, ==, 1)
-
-/**
- * ASSERT_STRNE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * ASSERT_STRNE(expected, measured): strcmp(expected, measured)
- */
-#define ASSERT_STRNE(expected, seen) \
-	__EXPECT_STR(expected, seen, !=, 1)
-
-/**
- * EXPECT_EQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_EQ(expected, measured): expected == measured
- */
-#define EXPECT_EQ(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, ==, 0)
-
-/**
- * EXPECT_NE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_NE(expected, measured): expected != measured
- */
-#define EXPECT_NE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, !=, 0)
-
-/**
- * EXPECT_LT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_LT(expected, measured): expected < measured
- */
-#define EXPECT_LT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <, 0)
-
-/**
- * EXPECT_LE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_LE(expected, measured): expected <= measured
- */
-#define EXPECT_LE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, <=, 0)
-
-/**
- * EXPECT_GT(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_GT(expected, measured): expected > measured
- */
-#define EXPECT_GT(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >, 0)
-
-/**
- * EXPECT_GE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_GE(expected, measured): expected >= measured
- */
-#define EXPECT_GE(expected, seen) \
-	__EXPECT(expected, #expected, seen, #seen, >=, 0)
-
-/**
- * EXPECT_NULL(seen)
- *
- * @seen: measured value
- *
- * EXPECT_NULL(measured): NULL == measured
- */
-#define EXPECT_NULL(seen) \
-	__EXPECT(NULL, "NULL", seen, #seen, ==, 0)
-
-/**
- * EXPECT_TRUE(seen)
- *
- * @seen: measured value
- *
- * EXPECT_TRUE(measured): 0 != measured
- */
-#define EXPECT_TRUE(seen) \
-	__EXPECT(0, "0", seen, #seen, !=, 0)
-
-/**
- * EXPECT_FALSE(seen)
- *
- * @seen: measured value
- *
- * EXPECT_FALSE(measured): 0 == measured
- */
-#define EXPECT_FALSE(seen) \
-	__EXPECT(0, "0", seen, #seen, ==, 0)
-
-/**
- * EXPECT_STREQ(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_STREQ(expected, measured): !strcmp(expected, measured)
- */
-#define EXPECT_STREQ(expected, seen) \
-	__EXPECT_STR(expected, seen, ==, 0)
-
-/**
- * EXPECT_STRNE(expected, seen)
- *
- * @expected: expected value
- * @seen: measured value
- *
- * EXPECT_STRNE(expected, measured): strcmp(expected, measured)
- */
-#define EXPECT_STRNE(expected, seen) \
-	__EXPECT_STR(expected, seen, !=, 0)
-
 #define ARRAY_SIZE(a)	(sizeof(a) / sizeof(a[0]))
 
 /* Support an optional handler after and ASSERT_* or EXPECT_*.  The approach is
diff --git a/tools/testing/selftests/kselftest_operators.h b/tools/testing/selftests/kselftest_operators.h
new file mode 100644
index 000000000000..6ae5b547313f
--- /dev/null
+++ b/tools/testing/selftests/kselftest_operators.h
@@ -0,0 +1,255 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * See documentation in Documentation/dev-tools/kselftest.rst
+ */
+
+#ifndef __KSELFTEST_OPERATORS_H
+#define __KSELFTEST_OPERATORS_H
+
+/**
+ * DOC:
+ *
+ * Operators for use in Test Harness's TEST() and TEST_F(), or with a custom
+ * implementation of __EXPECT().
+ * ASSERT_* calls will stop test execution immediately.
+ * EXPECT_* calls will emit a failure warning, note it, and continue.
+ */
+
+/**
+ * ASSERT_EQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_EQ(expected, measured): expected == measured
+ */
+#define ASSERT_EQ(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, ==, 1)
+
+/**
+ * ASSERT_NE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_NE(expected, measured): expected != measured
+ */
+#define ASSERT_NE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, !=, 1)
+
+/**
+ * ASSERT_LT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_LT(expected, measured): expected < measured
+ */
+#define ASSERT_LT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <, 1)
+
+/**
+ * ASSERT_LE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_LE(expected, measured): expected <= measured
+ */
+#define ASSERT_LE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <=, 1)
+
+/**
+ * ASSERT_GT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_GT(expected, measured): expected > measured
+ */
+#define ASSERT_GT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >, 1)
+
+/**
+ * ASSERT_GE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_GE(expected, measured): expected >= measured
+ */
+#define ASSERT_GE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >=, 1)
+
+/**
+ * ASSERT_NULL(seen)
+ *
+ * @seen: measured value
+ *
+ * ASSERT_NULL(measured): NULL == measured
+ */
+#define ASSERT_NULL(seen) \
+	__EXPECT(NULL, "NULL", seen, #seen, ==, 1)
+
+/**
+ * ASSERT_TRUE(seen)
+ *
+ * @seen: measured value
+ *
+ * ASSERT_TRUE(measured): measured != 0
+ */
+#define ASSERT_TRUE(seen) \
+	__EXPECT(0, "0", seen, #seen, !=, 1)
+
+/**
+ * ASSERT_FALSE(seen)
+ *
+ * @seen: measured value
+ *
+ * ASSERT_FALSE(measured): measured == 0
+ */
+#define ASSERT_FALSE(seen) \
+	__EXPECT(0, "0", seen, #seen, ==, 1)
+
+/**
+ * ASSERT_STREQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_STREQ(expected, measured): !strcmp(expected, measured)
+ */
+#define ASSERT_STREQ(expected, seen) \
+	__EXPECT_STR(expected, seen, ==, 1)
+
+/**
+ * ASSERT_STRNE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * ASSERT_STRNE(expected, measured): strcmp(expected, measured)
+ */
+#define ASSERT_STRNE(expected, seen) \
+	__EXPECT_STR(expected, seen, !=, 1)
+
+/**
+ * EXPECT_EQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_EQ(expected, measured): expected == measured
+ */
+#define EXPECT_EQ(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, ==, 0)
+
+/**
+ * EXPECT_NE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_NE(expected, measured): expected != measured
+ */
+#define EXPECT_NE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, !=, 0)
+
+/**
+ * EXPECT_LT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_LT(expected, measured): expected < measured
+ */
+#define EXPECT_LT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <, 0)
+
+/**
+ * EXPECT_LE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_LE(expected, measured): expected <= measured
+ */
+#define EXPECT_LE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, <=, 0)
+
+/**
+ * EXPECT_GT(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_GT(expected, measured): expected > measured
+ */
+#define EXPECT_GT(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >, 0)
+
+/**
+ * EXPECT_GE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_GE(expected, measured): expected >= measured
+ */
+#define EXPECT_GE(expected, seen) \
+	__EXPECT(expected, #expected, seen, #seen, >=, 0)
+
+/**
+ * EXPECT_NULL(seen)
+ *
+ * @seen: measured value
+ *
+ * EXPECT_NULL(measured): NULL == measured
+ */
+#define EXPECT_NULL(seen) \
+	__EXPECT(NULL, "NULL", seen, #seen, ==, 0)
+
+/**
+ * EXPECT_TRUE(seen)
+ *
+ * @seen: measured value
+ *
+ * EXPECT_TRUE(measured): 0 != measured
+ */
+#define EXPECT_TRUE(seen) \
+	__EXPECT(0, "0", seen, #seen, !=, 0)
+
+/**
+ * EXPECT_FALSE(seen)
+ *
+ * @seen: measured value
+ *
+ * EXPECT_FALSE(measured): 0 == measured
+ */
+#define EXPECT_FALSE(seen) \
+	__EXPECT(0, "0", seen, #seen, ==, 0)
+
+/**
+ * EXPECT_STREQ(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_STREQ(expected, measured): !strcmp(expected, measured)
+ */
+#define EXPECT_STREQ(expected, seen) \
+	__EXPECT_STR(expected, seen, ==, 0)
+
+/**
+ * EXPECT_STRNE(expected, seen)
+ *
+ * @expected: expected value
+ * @seen: measured value
+ *
+ * EXPECT_STRNE(expected, measured): strcmp(expected, measured)
+ */
+#define EXPECT_STRNE(expected, seen) \
+	__EXPECT_STR(expected, seen, !=, 0)
+
+#endif  /* __KSELFTEST_OPERATORS_H */
+
-- 
2.22.0

[PATCH for_v2? v2 09/14] selftests/x86/sgx: Use kselftest operators to check test results

[Sean Christopherson]

Use kselftest's operators, e.g. ASSERT_EQ, EXPECT_EQ, etc... to check
test results.  Implement a custom __EXPECT() macro instead of using the
framework defined in kselftest_harness.h.  The harness framework is
designed for tests that are short and sweet, e.g. true unit tests, and
don't work well with SGX's need for a large, run-once setup.  The
harness code will also be problematic when tests for the vDSO's callback
code are added in the future.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 52 ++++++++++++++++++++------
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 664a2ed98915..f1bd74913ec3 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -18,6 +18,7 @@
 #include <sys/auxv.h>
 
 #include "../../kselftest.h"
+#include "../../kselftest_operators.h"
 
 #include "defines.h"
 #include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
@@ -26,6 +27,41 @@
 
 #define PAGE_SIZE  4096
 
+#define EXPECT_FAILED(_assert, fmt, ...)			\
+do {								\
+	if (_assert)						\
+		ksft_exit_fail_msg(fmt, ##__VA_ARGS__);		\
+	else							\
+		ksft_test_result_fail(fmt, ##__VA_ARGS__);	\
+} while (0)
+
+#define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert)     \
+do {									      \
+	/* Avoid multiple evaluation of the cases */			      \
+	__typeof__(_expected) __exp = (_expected);			      \
+	__typeof__(_seen) __seen = (_seen);				      \
+	if (passed && !(__exp _t __seen)) {				      \
+		unsigned long long __exp_print = (uintptr_t)__exp;	      \
+		unsigned long long __seen_print = (uintptr_t)__seen;	      \
+		EXPECT_FAILED(_assert,					      \
+			      "Expected '%s (%llu) %s %s (%llu)' at %s:%u\n", \
+			      _expected_str, __exp_print, #_t, _seen_str,     \
+			      __seen_print, __FILE__, __LINE__);	      \
+		passed = false;						      \
+	}								      \
+} while (0)
+
+#define RUN_TEST(test_name)						\
+({									\
+	passed = true;							\
+									\
+	test_name(&secs);						\
+	if (passed)							\
+		ksft_test_result_pass("%s: Passed\n", #test_name);	\
+})
+
+static bool passed = true;
+
 static const uint64_t MAGIC = 0x1122334455667788ULL;
 void *eenter;
 
@@ -339,11 +375,7 @@ static void test_sgx_basic(struct sgx_secs *secs)
 	uint64_t result = 0;
 
 	sgx_call_eenter((void *)&MAGIC, &result, (void *)secs->base);
-	if (result != MAGIC) {
-		ksft_test_result_error("0x%lx != 0x%lx\n", result, MAGIC);
-		return;
-	}
-	ksft_test_result_pass("%s: Passed\n", __func__);
+	EXPECT_EQ(result, MAGIC);
 }
 
 static void test_sgx_vdso(struct sgx_secs *secs)
@@ -355,11 +387,7 @@ static void test_sgx_vdso(struct sgx_secs *secs)
 
 	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
 		      (void *)secs->base, &exception, NULL);
-	if (result != MAGIC) {
-		ksft_test_result_error("0x%lx != 0x%lx\n", result, MAGIC);
-		return;
-	}
-	ksft_test_result_pass("%s: Passed\n", __func__);
+	EXPECT_EQ(result, MAGIC);
 }
 
 int main(int argc, char *argv[], char *envp[])
@@ -381,12 +409,12 @@ int main(int argc, char *argv[], char *envp[])
 	if (!encl_build(&secs, bin, bin_size, &sigstruct))
 		exit(1);
 
-	test_sgx_basic(&secs);
+	RUN_TEST(test_sgx_basic);
 
 	if (!setup_vdso())
 		exit(1);
 
-	test_sgx_vdso(&secs);
+	RUN_TEST(test_sgx_vdso);
 
 	return ksft_exit_pass();
 }
-- 
2.22.0

[PATCH for_v2? v2 10/14] selftests/x86/sgx: Handle setup failures via kselftest assertions

[Sean Christopherson]

Use kselftest's assertion operators to report errors and exit instead of
propagating errors up the stack.  Using assertions reduces code and
provides more detailed error messages, and all existing errors lead to
exit(1) anyways, i.e. asserting isn't blocking forward progress.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 174 +++++++++----------------
 1 file changed, 58 insertions(+), 116 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index f1bd74913ec3..24647050657a 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -51,6 +51,14 @@ do {									      \
 	}								      \
 } while (0)
 
+#define ASSERT_RAW(_cond, fmt, ...)					\
+do {									\
+	if (!(_cond)) {							\
+		fprintf(stderr, "  %s:%u: ", __FILE__, __LINE__);	\
+		ksft_exit_fail_msg(fmt, ##__VA_ARGS__);			\
+	}								\
+} while (0)
+
 #define RUN_TEST(test_name)						\
 ({									\
 	passed = true;							\
@@ -100,23 +108,18 @@ static void *vdso_get_dyn(void *addr, Elf64_Dyn *dyntab, Elf64_Sxword tag)
 	return NULL;
 }
 
-static bool vdso_get_symtab(void *addr, struct vdso_symtab *symtab)
+static void vdso_get_symtab(void *addr, struct vdso_symtab *symtab)
 {
 	Elf64_Dyn *dyntab = vdso_get_dyntab(addr);
 
 	symtab->elf_symtab = vdso_get_dyn(addr, dyntab, DT_SYMTAB);
-	if (!symtab->elf_symtab)
-		return false;
+	ASSERT_NE(symtab->elf_symtab, NULL);
 
 	symtab->elf_symstrtab = vdso_get_dyn(addr, dyntab, DT_STRTAB);
-	if (!symtab->elf_symstrtab)
-		return false;
+	ASSERT_NE(symtab->elf_symstrtab, NULL);
 
 	symtab->elf_hashtab = vdso_get_dyn(addr, dyntab, DT_HASH);
-	if (!symtab->elf_hashtab)
-		return false;
-
-	return true;
+	ASSERT_NE(symtab->elf_hashtab, NULL);
 }
 
 static unsigned long elf_sym_hash(const char *name)
@@ -154,7 +157,7 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name)
 	return NULL;
 }
 
-static bool encl_create(int dev_fd, unsigned long bin_size,
+static void encl_create(int dev_fd, unsigned long bin_size,
 			struct sgx_secs *secs)
 {
 	struct sgx_enclave_create ioc;
@@ -170,10 +173,7 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 		secs->size <<= 1;
 
 	area = mmap(NULL, secs->size * 2, PROT_NONE, MAP_SHARED, dev_fd, 0);
-	if (area == MAP_FAILED) {
-		perror("mmap");
-		return false;
-	}
+	ASSERT_NE(area, MAP_FAILED);
 
 	secs->base = ((uint64_t)area + secs->size - 1) & ~(secs->size - 1);
 
@@ -183,16 +183,11 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 
 	ioc.src = (unsigned long)secs;
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_CREATE, &ioc);
-	if (rc) {
-		fprintf(stderr, "ECREATE failed rc=%d, err=%d.\n", rc, errno);
-		munmap((void *)secs->base, secs->size);
-		return false;
-	}
-
-	return true;
+	ASSERT_RAW(!rc, "ECREATE failed rc=%d, errno=%s.\n",
+		    rc, strerror(errno));
 }
 
-static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
+static void encl_add_page(int dev_fd, unsigned long addr, void *data,
 			  uint64_t flags)
 {
 	struct sgx_enclave_add_page ioc;
@@ -209,15 +204,10 @@ static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
 	memset(ioc.reserved, 0, sizeof(ioc.reserved));
 
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_ADD_PAGE, &ioc);
-	if (rc) {
-		fprintf(stderr, "EADD failed rc=%d.\n", rc);
-		return false;
-	}
-
-	return true;
+	ASSERT_RAW(!rc, "EADD failed rc=%d.\n", rc);
 }
 
-static bool encl_build(struct sgx_secs *secs, void *bin,
+static void encl_build(struct sgx_secs *secs, void *bin,
 		       unsigned long bin_size, struct sgx_sigstruct *sigstruct)
 {
 	struct sgx_enclave_init ioc;
@@ -228,13 +218,10 @@ static bool encl_build(struct sgx_secs *secs, void *bin,
 	int rc;
 
 	dev_fd = open("/dev/sgx/enclave", O_RDWR);
-	if (dev_fd < 0) {
-		fprintf(stderr, "Unable to open /dev/sgx\n");
-		return false;
-	}
+	ASSERT_RAW(dev_fd >= 0, "Unable to open /dev/sgx: %s\n",
+		   strerror(errno));
 
-	if (!encl_create(dev_fd, bin_size, secs))
-		goto out_dev_fd;
+	encl_create(dev_fd, bin_size, secs);
 
 	for (offset = 0; offset < bin_size; offset += 0x1000) {
 		if (!offset)
@@ -243,131 +230,90 @@ static bool encl_build(struct sgx_secs *secs, void *bin,
 			flags = SGX_SECINFO_REG | SGX_SECINFO_R |
 				SGX_SECINFO_W | SGX_SECINFO_X;
 
-		if (!encl_add_page(dev_fd, secs->base + offset,
-				   bin + offset, flags))
-			goto out_map;
+		encl_add_page(dev_fd, secs->base + offset, bin + offset, flags);
 	}
 
 	ioc.sigstruct = (uint64_t)sigstruct;
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_INIT, &ioc);
-	if (rc) {
-		printf("EINIT failed rc=%d\n", rc);
-		goto out_map;
-	}
+	ASSERT_RAW(!rc, "EINIT failed rc=%d, errno=%s.\n", rc, strerror(errno));
 
 	addr = mmap((void *)secs->base, PAGE_SIZE, PROT_READ | PROT_WRITE,
 		    MAP_SHARED | MAP_FIXED, dev_fd, 0);
-	if (addr == MAP_FAILED) {
-		fprintf(stderr, "mmap() failed on TCS, errno=%d.\n", errno);
-		return false;
-	}
+	ASSERT_RAW(addr != MAP_FAILED, "mmap() failed on TCS: %s\n",
+		    strerror(errno));
 
 	addr = mmap((void *)(secs->base + PAGE_SIZE), bin_size - PAGE_SIZE,
 		    PROT_READ | PROT_WRITE | PROT_EXEC,
 		    MAP_SHARED | MAP_FIXED, dev_fd, 0);
-	if (addr == MAP_FAILED) {
-		fprintf(stderr, "mmap() failed, errno=%d.\n", errno);
-		return false;
-	}
+	ASSERT_RAW(addr != MAP_FAILED, "mmap() failed on REG page: %s\n",
+		    strerror(errno));
 
 	close(dev_fd);
-	return true;
-out_map:
-	munmap((void *)secs->base, secs->size);
-out_dev_fd:
-	close(dev_fd);
-	return false;
 }
 
-static bool get_file_size(const char *path, off_t *bin_size)
+static off_t get_file_size(const char *path)
 {
 	struct stat sb;
 	int ret;
 
 	ret = stat(path, &sb);
-	if (ret) {
-		perror("stat");
-		return false;
-	}
-
-	if (!sb.st_size || sb.st_size & 0xfff) {
-		fprintf(stderr, "Invalid blob size %lu\n", sb.st_size);
-		return false;
-	}
-
-	*bin_size = sb.st_size;
-	return true;
+	ASSERT_RAW(!ret, "stat() %s failed: %s\n", path, strerror(errno));
+
+	ASSERT_RAW(sb.st_size && !(sb.st_size & 0xfff),
+		    "Invalid blob size: %llu", sb.st_size);
+
+	return sb.st_size;
 }
 
-static bool encl_data_map(const char *path, void **bin, off_t *bin_size)
+static void *encl_data_map(const char *path, off_t *bin_size)
 {
+	void *bin;
 	int fd;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1)  {
-		fprintf(stderr, "open() %s failed, errno=%d.\n", path, errno);
-		return false;
-	}
+	ASSERT_RAW(fd >= 0, "open() %s failed: %s\n", path, strerror(errno));
 
-	if (!get_file_size(path, bin_size))
-		goto err_out;
+	*bin_size = get_file_size(path);
 
-	*bin = mmap(NULL, *bin_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (*bin == MAP_FAILED) {
-		fprintf(stderr, "mmap() %s failed, errno=%d.\n", path, errno);
-		goto err_out;
-	}
+	bin = mmap(NULL, *bin_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	ASSERT_RAW(bin != MAP_FAILED, "mmap() %s failed: %s\n",
+		    path, strerror(errno));
 
 	close(fd);
-	return true;
-
-err_out:
-	close(fd);
-	return false;
+	return bin;
 }
 
-static bool load_sigstruct(const char *path, void *sigstruct)
+static void load_sigstruct(const char *path, struct sgx_sigstruct *sigstruct)
 {
+	ssize_t nr_read;
 	int fd;
 
 	fd = open(path, O_RDONLY);
-	if (fd == -1)  {
-		fprintf(stderr, "open() %s failed, errno=%d.\n", path, errno);
-		return false;
-	}
-
-	if (read(fd, sigstruct, sizeof(struct sgx_sigstruct)) !=
-	    sizeof(struct sgx_sigstruct)) {
-		fprintf(stderr, "read() %s failed, errno=%d.\n", path, errno);
-		close(fd);
-		return false;
-	}
+	ASSERT_RAW(fd > 0, "open() %s failed: %s\n", path, strerror(errno));
+
+	nr_read = read(fd, sigstruct, sizeof(struct sgx_sigstruct));
+	ASSERT_RAW(nr_read == sizeof(struct sgx_sigstruct),
+		    "read() %s failed: %s\n", path, strerror(errno));
 
 	close(fd);
-	return true;
 }
 
-static bool setup_vdso(void)
+static void setup_vdso(void)
 {
 	struct vdso_symtab symtab;
 	Elf64_Sym *eenter_sym;
 	void *addr;
 
 	addr = vdso_get_base_addr();
-	if (!addr)
-		return false;
+	ASSERT_NE(addr, NULL);
 
-	if (!vdso_get_symtab(addr, &symtab))
-		return false;
+	vdso_get_symtab(addr, &symtab);
 
 	eenter_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave");
-	if (!eenter_sym)
-		return false;
+	ASSERT_NE(eenter_sym, NULL);
 
 	/* eenter is used by sgx_call_vdso() to call into the vDSO. */
 	eenter = addr + eenter_sym->st_value;
-
-	return true;
 }
 
 static void test_sgx_basic(struct sgx_secs *secs)
@@ -400,19 +346,15 @@ int main(int argc, char *argv[], char *envp[])
 	ksft_print_header();
 	ksft_set_plan(2);
 
-	if (!encl_data_map("encl.bin", &bin, &bin_size))
-		exit(1);
+	bin = encl_data_map("encl.bin", &bin_size);
 
-	if (!load_sigstruct("encl.ss", &sigstruct))
-		exit(1);
+	load_sigstruct("encl.ss", &sigstruct);
 
-	if (!encl_build(&secs, bin, bin_size, &sigstruct))
-		exit(1);
+	encl_build(&secs, bin, bin_size, &sigstruct);
 
 	RUN_TEST(test_sgx_basic);
 
-	if (!setup_vdso())
-		exit(1);
+	setup_vdso();
 
 	RUN_TEST(test_sgx_vdso);
 
-- 
2.22.0

[PATCH for_v2? v2 11/14] selftests/x86/sgx: Add a check on the vDSO exception reporting mechanism

[Sean Christopherson]

Add a check to verify that an exception on EENTER is correctly reported.
Although the type of exception doesn't truly matter, e.g. a page fault
is no more or less interesting than a general protection fault, use an
unaligned TCS to trigger a #GP to avoid complications on platforms that
report EPCM related #PFs as #GPs, e.g. SGX1 systems.

Suggested-by: Cedric Xing <cedric.xing@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/defines.h | 4 ++++
 tools/testing/selftests/x86/sgx/main.c    | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/tools/testing/selftests/x86/sgx/defines.h b/tools/testing/selftests/x86/sgx/defines.h
index 1e67f2f29f42..b303bcaeb8dd 100644
--- a/tools/testing/selftests/x86/sgx/defines.h
+++ b/tools/testing/selftests/x86/sgx/defines.h
@@ -36,4 +36,8 @@ typedef uint64_t u64;
 #include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
 #include "../../../../../arch/x86/include/uapi/asm/sgx.h"
 
+#define ENCLU_EENTER	2
+
+#define GP_VECTOR 13
+
 #endif /* DEFINES_H */
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 24647050657a..56e13ae4f1ce 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -334,6 +334,12 @@ static void test_sgx_vdso(struct sgx_secs *secs)
 	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
 		      (void *)secs->base, &exception, NULL);
 	EXPECT_EQ(result, MAGIC);
+
+	/* Verify a #GP is reported if the TCS isn't 4k aligned. */
+	sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
+		      (void *)(secs->base | 0xfff), &exception, NULL);
+	EXPECT_EQ(exception.trapnr, GP_VECTOR);
+	EXPECT_EQ(exception.leaf, ENCLU_EENTER);
 }
 
 int main(int argc, char *argv[], char *envp[])
-- 
2.22.0

[PATCH for_v2? v2 12/14] selftests/x86/sgx: Add test of vDSO with basic exit handler

[Sean Christopherson]

Add a test to verify that nothing explodes when using an exit handler to
control the flow of the vDSO.

Suggested-by: Suggested-by: Cedric Xing <cedric.xing@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 28 +++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 56e13ae4f1ce..0a4716cef486 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -342,6 +342,31 @@ static void test_sgx_vdso(struct sgx_secs *secs)
 	EXPECT_EQ(exception.leaf, ENCLU_EENTER);
 }
 
+static int basic_exit_handler(long rdi, long rsi, long rdx, long ursp,
+			      long r8, long r9, void *tcs, int ret,
+			      struct sgx_enclave_exception *e)
+{
+	ASSERT_EQ(ret, 0);
+	return 0;
+}
+
+/*
+ * Test the vDSO API, __vdso_sgx_enter_enclave(), with an exit handler.
+ */
+static void test_sgx_vdso_exit_handler(struct sgx_secs *secs)
+{
+	struct sgx_enclave_exception exception;
+	uint64_t result = 0;
+	long ret;
+
+	memset(&exception, 0, sizeof(exception));
+
+	ret = sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
+			    (void *)secs->base, &exception, basic_exit_handler);
+	ASSERT_EQ(ret, 0);
+	ASSERT_EQ(result, MAGIC);
+}
+
 int main(int argc, char *argv[], char *envp[])
 {
 	struct sgx_sigstruct sigstruct;
@@ -350,7 +375,7 @@ int main(int argc, char *argv[], char *envp[])
 	void *bin;
 
 	ksft_print_header();
-	ksft_set_plan(2);
+	ksft_set_plan(3);
 
 	bin = encl_data_map("encl.bin", &bin_size);
 
@@ -363,6 +388,7 @@ int main(int argc, char *argv[], char *envp[])
 	setup_vdso();
 
 	RUN_TEST(test_sgx_vdso);
+	RUN_TEST(test_sgx_vdso_exit_handler);
 
 	return ksft_exit_pass();
 }
-- 
2.22.0

[PATCH for_v2? v2 13/14] selftests/x86/sgx: Add check to verify exit handler stack alignment

[Sean Christopherson]

Add an assembly trampoline to the basic exit handler to snapshot the
pre-CALL %rsp. Use the snapshot to verify that the stack is 16-byte
aligned as required by the x86_64 ABI.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/defines.h |  1 +
 tools/testing/selftests/x86/sgx/main.c    | 20 +++++++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/defines.h b/tools/testing/selftests/x86/sgx/defines.h
index b303bcaeb8dd..c4d8b88f3d8a 100644
--- a/tools/testing/selftests/x86/sgx/defines.h
+++ b/tools/testing/selftests/x86/sgx/defines.h
@@ -15,6 +15,7 @@ typedef uint64_t u64;
 
 #define __aligned(x) __attribute__((__aligned__(x)))
 #define __packed __attribute__((packed))
+#define __used __attribute__((__used__))
 
 /* Derived from asm-generic/bitsperlong.h. */
 #if __x86_64__
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 0a4716cef486..5ebe25dc877c 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -342,14 +342,28 @@ static void test_sgx_vdso(struct sgx_secs *secs)
 	EXPECT_EQ(exception.leaf, ENCLU_EENTER);
 }
 
-static int basic_exit_handler(long rdi, long rsi, long rdx, long ursp,
-			      long r8, long r9, void *tcs, int ret,
-			      struct sgx_enclave_exception *e)
+static int __used __basic_exit_handler(long rdi, long rsi, long rdx, long ursp,
+				       long r8, long r9, void *tcs, int ret,
+				       struct sgx_enclave_exception *e)
 {
+	ASSERT_RAW(!(r9 & 0xf), "Pre-CALL RSP not 16-byte aligned: %lx\n", r9);
 	ASSERT_EQ(ret, 0);
 	return 0;
 }
 
+extern void *basic_exit_handler;
+
+static void __used basic_exit_handler_trampoline(void)
+{
+	/* Load the pre-CALL %rsp into %r9 to verify correct alignment. */
+	asm volatile("1:\n\t"
+		     "lea 0x8(%%rsp), %%r9\n\t"
+		     "jmp __basic_exit_handler\n\t"
+		     "basic_exit_handler: .quad 1b\n\t"
+		     ".global basic_exit_handler"
+		     ::: "memory");
+}
+
 /*
  * Test the vDSO API, __vdso_sgx_enter_enclave(), with an exit handler.
  */
-- 
2.22.0

[PATCH for_v2? v2 14/14] selftests/x86/sgx: Add test for exception behavior with exit handler

[Sean Christopherson]

Add a test to verify the kernel and vDSO provide the correct exception
info when using an exit handler, e.g. leaf, trapnr and error_code, and
that the vDSO correctly interprets the return from the exit handler.  To
do so, change the enclave's protections to PROT_NONE and iteratively fix
the faults encountered, with various assertions along the way, e.g. the
first fault should always be on the TCS, at least three total faults
should occur, etc...

Suggested-by: Cedric Xing <cedric.xing@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/defines.h |  2 +
 tools/testing/selftests/x86/sgx/main.c    | 92 ++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sgx/defines.h b/tools/testing/selftests/x86/sgx/defines.h
index c4d8b88f3d8a..f9923fe64aff 100644
--- a/tools/testing/selftests/x86/sgx/defines.h
+++ b/tools/testing/selftests/x86/sgx/defines.h
@@ -38,7 +38,9 @@ typedef uint64_t u64;
 #include "../../../../../arch/x86/include/uapi/asm/sgx.h"
 
 #define ENCLU_EENTER	2
+#define ENCLU_ERESUME	3
 
 #define GP_VECTOR 13
+#define PF_VECTOR 14
 
 #endif /* DEFINES_H */
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 5ebe25dc877c..3f657902ba3f 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -381,6 +381,95 @@ static void test_sgx_vdso_exit_handler(struct sgx_secs *secs)
 	ASSERT_EQ(result, MAGIC);
 }
 
+static int nr_page_faults;
+
+static int mprotect_exit_handler(long rdi, long rsi, long rdx, long ursp,
+				 long r8, long r9, void *tcs, int ret,
+				 struct sgx_enclave_exception *e)
+{
+	int prot, rc;
+
+	if (!ret)
+		return 0;
+
+	++nr_page_faults;
+
+	ASSERT_EQ(ret, -EFAULT);
+	ASSERT_EQ(e->trapnr, PF_VECTOR);
+	ASSERT_RAW(e->leaf == ENCLU_EENTER || e->leaf == ENCLU_ERESUME,
+		    "Expected #PF on EENTER or ERESUME, leaf = %d\n", e->leaf);
+
+	/* The first #PF should be on the TCS, passed in via R9. */
+	if (nr_page_faults == 1) {
+		ASSERT_EQ(r9, (e->address & ~0xfff));
+		ASSERT_TRUE(e->error_code & 0x2);
+	}
+
+	prot = PROT_READ;
+	if (e->error_code & 0x2)
+		prot |= PROT_WRITE;
+	if (e->error_code & 0x10)
+		prot |= PROT_EXEC;
+	rc = mprotect((void *)(e->address & ~0xfff), PAGE_SIZE, prot);
+	ASSERT_EQ(rc, 0);
+
+	/*
+	 * If EENTER faulted, bounce all the way back to the test to verify
+	 * the vDSO is handling the return value correctly.
+	 */
+	if (e->leaf == ENCLU_EENTER)
+		return -EAGAIN;
+
+	/* Else ERESUME faulted, simply do ERESUME again. */
+	return e->leaf;
+}
+
+static void test_sgx_vdso_exception_handler(struct sgx_secs *secs)
+{
+	struct sgx_enclave_exception exception;
+	uint64_t result = 0;
+	int ret;
+
+	memset(&exception, 0, sizeof(exception));
+
+	/*
+	 * Make all pages inaccessible, then re-enter the enclave.  The exit
+	 * handler will service the resulting page faults using mprotect() to
+	 * restore the correct permissions.
+	 */
+	ret = mprotect((void *)secs->base, secs->size, PROT_NONE);
+	ASSERT_RAW(!ret, "mprotect() on enclave failed: %s\n", strerror(errno));
+
+	/* Loop on EENTER until it succeeds or it fails unexpectedly. */
+	result = 0;
+	do {
+		/*
+		 * Pass the address of the TCS to the exit handler via R9.
+		 * The first page fault should be on the TCS and R9 should
+		 * not be modified prior to entering the enclave (whic
+		 * requires an accessible TCS page).
+		 */
+		ret = sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL,
+				    (void *)secs->base, (void *)secs->base,
+				    &exception, mprotect_exit_handler);
+	} while (ret == -EAGAIN);
+	EXPECT_EQ(ret, 0);
+	EXPECT_EQ(result, MAGIC);
+
+	/* Enclave should re-execute cleanly. */
+	result = 0;
+	ret = sgx_call_vdso((void *)&MAGIC, &result, NULL, NULL, NULL, NULL,
+			    (void *)secs->base, &exception, basic_exit_handler);
+	EXPECT_EQ(ret, 0);
+	EXPECT_EQ(result, MAGIC);
+
+	/*
+	 * At least three faults should occur: one for the TCS, one for the
+	 * executable code, and one for the writable data (@result).
+	 */
+	EXPECT_GE(nr_page_faults, 3);
+}
+
 int main(int argc, char *argv[], char *envp[])
 {
 	struct sgx_sigstruct sigstruct;
@@ -389,7 +478,7 @@ int main(int argc, char *argv[], char *envp[])
 	void *bin;
 
 	ksft_print_header();
-	ksft_set_plan(3);
+	ksft_set_plan(4);
 
 	bin = encl_data_map("encl.bin", &bin_size);
 
@@ -403,6 +492,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	RUN_TEST(test_sgx_vdso);
 	RUN_TEST(test_sgx_vdso_exit_handler);
+	RUN_TEST(test_sgx_vdso_exception_handler);
 
 	return ksft_exit_pass();
 }
-- 
2.22.0

Re: [PATCH for_v2? v2 08/14] selftests/harness: Move operator macros to their own header file

[Jarkko Sakkinen]

On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> so that they can be reused by other selftests without pulling in the
> full harness framework, which is cumbersome to use for testing features
> that require a substantial amount of setup, need callbacks, etc...
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Is it possible to just use a "dull" selftest and not go into this before
the code is upstreamed? If yes, lets go with that. Do not mind using
frameworky stuff later on. Just trying to avoid new intersects with
other subsystems, that's all.

/Jarkko

Re: [PATCH for_v2? v2 08/14] selftests/harness: Move operator macros to their own header file

[Sean Christopherson]

On Thu, Oct 17, 2019 at 07:53:56PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> > Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> > so that they can be reused by other selftests without pulling in the
> > full harness framework, which is cumbersome to use for testing features
> > that require a substantial amount of setup, need callbacks, etc...
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Is it possible to just use a "dull" selftest and not go into this before
> the code is upstreamed? If yes, lets go with that.

It's certainly possible, but the code is verbose and ugly (IMO), which
means it will be harder for other to review.

> Do not mind using frameworky stuff later on. Just trying to avoid new
> intersects with other subsystems, that's all.

Understood, but IMO this particular change is safe-ish:

  - We're already intersecting with selftests
  - The odds of a conflict are very low
  - Andy is already involved in SGX


There are also other options:

  1. Use kselftest_harness.h as-is via #undef __EXPECT.  I actually would
     have gone with this option except the header also defines a function
     and we end up with a compiler error due to an unused function :-(

  2. Upstream the change separately from SGX, similar to the
     FEATURE_CONTROL handling.  E.g. I think the KVM selftests could
     use the operators, so there would even be concrete usage.

Re: [PATCH for_v2? v2 00/14] selftests/x86/sgx: Improve tests

[Jarkko Sakkinen]

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

Re: [PATCH for_v2? v2 00/14] selftests/x86/sgx: Improve tests

[Jarkko Sakkinen]

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

Re: [PATCH for_v2? v2 08/14] selftests/harness: Move operator macros to their own header file

[Jarkko Sakkinen]

On Thu, Oct 17, 2019 at 11:13:09AM -0700, Sean Christopherson wrote:
> On Thu, Oct 17, 2019 at 07:53:56PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> > > Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> > > so that they can be reused by other selftests without pulling in the
> > > full harness framework, which is cumbersome to use for testing features
> > > that require a substantial amount of setup, need callbacks, etc...
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Is it possible to just use a "dull" selftest and not go into this before
> > the code is upstreamed? If yes, lets go with that.
> 
> It's certainly possible, but the code is verbose and ugly (IMO), which
> means it will be harder for other to review.

Ok, I'll try to explain in more verbose terms how I see this.

Not all selftests use the harness and I'm not yet confident that SGX has
to. Unfortunately, ugly is for me something that I cannot put metrics
on. Also, often "ugly" is actually better than layering because it is
more transparent.

The test is comprised of simple POSIX calls that everyone knows whereas
using kselftest harness requires learning new framework. Less macros
makes code also easier to debug and pair compare to dissembly when
required. I've done the latter at least a few times.

It will also add a requirement for code reviewers who are simply looking
for a code example how SGX works also to learn the harness. In the scope
of the patch set the selftest serves as a such example.

/Jarkko

Re: [PATCH for_v2? v2 08/14] selftests/harness: Move operator macros to their own header file

[Sean Christopherson]

On Mon, Oct 21, 2019 at 02:08:23PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 17, 2019 at 11:13:09AM -0700, Sean Christopherson wrote:
> > On Thu, Oct 17, 2019 at 07:53:56PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Oct 16, 2019 at 08:03:34PM -0700, Sean Christopherson wrote:
> > > > Move the operator macros, ASSERT_* and EXTEND_*, to a standalone header
> > > > so that they can be reused by other selftests without pulling in the
> > > > full harness framework, which is cumbersome to use for testing features
> > > > that require a substantial amount of setup, need callbacks, etc...
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > 
> > > Is it possible to just use a "dull" selftest and not go into this before
> > > the code is upstreamed? If yes, lets go with that.
> > 
> > It's certainly possible, but the code is verbose and ugly (IMO), which
> > means it will be harder for other to review.
> 
> Ok, I'll try to explain in more verbose terms how I see this.
> 
> Not all selftests use the harness and I'm not yet confident that SGX has
> to. Unfortunately, ugly is for me something that I cannot put metrics
> on. Also, often "ugly" is actually better than layering because it is
> more transparent.
> 
> The test is comprised of simple POSIX calls that everyone knows whereas
> using kselftest harness requires learning new framework. Less macros
> makes code also easier to debug and pair compare to dissembly when
> required. I've done the latter at least a few times.
> 
> It will also add a requirement for code reviewers who are simply looking
> for a code example how SGX works also to learn the harness. In the scope
> of the patch set the selftest serves as a such example.

Eh, if SGX were actually using any of the harness stuff, sure, but I'd
hope most reviewers intuitively understand what ASSERT_EQ does.

Re: [PATCH for_v2? v2 00/14] selftests/x86/sgx: Improve tests

[Sean Christopherson]

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...

Re: [PATCH for_v2? v2 00/14] selftests/x86/sgx: Improve tests

[Jarkko Sakkinen]

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

Re: [PATCH for_v2? v2 00/14] selftests/x86/sgx: Improve tests

[Andy Lutomirski]

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.