[PATCH for_v23 00/16] x86/vdso: sgx: Major vDSO cleanup

[PATCH for_v23 00/16] x86/vdso: sgx: Major vDSO cleanup

[Sean Christopherson]

The main purpose of this series is to implement feedback from the original
RFC to expand the vDSO[*] that went unaddressed before the expanded
function was rushed into v21.

The other half of the series is to overhaul the selftest to actually test
the exit handler variation of the vDSO, with a bunch of prework to add an
assertion framework to standardize the various assertions in the test and
improve the readability of the code.

The basic ideas for the exit handler subtests are from Cedric's original
RFC, but rewritten from scratch to take advantage of the new assertion
framework.  I haven't yet implemented single-step subtest, ideally that
too will get done before v23.

[*] https://lkml.kernel.org/r/20190426210017.GA24467@linux.intel.com

Sean Christopherson (16):
  x86/vdso: sgx: Drop the pseudocode "documentation"
  x86/vdso: sgx: Do not use exception info to pass success/failure
  x86/vdso: sgx: Rename the enclave exit handler typedef
  x86/vdso: sgx: Move enclave exit handler declaration to UAPI header
  x86/vdso: sgx: Add comment regarding kernel-doc shenanigans
  x86/vdso: sgx: Rewrite __vdso_sgx_enter_enclave() function comment
  selftests/x86: Fix linker warning in SGX selftest
  selftests/x86/sgx: Use getauxval() to retrieve the vDSO base address
  selftests/x86/sgx: Add helper function and macros to assert results
  selftests/x86/sgx: Handle setup failures via test assertions
  selftests/x86/sgx: Sanitize the types for sgx_call()'s input params
  selftests/x86/sgx: Move existing sub-test to a separate helper
  selftests/x86/sgx: Add a test of the vDSO exception reporting
    mechanism
  selftests/x86/sgx: Add test of vDSO with basic exit handler
  selftests/x86/sgx: Add sub-test for exception behavior with exit
    handler
  x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no
    callback"

 arch/x86/entry/vdso/vsgx_enter_enclave.S  | 228 +++++++------
 arch/x86/include/uapi/asm/sgx.h           |  18 +
 tools/testing/selftests/x86/sgx/Makefile  |   2 +-
 tools/testing/selftests/x86/sgx/defines.h |   6 +
 tools/testing/selftests/x86/sgx/main.c    | 384 ++++++++++++++--------
 5 files changed, 387 insertions(+), 251 deletions(-)

-- 
2.22.0

[PATCH for_v23 01/16] x86/vdso: sgx: Drop the pseudocode "documentation"

[Sean Christopherson]

Remove the pseudocode documentation of __vdso_sgx_enter_enclave().  The
assembly itself needs to be cleaned up to be easily understood without
pseudocode, and the extra documentation adds maintenance overhead.

Only the prototype is needed to coerce kernel-doc into parsing the
function comment, so that isn't lost either.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 25 +-----------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 9331279b8fa6..96726000aa27 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -58,30 +58,7 @@ typedef int (*sgx_callback)(long rdi, long rsi, long rdx,
 			    long r9, void *tcs, long ursp);
 int __vdso_sgx_enter_enclave(int leaf, void *tcs,
 			     struct sgx_enclave_exinfo *exinfo,
-			     sgx_callback callback)
-{
-	while (leaf == EENTER || leaf == ERESUME) {
-		int rc;
-		try {
-			ENCLU[leaf];
-			rc = 0;
-			if (exinfo)
-				exinfo->leaf = EEXIT;
-		} catch (exception) {
-			rc = -EFAULT;
-			if (exinfo)
-				*exinfo = exception;
-		}
-
-		leaf = callback ? (*callback)(
-			rdi, rsi, rdx, exinfo, r8, r9, tcs, ursp) : rc;
-	}
-
-	if (leaf > 0)
-		return -EINVAL;
-
-	return leaf;
-}
+			     sgx_callback callback);
 #endif
 ENTRY(__vdso_sgx_enter_enclave)
 	/* Prolog */
-- 
2.22.0

[PATCH for_v23 02/16] x86/vdso: sgx: Do not use exception info to pass success/failure

[Sean Christopherson]

Do not overload sgx_enclave_exception.leaf to indicate success vs.
failure, i.e. EEXIT vs. fault.  Instead, explicitly pass what would be
the return value (from __vdso_sgx_enter_enclave()) to userspace's exit
handler.  Passing the return values makes the two flows (exit handler
vs. no exit handler) symmetric and provides the exit handler with a
fault indicator without requiring struct sgx_enclave_exception.

Opportunistically fix the typedef for the callback to reference struct
sgx_enclave_exception instead of the non-existent sgx_enclave_exinfo.

Intentionally leave the local labels out of whack, they'll be cleaned
up in a future patch.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 33 +++++++++++-------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 96726000aa27..06e18a2836de 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -53,11 +53,11 @@
  *    -EFAULT if ENCL or the enclave faults or non-positive value is returned
  *     from the callback.
  */
-typedef int (*sgx_callback)(long rdi, long rsi, long rdx,
-			    struct sgx_enclave_exinfo *exinfo, long r8,
-			    long r9, void *tcs, long ursp);
+typedef int (*sgx_callback)(long rdi, long rsi, long rdx, int ret,
+			    long r8, long r9, void *tcs, long ursp,
+			    struct sgx_enclave_exception *e);
 int __vdso_sgx_enter_enclave(int leaf, void *tcs,
-			     struct sgx_enclave_exinfo *exinfo,
+			     struct sgx_enclave_exception *e,
 			     sgx_callback callback);
 #endif
 ENTRY(__vdso_sgx_enter_enclave)
@@ -83,21 +83,12 @@ ENTRY(__vdso_sgx_enter_enclave)
 2:	enclu
 
 	/* EEXIT path */
-	xor	%ebx, %ebx
-3:	mov	0x18(%rbp), %rcx
-	jrcxz	4f
-	mov	%eax, EX_LEAF(%rcx)
-	jnc	4f
-	mov	%di, EX_TRAPNR(%rcx)
-	mov	%si, EX_ERROR_CODE(%rcx)
-	mov	%rdx, EX_ADDRESS(%rcx)
+	xor	%eax, %eax
+3:	mov	%eax, %ecx
 
-4:	/* Call *callback if supplied */
+	/* Call *callback if supplied */
 	mov	0x20(%rbp), %rax
 	test	%rax, %rax
-	/* At this point, %ebx holds the effective return value, which shall be
-	 * returned if no callback is specified */
-	cmovz	%rbx, %rax
 	jz	7f
 	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
 	 * restored after *callback returns. */
@@ -106,6 +97,7 @@ ENTRY(__vdso_sgx_enter_enclave)
 	/* Clear RFLAGS.DF per x86_64 ABI */
 	cld
 	/* Parameters for *callback */
+	push	0x18(%rbp)
 	push	%rbx
 	push	0x10(%rbp)
 	/* Call *%rax via retpoline */
@@ -126,8 +118,13 @@ ENTRY(__vdso_sgx_enter_enclave)
 	ret
 
 5:	/* Exception path */
-	mov	$-EFAULT, %ebx
-	stc
+	mov	0x18(%rbp), %rcx
+	jrcxz	52f
+	mov	%eax, EX_LEAF(%rcx)
+	mov	%di,  EX_TRAPNR(%rcx)
+	mov	%si,  EX_ERROR_CODE(%rcx)
+	mov	%rdx, EX_ADDRESS(%rcx)
+52:	mov	$-EFAULT, %eax
 	jmp	3b
 
 6:	/* Unsupported ENCLU leaf */
-- 
2.22.0

[PATCH for_v23 03/16] x86/vdso: sgx: Rename the enclave exit handler typedef

[Sean Christopherson]

Rename the exit handler callback to sgx_enclave_exit_handler_t so that
the name itself describes the purpose of the function to some extent.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 06e18a2836de..5fbe07a03e6c 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -53,12 +53,12 @@
  *    -EFAULT if ENCL or the enclave faults or non-positive value is returned
  *     from the callback.
  */
-typedef int (*sgx_callback)(long rdi, long rsi, long rdx, int ret,
-			    long r8, long r9, void *tcs, long ursp,
-			    struct sgx_enclave_exception *e);
+typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, int ret,
+					  long r8, long r9, void *tcs, long ursp,
+					  struct sgx_enclave_exception *e);
 int __vdso_sgx_enter_enclave(int leaf, void *tcs,
 			     struct sgx_enclave_exception *e,
-			     sgx_callback callback);
+			     sgx_enclave_exit_handler_t handler);
 #endif
 ENTRY(__vdso_sgx_enter_enclave)
 	/* Prolog */
@@ -86,17 +86,17 @@ ENTRY(__vdso_sgx_enter_enclave)
 	xor	%eax, %eax
 3:	mov	%eax, %ecx
 
-	/* Call *callback if supplied */
+	/* Call the exit handler if supplied */
 	mov	0x20(%rbp), %rax
 	test	%rax, %rax
 	jz	7f
 	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
-	 * restored after *callback returns. */
+	 * restored after the exit handler returns. */
 	mov	%rsp, %rbx
 	and	$-0x10, %rsp
 	/* Clear RFLAGS.DF per x86_64 ABI */
 	cld
-	/* Parameters for *callback */
+	/* Parameters for the exit handler */
 	push	0x18(%rbp)
 	push	%rbx
 	push	0x10(%rbp)
@@ -105,9 +105,9 @@ ENTRY(__vdso_sgx_enter_enclave)
 	/* Restore %rsp to its original value left off by the enclave from last
 	 * exit */
 	mov	%rbx, %rsp
-	/* Positive return value from *callback will be interpreted as an ENCLU
-	 * leaf, while a non-positive value will be interpreted as the return
-	 * value to be passed back to the caller. */
+	/* Positive return value from the exit handler will be interpreted as
+	 * an ENCLU leaf, while a non-positive value will be interpreted as the
+	 * return value to be passed back to the caller. */
 	jmp	1b
 40:	/* retpoline */
 	call	42f
-- 
2.22.0

[PATCH for_v23 04/16] x86/vdso: sgx: Move enclave exit handler declaration to UAPI header

[Sean Christopherson]

Move the typedef of the enclave exit handler to the UAPI header so that
it can be consumed by userspace and kernel-doc.  Add a proper comment,
primarily to document the parameters.  A future patch will update the
comment for __vdso_sgx_enter_enclave() to better describe how the
exit handler is used.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S |  3 ---
 arch/x86/include/uapi/asm/sgx.h          | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 5fbe07a03e6c..a382f3683b48 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -53,9 +53,6 @@
  *    -EFAULT if ENCL or the enclave faults or non-positive value is returned
  *     from the callback.
  */
-typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, int ret,
-					  long r8, long r9, void *tcs, long ursp,
-					  struct sgx_enclave_exception *e);
 int __vdso_sgx_enter_enclave(int leaf, void *tcs,
 			     struct sgx_enclave_exception *e,
 			     sgx_enclave_exit_handler_t handler);
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 8f4660e07f6b..0515de4e67cc 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -81,4 +81,22 @@ struct sgx_enclave_exception {
 	__u64 reserved[2];
 };
 
+/**
+ * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by
+ *					__vdso_sgx_enter_enclave()
+ *
+ * @rdi:	RDI at the time of enclave exit
+ * @rsi:	RSI at the time of enclave exit
+ * @rdx:	RDX at the time of enclave exit
+ * @ret:	0 on success (EEXIT), -EFAULT on an exception
+ * @r8:		R8 at the time of enclave exit
+ * @r9:		R9 at the time of enclave exit
+ * @tcs:	Thread Control Structure used to enter enclave
+ * @ursp:	RSP at the time of enclave exit
+ * @e:		Pointer to struct sgx_enclave_exception (as provided by caller)
+ */
+typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx, int ret,
+					  long r8, long r9, void *tcs, long ursp,
+					  struct sgx_enclave_exception *e);
+
 #endif /* _UAPI_ASM_X86_SGX_H */
-- 
2.22.0

[PATCH for_v23 05/16] x86/vdso: sgx: Add comment regarding kernel-doc shenanigans

[Sean Christopherson]

Move the fake SGX_KERNEL_DOC ifdef and add a comment to explicitly state
that the C-style prototype exists to trigger kernel-doc parsing.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index a382f3683b48..4dfb943172ed 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -14,7 +14,6 @@
 .code64
 .section .text, "ax"
 
-#ifdef SGX_KERNEL_DOC
 /**
  * __vdso_sgx_enter_enclave() - Enter an SGX enclave
  * @leaf:	ENCLU leaf, must be EENTER or ERESUME
@@ -53,6 +52,8 @@
  *    -EFAULT if ENCL or the enclave faults or non-positive value is returned
  *     from the callback.
  */
+#ifdef SGX_KERNEL_DOC
+/* C-style function prototype to coerce kernel-doc into parsing the comment. */
 int __vdso_sgx_enter_enclave(int leaf, void *tcs,
 			     struct sgx_enclave_exception *e,
 			     sgx_enclave_exit_handler_t handler);
-- 
2.22.0

[PATCH for_v23 06/16] x86/vdso: sgx: Rewrite __vdso_sgx_enter_enclave() function comment

[Sean Christopherson]

Rewrite the function comment for __vdso_sgx_enter_enclave() to eliminate
dependencies on markup (which currently doesn't work correctly anyways),
bring the comments up-to-date, and use phrasing and mood that is more
consistent with the rest of the kernel.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 74 +++++++++++++++---------
 1 file changed, 46 insertions(+), 28 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 4dfb943172ed..de54e47c83f4 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -18,39 +18,57 @@
  * __vdso_sgx_enter_enclave() - Enter an SGX enclave
  * @leaf:	ENCLU leaf, must be EENTER or ERESUME
  * @tcs:	TCS, must be non-NULL
- * @ex_info:	Optional struct sgx_enclave_exception instance
- * @callback:	Optional callback function to be called on enclave exit or
- *		exception
+ * @e:		Optional struct sgx_enclave_exception instance
+ * @handler:	Optional enclave exit handler
  *
  * **Important!**  __vdso_sgx_enter_enclave() is **NOT** compliant with the
- * x86-64 ABI, i.e. cannot be called from standard C code. As noted above,
- * input parameters must be passed via ``%eax``, ``8(%rsp)``, ``0x10(%rsp)`` and
- * ``0x18(%rsp)``, with the return value passed via ``%eax``. All other
- * registers will be passed through to the enclave as is. All registers except
- * ``%rbp`` must be treated as volatile from the caller's perspective, including
- * but not limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... Conversely, the
- * enclave being run **must** preserve the untrusted ``%rbp``.
+ * x86-64 ABI, i.e. cannot be called from standard C code.
  *
- * ``callback`` has the following signature:
- * int callback(long rdi, long rsi, long rdx,
- *		struct sgx_enclave_exinfo *exinfo, long r8, long r9,
- *		void *tcs, long ursp);
- * ``callback`` **shall** follow x86_64 ABI. All GPRs **except** ``%rax``,
- * ``%rbx`` and ``rcx`` are passed through to ``callback``. ``%rdi``, ``%rsi``,
- * ``%rdx``, ``%r8``, ``%r9``, along with the value of ``%rsp`` when the enclave
- * exited/excepted, can be accessed directly as input parameters, while other
- * GPRs can be accessed in assembly if needed.  A positive value returned from
- * ``callback`` will be treated as an ENCLU leaf (e.g. EENTER/ERESUME) to
- * reenter the enclave (without popping the extra data pushed by the enclave off
- * the stack), while 0 (zero) or a negative return value will be passed back to
- * the caller of __vdso_sgx_enter_enclave(). It is also safe to leave
- * ``callback`` via ``longjmp()`` or by throwing a C++ exception.
+ * Input ABI:
+ *  @leaf	%eax
+ *  @tcs	8(%rsp)
+ *  @e 		0x10(%rsp)
+ *  @handler	0x18(%rsp)
+ *
+ * Output ABI:
+ *  @ret	%eax
+ *
+ * All general purpose registers except RAX, RBX and RCX are passed as-is to
+ * the enclave. RAX, RBX and RCX are consumed by EENTER and ERESUME and are
+ * loaded with @leaf, asynchronous exit pointer, and @tcs respectively.
+ *
+ * RBP and the stack are used to anchor __vdso_sgx_enter_enclave() to the
+ * pre-enclave state, e.g. to retrieve @e and @handler after an enclave exit.
+ * All other registers are available for use by the enclave and its runtime,
+ * e.g. an enclave can push additional data onto the stack (and modify RSP) to
+ * pass information to the optional exit handler (see below).
+ *
+ * Most exceptions reported on ENCLU, including those that occur within the
+ * enclave, are fixed up and reported synchronously instead of being delivered
+ * via a standard signal. Debug Exceptions (#DB) and Breakpoints (#BP) are
+ * never fixed up and are always delivered via standard signals. On synchrously
+ * reported exceptions, -EFAULT is returned and details about the exception are
+ * recorded in @e, the optional sgx_enclave_exception struct.
+
+ * If an exit handler is provided, the handler will be invoked on synchronous
+ * exits from the enclave and for all synchronously reported exceptions. In
+ * latter case, @e is filled prior to invoking the handler.
+ *
+ * The exit handler's return value is interpreted as follows:
+ *  >0:		continue, restart __vdso_sgx_enter_enclave() with @ret as @leaf
+ *   0:		success, return @ret to the caller
+ *  <0:		error, return @ret to the caller
+ *
+ * The userspace exit handler is responsible for unwinding the stack, e.g. to
+ * pop @e, u_rsp and @tcs, prior to returning to __vdso_sgx_enter_enclave().
+ * The exit handler may also transfer control, e.g. via longjmp() or a C++
+ * exception, without returning to __vdso_sgx_enter_enclave().
  *
  * Return:
- *    0 on success,
- *    -EINVAL if ENCLU leaf is not allowed,
- *    -EFAULT if ENCL or the enclave faults or non-positive value is returned
- *     from the callback.
+ *  0 on success,
+ *  -EINVAL if ENCLU leaf is not allowed,
+ *  -EFAULT if an exception occurs on ENCLU or within the enclave
+ *  -errno for all other negative values returned by the userspace exit handler
  */
 #ifdef SGX_KERNEL_DOC
 /* C-style function prototype to coerce kernel-doc into parsing the comment. */
-- 
2.22.0

[PATCH for_v23 07/16] selftests/x86: Fix linker warning in SGX selftest

[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_v23 08/16] 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 f78ff458b0dd..3a0d76c40bcc 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"
@@ -30,22 +32,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)
@@ -342,7 +331,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_v23 09/16] selftests/x86/sgx: Add helper function and macros to assert results

[Sean Christopherson]

Borrow code and ideas from the KVM selftests for asserting and reporting
test results and failures.  Update the existing test assertions to use
the new functionality.  Defer other updates, e.g. error handling, to
future patches.

Change the license to GPL-2.0-only to accommodate the borrowed code.

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

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 3a0d76c40bcc..0c964bc1fca0 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -1,9 +1,10 @@
-// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// SPDX-License-Identifier: GPL-2.0-only
 // Copyright(c) 2016-18 Intel Corporation.
 
 #include <elf.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <stdarg.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -24,6 +25,47 @@
 #define PAGE_SIZE  4096
 
 static const uint64_t MAGIC = 0x1122334455667788ULL;
+
+void __attribute__((noinline)) test_assert(bool exp, const char *exp_str,
+					   const char *file, unsigned int line,
+					   const char *fmt, ...)
+{
+	va_list ap;
+
+	if (exp)
+		return;
+
+	va_start(ap, fmt);
+
+	fprintf(stderr, "==== SGX Selftest Assertion Failure ====\n");
+	if (exp_str)
+		fprintf(stderr, "  %s:%u: %s\n", file, line, exp_str);
+	if (fmt) {
+		if (exp_str)
+			fputs("  ", stderr);
+		else
+			fprintf(stderr, "  %s:%u: ", file, line);
+		vfprintf(stderr, fmt, ap);
+		fputs("\n", stderr);
+	}
+	va_end(ap);
+	exit(1);
+}
+
+#define TEST_ASSERT(e, fmt, ...) \
+	test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
+
+#define ASSERT_EQ(a, b)							     \
+do {									     \
+	typeof(a) __a = (a);						     \
+	typeof(b) __b = (b);						     \
+	test_assert(__a == __b, NULL, __FILE__, __LINE__,		     \
+		    "%s == %s failed.\n"				     \
+		    "\t%s is %#lx\n"					     \
+		    "\t%s is %#lx",					     \
+		    #a, #b, #a, (unsigned long)__a, #b, (unsigned long)__b); \
+} while (0)
+
 void *eenter;
 
 struct vdso_symtab {
@@ -346,15 +388,11 @@ 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((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
 		 (void *)secs.base, &exception, NULL);
 
-	if (result != MAGIC) {
-		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
-		exit(1);
-	}
+	ASSERT_EQ(result, MAGIC);
 
-	printf("Output: 0x%lx\n", result);
+	printf("All tests passed!\n");
 	exit(0);
 }
-- 
2.22.0

[PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Sean Christopherson]

Use the recently added assertion framework to report errors and exit
instead of propagating the error back up the stack.  Using assertions
reduces code and provides more detailed error messages, and has no
downsides as all errors lead to exit(1) anyways, i.e. an assertion
isn't blocking forward progress.

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

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 0c964bc1fca0..5b7575a948ba 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -66,6 +66,17 @@ do {									     \
 		    #a, #b, #a, (unsigned long)__a, #b, (unsigned long)__b); \
 } while (0)
 
+#define ASSERT_NE(a, b)							     \
+do {									     \
+	typeof(a) __a = (a);						     \
+	typeof(b) __b = (b);						     \
+	test_assert(__a != __b, NULL, __FILE__, __LINE__,		     \
+		    "%s != %s failed.\n"				     \
+		    "\t%s is %#lx\n"					     \
+		    "\t%s is %#lx",					     \
+		    #a, #b, #a, (unsigned long)__a, #b, (unsigned long)__b); \
+} while (0)
+
 void *eenter;
 
 struct vdso_symtab {
@@ -103,23 +114,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)
@@ -157,7 +163,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;
@@ -173,10 +179,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);
 
@@ -186,16 +189,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;
+	TEST_ASSERT(!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;
@@ -212,15 +210,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;
+	TEST_ASSERT(!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;
@@ -231,13 +224,9 @@ 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;
-	}
+	TEST_ASSERT(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)
@@ -246,108 +235,72 @@ 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;
-	}
+	TEST_ASSERT(!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;
-	}
+	TEST_ASSERT(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;
-	}
+	TEST_ASSERT(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;
 }
 
-bool get_file_size(const char *path, off_t *bin_size)
+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;
+	TEST_ASSERT(!ret, "stat() %s failed: %s\n", path, strerror(errno));
+
+	TEST_ASSERT(sb.st_size && !(sb.st_size & 0xfff),
+		    "Invalid blob size: %llu", sb.st_size);
+
+	return sb.st_size;
 }
 
-bool encl_data_map(const char *path, void **bin, off_t *bin_size)
+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;
-	}
+	TEST_ASSERT(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);
+	TEST_ASSERT(bin != MAP_FAILED, "mmap() %s failed: %s\n",
+		    path, strerror(errno));
 
 	close(fd);
-	return true;
-
-err_out:
-	close(fd);
-	return false;
+	return bin;
 }
 
-bool load_sigstruct(const char *path, void *sigstruct)
+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;
-	}
+	TEST_ASSERT(fd >= 0, "open() %s failed: %s\n", path, strerror(errno));
+
+	nr_read = read(fd, sigstruct, sizeof(struct sgx_sigstruct));
+	TEST_ASSERT(nr_read == sizeof(struct sgx_sigstruct),
+		    "read() %s failed: %s\n", path, strerror(errno));
 
 	close(fd);
-	return true;
 }
 
 int sgx_call(void *rdi, void *rsi, long rdx, void *rcx, void *r8, void *r9,
@@ -365,28 +318,23 @@ int main(int argc, char *argv[], char *envp[])
 	void *bin;
 	void *addr;
 
-	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);
 
 	memset(&exception, 0, sizeof(exception));
 
 	addr = vdso_get_base_addr();
-	if (!addr)
-		exit(1);
+	ASSERT_NE(addr, NULL);
 
-	if (!vdso_get_symtab(addr, &symtab))
-		exit(1);
+	vdso_get_symtab(addr, &symtab);
 
 	eenter_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave");
-	if (!eenter_sym)
-		exit(1);
+	ASSERT_NE(eenter_sym, NULL);
+
 	eenter = addr + eenter_sym->st_value;
 
-	if (!encl_build(&secs, bin, bin_size, &sigstruct))
-		exit(1);
+	encl_build(&secs, bin, bin_size, &sigstruct);
 
 	sgx_call((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
 		 (void *)secs.base, &exception, NULL);
-- 
2.22.0

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Jarkko Sakkinen]

On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> Use the recently added assertion framework to report errors and exit
> instead of propagating the error back up the stack.  Using assertions
> reduces code and provides more detailed error messages, and has no
> downsides as all errors lead to exit(1) anyways, i.e. an assertion
> isn't blocking forward progress.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I'm also dropping all of this. Was too hazy with it because of rush last
week.

You shoud use EXCEPT_* macros instead of your home baked ones:

https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html

I don't know what you are talking about in this commit message.
"Recently added" tells me absolutely nothing. All I see that you
are adding your own ad hoc crap.

/Jarkko

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Jarkko Sakkinen]

On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > Use the recently added assertion framework to report errors and exit
> > instead of propagating the error back up the stack.  Using assertions
> > reduces code and provides more detailed error messages, and has no
> > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > isn't blocking forward progress.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I'm also dropping all of this. Was too hazy with it because of rush last
> week.
> 
> You shoud use EXCEPT_* macros instead of your home baked ones:
> 
> https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> 
> I don't know what you are talking about in this commit message.
> "Recently added" tells me absolutely nothing. All I see that you
> are adding your own ad hoc crap.

E.g.

1. WTH the new thing is.
2. Why is it overriding the macros already defined for kselftest
   (see the documentation).
3. Before vDSO commits please provide a patch set that does the
   migration with clear explanation what is going on.

/Jarkko

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Jarkko Sakkinen]

On Tue, Oct 15, 2019 at 01:24:08PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > > Use the recently added assertion framework to report errors and exit
> > > instead of propagating the error back up the stack.  Using assertions
> > > reduces code and provides more detailed error messages, and has no
> > > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > > isn't blocking forward progress.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > I'm also dropping all of this. Was too hazy with it because of rush last
> > week.
> > 
> > You shoud use EXCEPT_* macros instead of your home baked ones:
> > 
> > https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> > 
> > I don't know what you are talking about in this commit message.
> > "Recently added" tells me absolutely nothing. All I see that you
> > are adding your own ad hoc crap.
> 
> E.g.
> 
> 1. WTH the new thing is.
> 2. Why is it overriding the macros already defined for kselftest
>    (see the documentation).
> 3. Before vDSO commits please provide a patch set that does the
>    migration with clear explanation what is going on.

See kselftest_harness.h.

/Jarkko

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Jarkko Sakkinen]

On Tue, Oct 15, 2019 at 01:25:55PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 15, 2019 at 01:24:08PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > > > Use the recently added assertion framework to report errors and exit
> > > > instead of propagating the error back up the stack.  Using assertions
> > > > reduces code and provides more detailed error messages, and has no
> > > > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > > > isn't blocking forward progress.
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > 
> > > I'm also dropping all of this. Was too hazy with it because of rush last
> > > week.
> > > 
> > > You shoud use EXCEPT_* macros instead of your home baked ones:
> > > 
> > > https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> > > 
> > > I don't know what you are talking about in this commit message.
> > > "Recently added" tells me absolutely nothing. All I see that you
> > > are adding your own ad hoc crap.
> > 
> > E.g.
> > 
> > 1. WTH the new thing is.
> > 2. Why is it overriding the macros already defined for kselftest
> >    (see the documentation).
> > 3. Before vDSO commits please provide a patch set that does the
> >    migration with clear explanation what is going on.
> 
> See kselftest_harness.h.

For me this all seems like unnecessary stuff done just before patch set
release. It is no in anyway necessary for v23 or even for merging SGX to
mainline. You seriously cannot stuff like this being merged quickly. It
will take a number of days to discuss all this through. My mistake was
to merge it before enough consideration.

Sometimes it is better just to do the *necessary*. Especially when it
is time to release something (e.g. just the minimal changes for vDSO
stuff).

I replaced all of this with v22 selftest and I'm in the process of
adding EENTER call path and splitting that to two commits.

/Jarkko

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Sean Christopherson]

On Tue, Oct 15, 2019 at 02:03:48PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 15, 2019 at 01:25:55PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 15, 2019 at 01:24:08PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > > > > Use the recently added assertion framework to report errors and exit
> > > > > instead of propagating the error back up the stack.  Using assertions
> > > > > reduces code and provides more detailed error messages, and has no
> > > > > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > > > > isn't blocking forward progress.
> > > > > 
> > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > 
> > > > I'm also dropping all of this. Was too hazy with it because of rush last
> > > > week.
> > > > 
> > > > You shoud use EXCEPT_* macros instead of your home baked ones:
> > > > 
> > > > https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> > > > 
> > > > I don't know what you are talking about in this commit message.
> > > > "Recently added" tells me absolutely nothing. All I see that you
> > > > are adding your own ad hoc crap.
> > > 
> > > E.g.
> > > 
> > > 1. WTH the new thing is.
> > > 2. Why is it overriding the macros already defined for kselftest
> > >    (see the documentation).
> > > 3. Before vDSO commits please provide a patch set that does the
> > >    migration with clear explanation what is going on.
> > 
> > See kselftest_harness.h.
> 
> For me this all seems like unnecessary stuff done just before patch set
> release. It is no in anyway necessary for v23 or even for merging SGX to
> mainline. You seriously cannot stuff like this being merged quickly. It
> will take a number of days to discuss all this through.

I never requested that this be merged quickly.  I sent patches to improve
the coverage of the selftests and make them easier to debug, no more, no
less.

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Jarkko Sakkinen]

On Tue, Oct 15, 2019 at 09:27:53AM -0700, Sean Christopherson wrote:
> On Tue, Oct 15, 2019 at 02:03:48PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 15, 2019 at 01:25:55PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Oct 15, 2019 at 01:24:08PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> > > > > On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > > > > > Use the recently added assertion framework to report errors and exit
> > > > > > instead of propagating the error back up the stack.  Using assertions
> > > > > > reduces code and provides more detailed error messages, and has no
> > > > > > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > > > > > isn't blocking forward progress.
> > > > > > 
> > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > > 
> > > > > I'm also dropping all of this. Was too hazy with it because of rush last
> > > > > week.
> > > > > 
> > > > > You shoud use EXCEPT_* macros instead of your home baked ones:
> > > > > 
> > > > > https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> > > > > 
> > > > > I don't know what you are talking about in this commit message.
> > > > > "Recently added" tells me absolutely nothing. All I see that you
> > > > > are adding your own ad hoc crap.
> > > > 
> > > > E.g.
> > > > 
> > > > 1. WTH the new thing is.
> > > > 2. Why is it overriding the macros already defined for kselftest
> > > >    (see the documentation).
> > > > 3. Before vDSO commits please provide a patch set that does the
> > > >    migration with clear explanation what is going on.
> > > 
> > > See kselftest_harness.h.
> > 
> > For me this all seems like unnecessary stuff done just before patch set
> > release. It is no in anyway necessary for v23 or even for merging SGX to
> > mainline. You seriously cannot stuff like this being merged quickly. It
> > will take a number of days to discuss all this through.
> 
> I never requested that this be merged quickly.  I sent patches to improve
> the coverage of the selftests and make them easier to debug, no more, no
> less.

OK, sorry I was over the top with this. Sincere apologies from my
part :-)

/Jarkko

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Sean Christopherson]

On Tue, Oct 15, 2019 at 01:25:55PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 15, 2019 at 01:24:08PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > > > Use the recently added assertion framework to report errors and exit
> > > > instead of propagating the error back up the stack.  Using assertions
> > > > reduces code and provides more detailed error messages, and has no
> > > > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > > > isn't blocking forward progress.
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > 
> > > I'm also dropping all of this. Was too hazy with it because of rush last
> > > week.
> > > 
> > > You shoud use EXCEPT_* macros instead of your home baked ones:
> > > 
> > > https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> > > 
> > > I don't know what you are talking about in this commit message.
> > > "Recently added" tells me absolutely nothing. All I see that you
> > > are adding your own ad hoc crap.
> > 
> > E.g.
> > 
> > 1. WTH the new thing is.
> > 2. Why is it overriding the macros already defined for kselftest
> >    (see the documentation).
> > 3. Before vDSO commits please provide a patch set that does the
> >    migration with clear explanation what is going on.
> 
> See kselftest_harness.h.

I spent a bit of time looking at kselftest_harness.h.  It's simply not
designed to handle testing things like SGX and KVM where the nature of the
thing being tested requires a substantial amount of setup, or where a
non-trivial amount of work is done in a callback, e.g. the guest in KVM
and the vDSO callback in SGX.

And IMO "designed" is a strong word.  The macros are just someone else's
ad hoc code that got hoisted into the top-level selftests directory.

  commit 0b40808a10842131742b1646a465b877a277168a
  Author: Mickaël Salaün <mic@digikod.net>
  Date:   Fri May 26 20:43:56 2017 +0200

    selftests: Make test_harness.h more generally available

    The seccomp/test_harness.h file contains useful helpers to build tests.
    Moving it to the selftest directory should benefit to other test
    components.

    Keep seccomp maintainers for this file.

It's gained two non-seccomp users in the 2+ years since being hoisted, so
it's not exactly a kernel-wide standard.

  seccomp/seccomp_bpf.c:TEST_HARNESS_MAIN
  uevent/uevent_filtering.c:TEST_HARNESS_MAIN
  net/tls.c:TEST_HARNESS_MAIN

The SGX selftest can be massaged/butchered to work with kselftest_harness,
but the error messages are still unhelpful and the resulting code is a
mess.

What I do think makes sense is to use the ksft_test_* helpers that are
provided by kselftest.h.  I'd still like to add ASSERT_* macros to cut
down on the amount of boilerplate code, but the result should be much less
ad hoc than this version.

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Sean Christopherson]

On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > Use the recently added assertion framework to report errors and exit
> > instead of propagating the error back up the stack.  Using assertions
> > reduces code and provides more detailed error messages, and has no
> > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > isn't blocking forward progress.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I'm also dropping all of this. Was too hazy with it because of rush last
> week.
> 
> You shoud use EXCEPT_* macros instead of your home baked ones:
> 
> https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> 
> I don't know what you are talking about in this commit message.
> "Recently added" tells me absolutely nothing.

It's a fairly common way to refer to changes introduced in prior patches
of the same series.

> All I see that you are adding your own ad hoc crap.

Yo, same team.  A simple "hey, there's already macros for this!" would
suffice.

Re: [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions

[Jarkko Sakkinen]

On Tue, Oct 15, 2019 at 09:18:39AM -0700, Sean Christopherson wrote:
> On Tue, Oct 15, 2019 at 01:16:35PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 07, 2019 at 09:46:07PM -0700, Sean Christopherson wrote:
> > > Use the recently added assertion framework to report errors and exit
> > > instead of propagating the error back up the stack.  Using assertions
> > > reduces code and provides more detailed error messages, and has no
> > > downsides as all errors lead to exit(1) anyways, i.e. an assertion
> > > isn't blocking forward progress.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > I'm also dropping all of this. Was too hazy with it because of rush last
> > week.
> > 
> > You shoud use EXCEPT_* macros instead of your home baked ones:
> > 
> > https://www.kernel.org/doc/html/v4.15/dev-tools/kselftest.html
> > 
> > I don't know what you are talking about in this commit message.
> > "Recently added" tells me absolutely nothing.
> 
> It's a fairly common way to refer to changes introduced in prior patches
> of the same series.
> 
> > All I see that you are adding your own ad hoc crap.
> 
> Yo, same team.  A simple "hey, there's already macros for this!" would
> suffice.

Since apologies, was over the top with this.

/Jarkko

[PATCH for_v23 11/16] selftests/x86/sgx: Sanitize the types for sgx_call()'s input params

[Sean Christopherson]

Declare the unused registers for sgx_call() a 'long' instead of 'void *'
and add a comment to explain that @rdi and @rsi are declared as 'void *'
only because they're always used to pass pointers.  Since the registers
are pass-through values, 'long' is the more intuitive declaration.

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

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 5b7575a948ba..b7b32cf144f4 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -303,7 +303,8 @@ void load_sigstruct(const char *path, struct sgx_sigstruct *sigstruct)
 	close(fd);
 }
 
-int sgx_call(void *rdi, void *rsi, long rdx, void *rcx, void *r8, void *r9,
+/* Declare RDI and RSI as 'void *', they're always used to pass pointers. */
+int sgx_call(void *rdi, void *rsi, long rdx, long rcx, long r8, long r9,
 	     void *tcs, struct sgx_enclave_exception *ei, void *cb);
 
 int main(int argc, char *argv[], char *envp[])
@@ -336,8 +337,8 @@ int main(int argc, char *argv[], char *envp[])
 
 	encl_build(&secs, bin, bin_size, &sigstruct);
 
-	sgx_call((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
-		 (void *)secs.base, &exception, NULL);
+	sgx_call((void *)&MAGIC, &result, 0, 0, 0, 0, (void *)secs.base,
+		 &exception, NULL);
 
 	ASSERT_EQ(result, MAGIC);
 
-- 
2.22.0

[PATCH for_v23 12/16] selftests/x86/sgx: Move existing sub-test to a separate helper

[Sean Christopherson]

Move the basic test of running the enclave using the vDSO to a separate
helper in preparation for introducing new sub-tests and variations on
the existing test.

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

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index b7b32cf144f4..93b8d7781782 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -307,14 +307,28 @@ void load_sigstruct(const char *path, struct sgx_sigstruct *sigstruct)
 int sgx_call(void *rdi, void *rsi, long rdx, long rcx, long r8, long r9,
 	     void *tcs, struct sgx_enclave_exception *ei, void *cb);
 
-int main(int argc, char *argv[], char *envp[])
+/*
+ * Test the vDSO API, __vdso_sgx_enter_enclave(), without an exit handler.
+ */
+static void test_vdso_no_exit_handler(struct sgx_secs *secs)
 {
 	struct sgx_enclave_exception exception;
+	uint64_t result = 0;
+
+	memset(&exception, 0, sizeof(exception));
+
+	/* Verify the enclave copies MAGIC to result. */
+	sgx_call((void *)&MAGIC, &result, 0, 0, 0, 0, (void *)secs->base,
+		 &exception, NULL);
+	ASSERT_EQ(result, MAGIC);
+}
+
+int main(int argc, char *argv[], char *envp[])
+{
 	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 *bin;
 	void *addr;
@@ -323,8 +337,6 @@ int main(int argc, char *argv[], char *envp[])
 
 	load_sigstruct("encl.ss", &sigstruct);
 
-	memset(&exception, 0, sizeof(exception));
-
 	addr = vdso_get_base_addr();
 	ASSERT_NE(addr, NULL);
 
@@ -337,10 +349,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	encl_build(&secs, bin, bin_size, &sigstruct);
 
-	sgx_call((void *)&MAGIC, &result, 0, 0, 0, 0, (void *)secs.base,
-		 &exception, NULL);
-
-	ASSERT_EQ(result, MAGIC);
+	test_vdso_no_exit_handler(&secs);
 
 	printf("All tests passed!\n");
 	exit(0);
-- 
2.22.0

[PATCH for_v23 13/16] selftests/x86/sgx: Add a test of the vDSO exception reporting mechanism

[Sean Christopherson]

Add a sub-test to verify that an exception on EENTER is correctly
reported.  Although the type of exception doesn't truly matter, e.g. a
page fault (#PF) is no more or less interesting than a general protection
fault (#GP), use an unaligned TCS to trigger a #GP to avoid errors 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 3ff73a9d9b93..ab9671b8a993 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 /* TYPES_H */
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 93b8d7781782..2676570493f2 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -321,6 +321,12 @@ static void test_vdso_no_exit_handler(struct sgx_secs *secs)
 	sgx_call((void *)&MAGIC, &result, 0, 0, 0, 0, (void *)secs->base,
 		 &exception, NULL);
 	ASSERT_EQ(result, MAGIC);
+
+	/* Verify a #GP is reported if the TCS isn't 4k aligned. */
+	sgx_call((void *)&MAGIC, &result, 0, 0, 0, 0,
+		 (void *)(secs->base | 0xfff), &exception, NULL);
+	ASSERT_EQ(exception.trapnr, GP_VECTOR);
+	ASSERT_EQ(exception.leaf, ENCLU_EENTER);
 }
 
 int main(int argc, char *argv[], char *envp[])
-- 
2.22.0

[PATCH for_v23 14/16] 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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 2676570493f2..ae1822b10c6f 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -329,6 +329,31 @@ static void test_vdso_no_exit_handler(struct sgx_secs *secs)
 	ASSERT_EQ(exception.leaf, ENCLU_EENTER);
 }
 
+static int basic_exit_handler(long rdi, long rsi, long rdx, int ret,
+			      long r8, long r9, void *tcs, long ursp,
+			      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_vdso_with_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((void *)&MAGIC, &result, 0, 0, 0, 0, (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;
@@ -356,6 +381,7 @@ int main(int argc, char *argv[], char *envp[])
 	encl_build(&secs, bin, bin_size, &sigstruct);
 
 	test_vdso_no_exit_handler(&secs);
+	test_vdso_with_exit_handler(&secs);
 
 	printf("All tests passed!\n");
 	exit(0);
-- 
2.22.0

[PATCH for_v23 15/16] selftests/x86/sgx: Add sub-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 read-only and iteratively fix
the faults encountered, with various assertions along the way, e.g. the
first fault should always be a !writable fault 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    | 87 ++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/defines.h b/tools/testing/selftests/x86/sgx/defines.h
index ab9671b8a993..199a830e198a 100644
--- a/tools/testing/selftests/x86/sgx/defines.h
+++ b/tools/testing/selftests/x86/sgx/defines.h
@@ -37,7 +37,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 /* TYPES_H */
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index ae1822b10c6f..8c3f0cd41098 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -337,14 +337,58 @@ static int basic_exit_handler(long rdi, long rsi, long rdx, int ret,
 	return 0;
 }
 
+static int nr_page_faults;
+
+static int mprotect_exit_handler(long rdi, long rsi, long rdx, int ret,
+				 long r8, long r9, void *tcs, long ursp,
+				 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);
+	TEST_ASSERT(e->leaf == ENCLU_EENTER || e->leaf == ENCLU_ERESUME,
+		    "Expected #PF on EENTER or ERESUME, leaf = %d\n", e->leaf);
+	TEST_ASSERT(e->error_code & 1, "Unexpected !PRESENT #PF");
+
+	/* The first #PF should be on the TCS, passed in via R9. */
+	if (nr_page_faults == 1)
+		ASSERT_EQ(r9, (e->address & ~0xfff));
+
+	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;
+}
+
 /*
  * Test the vDSO API, __vdso_sgx_enter_enclave(), with an exit handler.
  */
-static void test_vdso_with_exit_handler(struct sgx_secs *secs)
+static void test_vdso_with_exit_handler(struct sgx_secs *secs,
+					unsigned long encl_size)
 {
 	struct sgx_enclave_exception exception;
 	uint64_t result = 0;
 	long ret;
+	int r;
 
 	memset(&exception, 0, sizeof(exception));
 
@@ -352,6 +396,45 @@ static void test_vdso_with_exit_handler(struct sgx_secs *secs)
 		       &exception, basic_exit_handler);
 	ASSERT_EQ(ret, 0);
 	ASSERT_EQ(result, MAGIC);
+
+	/*
+	 * Map the enclave read-only, then re-enter the enclave.  The exit
+	 * handler will service the resulting page faults using mprotect() to
+	 * restore the correct permissions.
+	 */
+	r = mprotect((void *)secs->base, encl_size, PROT_READ);
+	TEST_ASSERT(!r, "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 (which
+		 * requires an accessible TCS page).
+		 */
+		ret = sgx_call((void *)&MAGIC, &result, 0, 0, 0, secs->base,
+			       (void *)secs->base, &exception,
+			       mprotect_exit_handler);
+	} while (ret == -EAGAIN);
+	ASSERT_EQ(ret, 0);
+	ASSERT_EQ(result, MAGIC);
+
+	/* Enclave should re-execute cleanly. */
+	result = 0;
+	ret = sgx_call((void *)&MAGIC, &result, 0, 0, 0, 0, (void *)secs->base,
+		       &exception, basic_exit_handler);
+	ASSERT_EQ(ret, 0);
+	ASSERT_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).
+	 */
+	TEST_ASSERT(nr_page_faults >= 3, "Expected 3+ page faults, only hit %d",
+		    nr_page_faults);
 }
 
 int main(int argc, char *argv[], char *envp[])
@@ -381,7 +464,7 @@ int main(int argc, char *argv[], char *envp[])
 	encl_build(&secs, bin, bin_size, &sigstruct);
 
 	test_vdso_no_exit_handler(&secs);
-	test_vdso_with_exit_handler(&secs);
+	test_vdso_with_exit_handler(&secs, bin_size);
 
 	printf("All tests passed!\n");
 	exit(0);
-- 
2.22.0

[PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Sean Christopherson]

Rework __vdso_sgx_enter_enclave() to prioritize the flow where userspace
is not providing a callback, which is the preferred method of operation.
Using a callback requires a retpoline, and the only known motivation for
employing a callback is to allow the enclave to muck with the stack of
the untrusted runtime.

Opportunistically replace the majority of the local labels with local
symbol names to improve the readability of the code.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 120 ++++++++++++++---------
 1 file changed, 71 insertions(+), 49 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index de54e47c83f4..fc5622dcd2fa 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -85,75 +85,97 @@ ENTRY(__vdso_sgx_enter_enclave)
 	mov	%rsp, %rbp
 	.cfi_def_cfa_register	%rbp
 
-1:	/* EENTER <= leaf <= ERESUME */
+.Lenter_enclave:
+	/* EENTER <= leaf <= ERESUME */
 	cmp	$0x2, %eax
-	jb	6f
+	jb	.Linvalid_leaf
 	cmp	$0x3, %eax
-	ja	6f
+	ja	.Linvalid_leaf
 
 	/* Load TCS and AEP */
 	mov	0x10(%rbp), %rbx
-	lea	2f(%rip), %rcx
+	lea	.Lasync_exit_pointer(%rip), %rcx
 
 	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
-2:	enclu
+.Lasync_exit_pointer:
+.Lenclu_eenter_eresume:
+	enclu
 
-	/* EEXIT path */
+	/* EEXIT jumps here unless the enclave is doing something fancy. */
 	xor	%eax, %eax
-3:	mov	%eax, %ecx
-
-	/* Call the exit handler if supplied */
-	mov	0x20(%rbp), %rax
-	test	%rax, %rax
-	jz	7f
-	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
-	 * restored after the exit handler returns. */
+
+	/* Invoke userspace's exit handler if one was provided. */
+.Lhandle_exit:
+	cmp	$0, 0x20(%rbp)
+	jne	.Linvoke_userspace_handler
+
+.Lout:
+	leave
+	.cfi_def_cfa		%rsp, 8
+	ret
+
+.Linvalid_leaf:
+	mov	$(-EINVAL), %eax
+	jmp	.Lout
+
+.Lhandle_exception:
+	mov	0x18(%rbp), %rcx
+	test    %rcx, %rcx
+	je	.Lskip_exception_info
+
+	/* Fill optional exception info. */
+	mov	%eax, EX_LEAF(%rcx)
+	mov	%di,  EX_TRAPNR(%rcx)
+	mov	%si,  EX_ERROR_CODE(%rcx)
+	mov	%rdx, EX_ADDRESS(%rcx)
+.Lskip_exception_info:
+	mov	$(-EFAULT), %eax
+	jmp	.Lhandle_exit
+
+.Linvoke_userspace_handler:
+	/*
+	 * Align stack per x86_64 ABI. Save the original %rsp in %rbx to be
+	 * restored after the callback returns.
+	 */
 	mov	%rsp, %rbx
 	and	$-0x10, %rsp
-	/* Clear RFLAGS.DF per x86_64 ABI */
-	cld
-	/* Parameters for the exit handler */
+
+	/* Push @e, u_rsp and @tcs as parameters to the callback. */
 	push	0x18(%rbp)
 	push	%rbx
 	push	0x10(%rbp)
-	/* Call *%rax via retpoline */
-	call	40f
-	/* Restore %rsp to its original value left off by the enclave from last
-	 * exit */
+
+	/* Pass the "return" value to the callback via %rcx. */
+	mov	%eax, %ecx
+
+	/* Clear RFLAGS.DF per x86_64 ABI */
+	cld
+
+	/* Load the callback pointer to %rax and invoke it via retpoline. */
+	mov	0x20(%rbp), %rax
+	call	.Lretpoline
+
+	/* Restore %rsp to its post-exit value. */
 	mov	%rbx, %rsp
-	/* Positive return value from the exit handler will be interpreted as
-	 * an ENCLU leaf, while a non-positive value will be interpreted as the
-	 * return value to be passed back to the caller. */
-	jmp	1b
-40:	/* retpoline */
-	call	42f
-41:	pause
-	lfence
-	jmp	41b
-42:	mov	%rax, (%rsp)
-	ret
 
-5:	/* Exception path */
-	mov	0x18(%rbp), %rcx
-	jrcxz	52f
-	mov	%eax, EX_LEAF(%rcx)
-	mov	%di,  EX_TRAPNR(%rcx)
-	mov	%si,  EX_ERROR_CODE(%rcx)
-	mov	%rdx, EX_ADDRESS(%rcx)
-52:	mov	$-EFAULT, %eax
-	jmp	3b
-
-6:	/* Unsupported ENCLU leaf */
+	/*
+	 * If the return from callback is zero or negative, return immediately,
+	 * else re-execute ENCLU with the postive return value interpreted as
+	 * the requested ENCLU leaf.
+	 */
 	cmp	$0, %eax
-	jle	7f
-	mov	$-EINVAL, %eax
+	jle	.Lout
+	jmp	.Lenter_enclave
 
-7:	/* Epilog */
-	leave
-	.cfi_def_cfa		%rsp, 8
+.Lretpoline:
+	call	2f
+1:	pause
+	lfence
+	jmp	1b
+2:	mov	%rax, (%rsp)
 	ret
 	.cfi_endproc
 
-_ASM_VDSO_EXTABLE_HANDLE(2b, 5b)
+_ASM_VDSO_EXTABLE_HANDLE(.Lenclu_eenter_eresume, .Lhandle_exception)
 
 ENDPROC(__vdso_sgx_enter_enclave)
-- 
2.22.0

Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Xing, Cedric]

On 10/7/2019 9:46 PM, Sean Christopherson wrote:
> Rework __vdso_sgx_enter_enclave() to prioritize the flow where userspace
> is not providing a callback, which is the preferred method of operation.

Processors have branch predictors so your "prioritizing" may not get 
what your want.

But if you still insist, a simple ht/hnt prefixing the original branch 
instruction would have sufficed. Rearrangement of code blocks is indeed 
unnecessary.

A caveat though, for any given process, whether it supplies a callback 
or not is usually hard-coded. So either it always takes the callback 
path, or it always takes the other. And the branch predictor will do 
well in both cases. It's usually unwise to apply ht/hnt prefixes.

> Using a callback requires a retpoline, and the only known motivation for
> employing a callback is to allow the enclave to muck with the stack of
> the untrusted runtime.
> 
> Opportunistically replace the majority of the local labels with local
> symbol names to improve the readability of the code.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/entry/vdso/vsgx_enter_enclave.S | 120 ++++++++++++++---------
>   1 file changed, 71 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index de54e47c83f4..fc5622dcd2fa 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -85,75 +85,97 @@ ENTRY(__vdso_sgx_enter_enclave)
>   	mov	%rsp, %rbp
>   	.cfi_def_cfa_register	%rbp
>   
> -1:	/* EENTER <= leaf <= ERESUME */
> +.Lenter_enclave:
> +	/* EENTER <= leaf <= ERESUME */
>   	cmp	$0x2, %eax
> -	jb	6f
> +	jb	.Linvalid_leaf
>   	cmp	$0x3, %eax
> -	ja	6f
> +	ja	.Linvalid_leaf
>   
>   	/* Load TCS and AEP */
>   	mov	0x10(%rbp), %rbx
> -	lea	2f(%rip), %rcx
> +	lea	.Lasync_exit_pointer(%rip), %rcx
>   
>   	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> -2:	enclu
> +.Lasync_exit_pointer:
> +.Lenclu_eenter_eresume:
> +	enclu
>   
> -	/* EEXIT path */
> +	/* EEXIT jumps here unless the enclave is doing something fancy. */
>   	xor	%eax, %eax
> -3:	mov	%eax, %ecx
> -
> -	/* Call the exit handler if supplied */
> -	mov	0x20(%rbp), %rax
> -	test	%rax, %rax
> -	jz	7f

> -	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
> -	 * restored after the exit handler returns. */
> +
> +	/* Invoke userspace's exit handler if one was provided. */
> +.Lhandle_exit:
> +	cmp	$0, 0x20(%rbp)
> +	jne	.Linvoke_userspace_handler
> +
> +.Lout:
> +	leave
> +	.cfi_def_cfa		%rsp, 8
> +	ret
> +
> +.Linvalid_leaf:

Please set frame pointer back to %rbp here, or stack unwinding will fail.

> +	mov	$(-EINVAL), %eax
> +	jmp	.Lout
> +
> +.Lhandle_exception:
> +	mov	0x18(%rbp), %rcx
> +	test    %rcx, %rcx
> +	je	.Lskip_exception_info

A single "jrcxz .Lskip_exception_info" is equivalent to the above 2 
instructions combined.

> +
> +	/* Fill optional exception info. */
> +	mov	%eax, EX_LEAF(%rcx)
> +	mov	%di,  EX_TRAPNR(%rcx)
> +	mov	%si,  EX_ERROR_CODE(%rcx)
> +	mov	%rdx, EX_ADDRESS(%rcx)
> +.Lskip_exception_info:
> +	mov	$(-EFAULT), %eax
> +	jmp	.Lhandle_exit
> +
> +.Linvoke_userspace_handler:
> +	/*
> +	 * Align stack per x86_64 ABI. Save the original %rsp in %rbx to be
> +	 * restored after the callback returns.
> +	 */
>   	mov	%rsp, %rbx
>   	and	$-0x10, %rsp
> -	/* Clear RFLAGS.DF per x86_64 ABI */
> -	cld
> -	/* Parameters for the exit handler */
> +
> +	/* Push @e, u_rsp and @tcs as parameters to the callback. */
>   	push	0x18(%rbp)
>   	push	%rbx
>   	push	0x10(%rbp)
> -	/* Call *%rax via retpoline */
> -	call	40f
> -	/* Restore %rsp to its original value left off by the enclave from last
> -	 * exit */
> +
> +	/* Pass the "return" value to the callback via %rcx. */
> +	mov	%eax, %ecx

@e (ex_info) is almost always needed by every callback as it also serves 
as the "context pointer". The return value on the other hand is 
insignificant because it could be deduced from @e->EX_LEAF anyway. So 
I'd retain %rcx and push %rax to the stack instead, given the purpose of 
this patch is to squeeze out a bit performance.

> +
> +	/* Clear RFLAGS.DF per x86_64 ABI */
> +	cld
> +
> +	/* Load the callback pointer to %rax and invoke it via retpoline. */
> +	mov	0x20(%rbp), %rax

Per X86_64 ABI, %rsp shall be 16 bytes aligned before "call". But %rsp 
here doesn't look aligned properly.

> +	call	.Lretpoline
> +
> +	/* Restore %rsp to its post-exit value. */
>   	mov	%rbx, %rsp
> -	/* Positive return value from the exit handler will be interpreted as
> -	 * an ENCLU leaf, while a non-positive value will be interpreted as the
> -	 * return value to be passed back to the caller. */
> -	jmp	1b
> -40:	/* retpoline */
> -	call	42f
> -41:	pause
> -	lfence
> -	jmp	41b
> -42:	mov	%rax, (%rsp)
> -	ret
>   
> -5:	/* Exception path */
> -	mov	0x18(%rbp), %rcx
> -	jrcxz	52f
> -	mov	%eax, EX_LEAF(%rcx)
> -	mov	%di,  EX_TRAPNR(%rcx)
> -	mov	%si,  EX_ERROR_CODE(%rcx)
> -	mov	%rdx, EX_ADDRESS(%rcx)
> -52:	mov	$-EFAULT, %eax
> -	jmp	3b
> -
> -6:	/* Unsupported ENCLU leaf */
> +	/*
> +	 * If the return from callback is zero or negative, return immediately,
> +	 * else re-execute ENCLU with the postive return value interpreted as
> +	 * the requested ENCLU leaf.
> +	 */
>   	cmp	$0, %eax
> -	jle	7f
> -	mov	$-EINVAL, %eax
> +	jle	.Lout
> +	jmp	.Lenter_enclave
>   
> -7:	/* Epilog */
> -	leave
> -	.cfi_def_cfa		%rsp, 8
> +.Lretpoline:
> +	call	2f
> +1:	pause
> +	lfence
> +	jmp	1b
> +2:	mov	%rax, (%rsp)
>   	ret
>   	.cfi_endproc
>   
> -_ASM_VDSO_EXTABLE_HANDLE(2b, 5b)
> +_ASM_VDSO_EXTABLE_HANDLE(.Lenclu_eenter_eresume, .Lhandle_exception)
>   
>   ENDPROC(__vdso_sgx_enter_enclave)
> 

Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Sean Christopherson]

On Wed, Oct 09, 2019 at 11:00:55AM -0700, Xing, Cedric wrote:
> On 10/7/2019 9:46 PM, Sean Christopherson wrote:
> >Rework __vdso_sgx_enter_enclave() to prioritize the flow where userspace
> >is not providing a callback, which is the preferred method of operation.
> 
> Processors have branch predictors so your "prioritizing" may not get what
> your want.
> 
> But if you still insist, a simple ht/hnt prefixing the original branch
> instruction would have sufficed. Rearrangement of code blocks is indeed
> unnecessary.

Tying into the other thread, it's not a matter of absolute necessity, it's
a matter of us providing the best code possible.

> A caveat though, for any given process, whether it supplies a callback or
> not is usually hard-coded. So either it always takes the callback path, or
> it always takes the other. And the branch predictor will do well in both
> cases. It's usually unwise to apply ht/hnt prefixes.

Readability is the primary concern, performance is secondary concern.  For
a random joe user, this:

   0x0000000000000a20 <+0>:	push   %rbp
   0x0000000000000a21 <+1>:	mov    %rsp,%rbp
   0x0000000000000a24 <+4>:	cmp    $0x2,%eax
   0x0000000000000a27 <+7>:	jb     0xa46 <__vdso_sgx_enter_enclave+38>
   0x0000000000000a29 <+9>:	cmp    $0x3,%eax
   0x0000000000000a2c <+12>:	ja     0xa46 <__vdso_sgx_enter_enclave+38>
   0x0000000000000a2e <+14>:	mov    0x10(%rbp),%rbx
   0x0000000000000a32 <+18>:	lea    0x0(%rip),%rcx        # 0xa39 <__vdso_sgx_enter_enclave+25>
   0x0000000000000a39 <+25>:	enclu  
   0x0000000000000a3c <+28>:	xor    %eax,%eax
   0x0000000000000a3e <+30>:	cmpl   $0x0,0x20(%rbp)
   0x0000000000000a42 <+34>:	jne    0xa6b <__vdso_sgx_enter_enclave+75>
   0x0000000000000a44 <+36>:	leaveq 
   0x0000000000000a45 <+37>:	retq   

is easier to follow for the happy path than this:

   0x0000000000000a20 <+0>:	push   %rbp
   0x0000000000000a21 <+1>:	mov    %rsp,%rbp
   0x0000000000000a24 <+4>:	cmp    $0x2,%eax
   0x0000000000000a27 <+7>:	jb     0xa8e <__vdso_sgx_enter_enclave+110>
   0x0000000000000a29 <+9>:	cmp    $0x3,%eax
   0x0000000000000a2c <+12>:	ja     0xa8e <__vdso_sgx_enter_enclave+110>
   0x0000000000000a2e <+14>:	mov    0x10(%rbp),%rbx
   0x0000000000000a32 <+18>:	lea    0x0(%rip),%rcx        # 0xa39 <__vdso_sgx_enter_enclave+25>
   0x0000000000000a39 <+25>:	enclu  
   0x0000000000000a3c <+28>:	xor    %eax,%eax
   0x0000000000000a3e <+30>:	mov    %eax,%ecx
   0x0000000000000a40 <+32>:	mov    0x20(%rbp),%rax
   0x0000000000000a44 <+36>:	test   %rax,%rax
   0x0000000000000a47 <+39>:	je     0xa98 <__vdso_sgx_enter_enclave+120>
   0x0000000000000a49 <+41>:	mov    %rsp,%rbx
   0x0000000000000a4c <+44>:	and    $0xfffffffffffffff0,%rsp
   0x0000000000000a50 <+48>:	cld    
   0x0000000000000a51 <+49>:	pushq  0x18(%rbp)
   0x0000000000000a54 <+52>:	push   %rbx
   0x0000000000000a55 <+53>:	pushq  0x10(%rbp)
   0x0000000000000a58 <+56>:	callq  0xa62 <__vdso_sgx_enter_enclave+66>
   0x0000000000000a5d <+61>:	mov    %rbx,%rsp
   0x0000000000000a60 <+64>:	jmp    0xa24 <__vdso_sgx_enter_enclave+4>
   0x0000000000000a62 <+66>:	callq  0xa6e <__vdso_sgx_enter_enclave+78>
   0x0000000000000a67 <+71>:	pause  
   0x0000000000000a69 <+73>:	lfence 
   0x0000000000000a6c <+76>:	jmp    0xa67 <__vdso_sgx_enter_enclave+71>
   0x0000000000000a6e <+78>:	mov    %rax,(%rsp)
   0x0000000000000a72 <+82>:	retq   
   0x0000000000000a73 <+83>:	mov    0x18(%rbp),%rcx
   0x0000000000000a77 <+87>:	jrcxz  0xa87 <__vdso_sgx_enter_enclave+103>
   0x0000000000000a79 <+89>:	mov    %eax,(%rcx)
   0x0000000000000a7b <+91>:	mov    %di,0x4(%rcx)
   0x0000000000000a7f <+95>:	mov    %si,0x6(%rcx)
   0x0000000000000a83 <+99>:	mov    %rdx,0x8(%rcx)
   0x0000000000000a87 <+103>:	mov    $0xfffffff2,%eax
   0x0000000000000a8c <+108>:	jmp    0xa3e <__vdso_sgx_enter_enclave+30>
   0x0000000000000a8e <+110>:	cmp    $0x0,%eax
   0x0000000000000a91 <+113>:	jle    0xa98 <__vdso_sgx_enter_enclave+120>
   0x0000000000000a93 <+115>:	mov    $0xffffffea,%eax
   0x0000000000000a98 <+120>:	leaveq 
   0x0000000000000a99 <+121>:	retq  

and much easier to follow than the version where the exception struct is
filled in on a synchronous EEXIT:

   0x0000000000000a20 <+0>:	push   %rbp
   0x0000000000000a21 <+1>:	mov    %rsp,%rbp
   0x0000000000000a24 <+4>:	cmp    $0x2,%eax
   0x0000000000000a27 <+7>:	jb     0xa90 <__vdso_sgx_enter_enclave+112>
   0x0000000000000a29 <+9>:	cmp    $0x3,%eax
   0x0000000000000a2c <+12>:	ja     0xa90 <__vdso_sgx_enter_enclave+112>
   0x0000000000000a2e <+14>:	mov    0x10(%rbp),%rbx
   0x0000000000000a32 <+18>:	lea    0x0(%rip),%rcx        # 0xa39 <__vdso_sgx_enter_enclave+25>
   0x0000000000000a39 <+25>:	enclu  
   0x0000000000000a3c <+28>:	xor    %ebx,%ebx
   0x0000000000000a3e <+30>:	mov    0x18(%rbp),%rcx
   0x0000000000000a42 <+34>:	jrcxz  0xa54 <__vdso_sgx_enter_enclave+52>
   0x0000000000000a44 <+36>:	mov    %eax,(%rcx)
   0x0000000000000a46 <+38>:	jae    0xa54 <__vdso_sgx_enter_enclave+52>
   0x0000000000000a48 <+40>:	mov    %di,0x4(%rcx)
   0x0000000000000a4c <+44>:	mov    %si,0x6(%rcx)
   0x0000000000000a50 <+48>:	mov    %rdx,0x8(%rcx)
   0x0000000000000a54 <+52>:	mov    0x20(%rbp),%rax
   0x0000000000000a58 <+56>:	test   %rax,%rax
   0x0000000000000a5b <+59>:	cmove  %rbx,%rax
   0x0000000000000a5f <+63>:	je     0xa9a <__vdso_sgx_enter_enclave+122>

   ...

   0x0000000000000a9a <+122>:	leaveq 
   0x0000000000000a9b <+123>:	retq   


> >Using a callback requires a retpoline, and the only known motivation for
> >employing a callback is to allow the enclave to muck with the stack of
> >the untrusted runtime.
> >
> >Opportunistically replace the majority of the local labels with local
> >symbol names to improve the readability of the code.
> >
> >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >---
> >  arch/x86/entry/vdso/vsgx_enter_enclave.S | 120 ++++++++++++++---------
> >  1 file changed, 71 insertions(+), 49 deletions(-)
> >
> >diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> >index de54e47c83f4..fc5622dcd2fa 100644
> >--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> >+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> >@@ -85,75 +85,97 @@ ENTRY(__vdso_sgx_enter_enclave)
> >  	mov	%rsp, %rbp
> >  	.cfi_def_cfa_register	%rbp
> >-1:	/* EENTER <= leaf <= ERESUME */
> >+.Lenter_enclave:
> >+	/* EENTER <= leaf <= ERESUME */
> >  	cmp	$0x2, %eax
> >-	jb	6f
> >+	jb	.Linvalid_leaf
> >  	cmp	$0x3, %eax
> >-	ja	6f
> >+	ja	.Linvalid_leaf
> >  	/* Load TCS and AEP */
> >  	mov	0x10(%rbp), %rbx
> >-	lea	2f(%rip), %rcx
> >+	lea	.Lasync_exit_pointer(%rip), %rcx
> >  	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> >-2:	enclu
> >+.Lasync_exit_pointer:
> >+.Lenclu_eenter_eresume:
> >+	enclu
> >-	/* EEXIT path */
> >+	/* EEXIT jumps here unless the enclave is doing something fancy. */
> >  	xor	%eax, %eax
> >-3:	mov	%eax, %ecx
> >-
> >-	/* Call the exit handler if supplied */
> >-	mov	0x20(%rbp), %rax
> >-	test	%rax, %rax
> >-	jz	7f
> 
> >-	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
> >-	 * restored after the exit handler returns. */
> >+
> >+	/* Invoke userspace's exit handler if one was provided. */
> >+.Lhandle_exit:
> >+	cmp	$0, 0x20(%rbp)
> >+	jne	.Linvoke_userspace_handler
> >+
> >+.Lout:
> >+	leave
> >+	.cfi_def_cfa		%rsp, 8
> >+	ret
> >+
> >+.Linvalid_leaf:
> 
> Please set frame pointer back to %rbp here, or stack unwinding will fail.

Sorry, coffee isn't doing it's job, what's getting crushed, and where?

> >+	mov	$(-EINVAL), %eax
> >+	jmp	.Lout
> >+
> >+.Lhandle_exception:
> >+	mov	0x18(%rbp), %rcx
> >+	test    %rcx, %rcx
> >+	je	.Lskip_exception_info
> 
> A single "jrcxz .Lskip_exception_info" is equivalent to the above 2
> instructions combined.

Both implementations take a single uop on CPUs that support SGX.  IMO,
using the simpler and more common instructions is more universally
readable.

> >+
> >+	/* Fill optional exception info. */
> >+	mov	%eax, EX_LEAF(%rcx)
> >+	mov	%di,  EX_TRAPNR(%rcx)
> >+	mov	%si,  EX_ERROR_CODE(%rcx)
> >+	mov	%rdx, EX_ADDRESS(%rcx)
> >+.Lskip_exception_info:
> >+	mov	$(-EFAULT), %eax
> >+	jmp	.Lhandle_exit
> >+
> >+.Linvoke_userspace_handler:
> >+	/*
> >+	 * Align stack per x86_64 ABI. Save the original %rsp in %rbx to be
> >+	 * restored after the callback returns.
> >+	 */
> >  	mov	%rsp, %rbx
> >  	and	$-0x10, %rsp
> >-	/* Clear RFLAGS.DF per x86_64 ABI */
> >-	cld
> >-	/* Parameters for the exit handler */
> >+
> >+	/* Push @e, u_rsp and @tcs as parameters to the callback. */
> >  	push	0x18(%rbp)
> >  	push	%rbx
> >  	push	0x10(%rbp)
> >-	/* Call *%rax via retpoline */
> >-	call	40f
> >-	/* Restore %rsp to its original value left off by the enclave from last
> >-	 * exit */
> >+
> >+	/* Pass the "return" value to the callback via %rcx. */
> >+	mov	%eax, %ecx
> 
> @e (ex_info) is almost always needed by every callback as it also serves as
> the "context pointer". The return value on the other hand is insignificant
> because it could be deduced from @e->EX_LEAF anyway. So I'd retain %rcx and
> push %rax to the stack instead, given the purpose of this patch is to
> squeeze out a bit performance.

Please take this up in patch 02/16, which actually introduced this change.
> 
> >+
> >+	/* Clear RFLAGS.DF per x86_64 ABI */
> >+	cld
> >+
> >+	/* Load the callback pointer to %rax and invoke it via retpoline. */
> >+	mov	0x20(%rbp), %rax
> 
> Per X86_64 ABI, %rsp shall be 16 bytes aligned before "call". But %rsp here
> doesn't look aligned properly.

Argh, I probably botched it back in patch 02/16 too.  I'll see if I can
add a check to verify %rsp alignment in the selftest, verifying via code
inspection is bound to be error prone.

> >+	call	.Lretpoline

Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Sean Christopherson]

On Wed, Oct 09, 2019 at 12:10:03PM -0700, Sean Christopherson wrote:
> > >+
> > >+	/* Clear RFLAGS.DF per x86_64 ABI */
> > >+	cld
> > >+
> > >+	/* Load the callback pointer to %rax and invoke it via retpoline. */
> > >+	mov	0x20(%rbp), %rax
> > 
> > Per X86_64 ABI, %rsp shall be 16 bytes aligned before "call". But %rsp here
> > doesn't look aligned properly.
> 
> Argh, I probably botched it back in patch 02/16 too.  I'll see if I can
> add a check to verify %rsp alignment in the selftest, verifying via code
> inspection is bound to be error prone.

Added a selftest, stack is indeed not properly aligned.

Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Xing, Cedric]

On 10/9/2019 12:10 PM, Sean Christopherson wrote:
> On Wed, Oct 09, 2019 at 11:00:55AM -0700, Xing, Cedric wrote:
>> On 10/7/2019 9:46 PM, Sean Christopherson wrote:
>>> -	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
>>> -	 * restored after the exit handler returns. */
>>> +
>>> +	/* Invoke userspace's exit handler if one was provided. */
>>> +.Lhandle_exit:
>>> +	cmp	$0, 0x20(%rbp)
>>> +	jne	.Linvoke_userspace_handler
>>> +
>>> +.Lout:
>>> +	leave
>>> +	.cfi_def_cfa		%rsp, 8
>>> +	ret
>>> +
>>> +.Linvalid_leaf:
>>
>> Please set frame pointer back to %rbp here, or stack unwinding will fail.
> 
> Sorry, coffee isn't doing it's job, what's getting crushed, and where?

The frame pointer was %rbp but you changed it to %rsp 3 lines ago. 
That's correct after "leave" and execution won't pass "ret". But the 
unwinder doesn't know. So you have to restore frame pointer after "ret", by
	.cfi_def_cfa		%rbp, 16

As you mentioned in the stack alignment case, we just can't rely on code 
review to catch such bugs. We need a test case to make sure all CFI 
directives are correct, which was also a request from Andy.

>>> +.Lhandle_exception:
>>> +	mov	0x18(%rbp), %rcx
>>> +	test    %rcx, %rcx
>>> +	je	.Lskip_exception_info
>>
>> A single "jrcxz .Lskip_exception_info" is equivalent to the above 2
>> instructions combined.
> 
> Both implementations take a single uop on CPUs that support SGX.  IMO,
> using the simpler and more common instructions is more universally
> readable.

I'm not sure the processor could combine 2 instructions ("test"+"je") 
into just 1 uop. And "jrcxz" is also a broadly used instruction.

>>> +	/* Push @e, u_rsp and @tcs as parameters to the callback. */
>>>   	push	0x18(%rbp)
>>>   	push	%rbx
>>>   	push	0x10(%rbp)
>>> -	/* Call *%rax via retpoline */
>>> -	call	40f
>>> -	/* Restore %rsp to its original value left off by the enclave from last
>>> -	 * exit */
>>> +
>>> +	/* Pass the "return" value to the callback via %rcx. */
>>> +	mov	%eax, %ecx
>>
>> @e (ex_info) is almost always needed by every callback as it also serves as
>> the "context pointer". The return value on the other hand is insignificant
>> because it could be deduced from @e->EX_LEAF anyway. So I'd retain %rcx and
>> push %rax to the stack instead, given the purpose of this patch is to
>> squeeze out a bit performance.
> 
> Please take this up in patch 02/16, which actually introduced this change.

My apology but willing to pull all related discussions into a single thread.

If you adhere to the convention of "%rcx containing @e", then the code 
here could be
	push	%rax		// for stack alignment
	push	%rax		// return value
	push	%rbx		// u_rsp
	push	0x10(%rsp)	// tcs
				// %rcx left unchanged pointing to @e
>>> +	/* Clear RFLAGS.DF per x86_64 ABI */
>>> +	cld
>>> +
>>> +	/* Load the callback pointer to %rax and invoke it via retpoline. */
>>> +	mov	0x20(%rbp), %rax
>>
>> Per X86_64 ABI, %rsp shall be 16 bytes aligned before "call". But %rsp here
>> doesn't look aligned properly.
> 
> Argh, I probably botched it back in patch 02/16 too.  I'll see if I can
> add a check to verify %rsp alignment in the selftest, verifying via code
> inspection is bound to be error prone.
> 
>>> +	call	.Lretpoline

Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Sean Christopherson]

On Thu, Oct 10, 2019 at 10:49:59AM -0700, Xing, Cedric wrote:
> On 10/9/2019 12:10 PM, Sean Christopherson wrote:
> >On Wed, Oct 09, 2019 at 11:00:55AM -0700, Xing, Cedric wrote:
> >>On 10/7/2019 9:46 PM, Sean Christopherson wrote:
> >>>-	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
> >>>-	 * restored after the exit handler returns. */
> >>>+
> >>>+	/* Invoke userspace's exit handler if one was provided. */
> >>>+.Lhandle_exit:
> >>>+	cmp	$0, 0x20(%rbp)
> >>>+	jne	.Linvoke_userspace_handler
> >>>+
> >>>+.Lout:
> >>>+	leave
> >>>+	.cfi_def_cfa		%rsp, 8
> >>>+	ret
> >>>+
> >>>+.Linvalid_leaf:
> >>
> >>Please set frame pointer back to %rbp here, or stack unwinding will fail.
> >
> >Sorry, coffee isn't doing it's job, what's getting crushed, and where?
> 
> The frame pointer was %rbp but you changed it to %rsp 3 lines ago. That's
> correct after "leave" and execution won't pass "ret". But the unwinder
> doesn't know. So you have to restore frame pointer after "ret", by
> 	.cfi_def_cfa		%rbp, 16

Isn't the proper fix to move ".cfi_endproc" here?  Which I incorrectly
left after the RET for the retpoline.
 
> As you mentioned in the stack alignment case, we just can't rely on code
> review to catch such bugs. We need a test case to make sure all CFI
> directives are correct, which was also a request from Andy.

On the todo list...

> >>>+.Lhandle_exception:
> >>>+	mov	0x18(%rbp), %rcx
> >>>+	test    %rcx, %rcx
> >>>+	je	.Lskip_exception_info
> >>
> >>A single "jrcxz .Lskip_exception_info" is equivalent to the above 2
> >>instructions combined.
> >
> >Both implementations take a single uop on CPUs that support SGX.  IMO,
> >using the simpler and more common instructions is more universally
> >readable.
> 
> I'm not sure the processor could combine 2 instructions ("test"+"je") into
> just 1 uop. And "jrcxz" is also a broadly used instruction.

TEST+Jcc macrofusion has been supported since Merom (Core 2)[*].  CMP+Jcc
have also been fused since Merom, though not for all Jcc flavors (uarch
specific), whereas TEST can fuse with everything.  Sandy Bridge added
fusing of ADD, SUB, INC, DEC, AND and OR, with AND/OR following TEST
in terms of fusing capabilities, the rest following CMP behavior.

[*] https://en.wikichip.org/wiki/macro-operation_fusion

> >>>+	/* Push @e, u_rsp and @tcs as parameters to the callback. */
> >>>  	push	0x18(%rbp)
> >>>  	push	%rbx
> >>>  	push	0x10(%rbp)
> >>>-	/* Call *%rax via retpoline */
> >>>-	call	40f
> >>>-	/* Restore %rsp to its original value left off by the enclave from last
> >>>-	 * exit */
> >>>+
> >>>+	/* Pass the "return" value to the callback via %rcx. */
> >>>+	mov	%eax, %ecx
> >>
> >>@e (ex_info) is almost always needed by every callback as it also serves as
> >>the "context pointer". The return value on the other hand is insignificant
> >>because it could be deduced from @e->EX_LEAF anyway. So I'd retain %rcx and
> >>push %rax to the stack instead, given the purpose of this patch is to
> >>squeeze out a bit performance.
> >
> >Please take this up in patch 02/16, which actually introduced this change.
> 
> My apology but willing to pull all related discussions into a single thread.
> 
> If you adhere to the convention of "%rcx containing @e", then the code here
> could be
> 	push	%rax		// for stack alignment
> 	push	%rax		// return value
> 	push	%rbx		// u_rsp
> 	push	0x10(%rsp)	// tcs
> 				// %rcx left unchanged pointing to @e

Hmm, I still think it makes sense to have @e as the last parameters since
it's the one thing that's optional.  What if the callback prototype were
instead:

typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
					  long ursp, long r8, long r9,
					  void *tcs, int ret,
					  struct sgx_enclave_exception *e);

I.e. put @ret and @e next to each other since they go hand-in-hand.  For
me, that's visually easies to parse than burying 'int ret' or 'struct ... *e'
in the middle of the prototype.

And the relevant asm:
	/* Push @e, "return" value and @tcs as parameters to the callback. */
	push	0x18(%rbp)
	push	%eax
	push	0x10(%rbp)

	/* Pass the untrusted RSP (at exit) to the callback via %rcx. */
	mov	%ebx, %ecx

> >>>+	/* Clear RFLAGS.DF per x86_64 ABI */
> >>>+	cld
> >>>+
> >>>+	/* Load the callback pointer to %rax and invoke it via retpoline. */
> >>>+	mov	0x20(%rbp), %rax
> >>
> >>Per X86_64 ABI, %rsp shall be 16 bytes aligned before "call". But %rsp here
> >>doesn't look aligned properly.
> >
> >Argh, I probably botched it back in patch 02/16 too.  I'll see if I can
> >add a check to verify %rsp alignment in the selftest, verifying via code
> >inspection is bound to be error prone.
> >
> >>>+	call	.Lretpoline

Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Xing, Cedric]

On 10/10/2019 4:59 PM, Sean Christopherson wrote:
> On Thu, Oct 10, 2019 at 10:49:59AM -0700, Xing, Cedric wrote:
>> On 10/9/2019 12:10 PM, Sean Christopherson wrote:
>>> On Wed, Oct 09, 2019 at 11:00:55AM -0700, Xing, Cedric wrote:
>>>> On 10/7/2019 9:46 PM, Sean Christopherson wrote:
>>>>> -	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
>>>>> -	 * restored after the exit handler returns. */
>>>>> +
>>>>> +	/* Invoke userspace's exit handler if one was provided. */
>>>>> +.Lhandle_exit:
>>>>> +	cmp	$0, 0x20(%rbp)
>>>>> +	jne	.Linvoke_userspace_handler
>>>>> +
>>>>> +.Lout:
>>>>> +	leave
>>>>> +	.cfi_def_cfa		%rsp, 8
>>>>> +	ret
>>>>> +
>>>>> +.Linvalid_leaf:
>>>>
>>>> Please set frame pointer back to %rbp here, or stack unwinding will fail.
>>>
>>> Sorry, coffee isn't doing it's job, what's getting crushed, and where?
>>
>> The frame pointer was %rbp but you changed it to %rsp 3 lines ago. That's
>> correct after "leave" and execution won't pass "ret". But the unwinder
>> doesn't know. So you have to restore frame pointer after "ret", by
>> 	.cfi_def_cfa		%rbp, 16
> 
> Isn't the proper fix to move ".cfi_endproc" here?  Which I incorrectly
> left after the RET for the retpoline.

No. .cfi_endproc is used by the unwinder to determine if an address 
falls within a function. Its location has nothing to do with where RET 
is but shall always be at the end of the whole function.

.cfi_def_cfa tells the unwinder where the call frame starts. At here, 
the call frame starts at %rbp+16 but not %rsp+8, so ".cfi_def_cfa %rbp, 
16" is a must.

>>>>> +.Lhandle_exception:
>>>>> +	mov	0x18(%rbp), %rcx
>>>>> +	test    %rcx, %rcx
>>>>> +	je	.Lskip_exception_info
>>>>
>>>> A single "jrcxz .Lskip_exception_info" is equivalent to the above 2
>>>> instructions combined.
>>>
>>> Both implementations take a single uop on CPUs that support SGX.  IMO,
>>> using the simpler and more common instructions is more universally
>>> readable.
>>
>> I'm not sure the processor could combine 2 instructions ("test"+"je") into
>> just 1 uop. And "jrcxz" is also a broadly used instruction.
> 
> TEST+Jcc macrofusion has been supported since Merom (Core 2)[*].  CMP+Jcc
> have also been fused since Merom, though not for all Jcc flavors (uarch
> specific), whereas TEST can fuse with everything.  Sandy Bridge added
> fusing of ADD, SUB, INC, DEC, AND and OR, with AND/OR following TEST
> in terms of fusing capabilities, the rest following CMP behavior.
> 
> [*] https://en.wikichip.org/wiki/macro-operation_fusion

Good to know. Thanks for the info!

>>>>> +	/* Push @e, u_rsp and @tcs as parameters to the callback. */
>>>>>   	push	0x18(%rbp)
>>>>>   	push	%rbx
>>>>>   	push	0x10(%rbp)
>>>>> -	/* Call *%rax via retpoline */
>>>>> -	call	40f
>>>>> -	/* Restore %rsp to its original value left off by the enclave from last
>>>>> -	 * exit */
>>>>> +
>>>>> +	/* Pass the "return" value to the callback via %rcx. */
>>>>> +	mov	%eax, %ecx
>>>>
>>>> @e (ex_info) is almost always needed by every callback as it also serves as
>>>> the "context pointer". The return value on the other hand is insignificant
>>>> because it could be deduced from @e->EX_LEAF anyway. So I'd retain %rcx and
>>>> push %rax to the stack instead, given the purpose of this patch is to
>>>> squeeze out a bit performance.
>>>
>>> Please take this up in patch 02/16, which actually introduced this change.
>>
>> My apology but willing to pull all related discussions into a single thread.
>>
>> If you adhere to the convention of "%rcx containing @e", then the code here
>> could be
>> 	push	%rax		// for stack alignment
>> 	push	%rax		// return value
>> 	push	%rbx		// u_rsp
>> 	push	0x10(%rsp)	// tcs
>> 				// %rcx left unchanged pointing to @e
> 
> Hmm, I still think it makes sense to have @e as the last parameters since
> it's the one thing that's optional.  What if the callback prototype were
> instead:
> 
> typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> 					  long ursp, long r8, long r9,
> 					  void *tcs, int ret,
> 					  struct sgx_enclave_exception *e);
> 
> I.e. put @ret and @e next to each other since they go hand-in-hand.  For
> me, that's visually easies to parse than burying 'int ret' or 'struct ... *e'
> in the middle of the prototype.
> 
> And the relevant asm:
> 	/* Push @e, "return" value and @tcs as parameters to the callback. */
> 	push	0x18(%rbp)
> 	push	%eax
> 	push	0x10(%rbp)
> 
> 	/* Pass the untrusted RSP (at exit) to the callback via %rcx. */
> 	mov	%ebx, %ecx
> 

Looks good to me. Don't forget to align the stack though, and ursp shall 
be 64-bit. That is,

	push	%rax		// align stack
	push	%rcx		// @e
	push	%rax		// @ret
	push	0x10(%rsp)	// @tcs
	mov	%rbx, %rcx	// @ursp

Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"

[Sean Christopherson]

On Wed, Oct 16, 2019 at 03:18:05PM -0700, Xing, Cedric wrote:
> On 10/10/2019 4:59 PM, Sean Christopherson wrote:
> >On Thu, Oct 10, 2019 at 10:49:59AM -0700, Xing, Cedric wrote:
> >>On 10/9/2019 12:10 PM, Sean Christopherson wrote:
> >>>On Wed, Oct 09, 2019 at 11:00:55AM -0700, Xing, Cedric wrote:
> >>>>On 10/7/2019 9:46 PM, Sean Christopherson wrote:
> >>>>>-	/* Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
> >>>>>-	 * restored after the exit handler returns. */
> >>>>>+
> >>>>>+	/* Invoke userspace's exit handler if one was provided. */
> >>>>>+.Lhandle_exit:
> >>>>>+	cmp	$0, 0x20(%rbp)
> >>>>>+	jne	.Linvoke_userspace_handler
> >>>>>+
> >>>>>+.Lout:
> >>>>>+	leave
> >>>>>+	.cfi_def_cfa		%rsp, 8
> >>>>>+	ret
> >>>>>+
> >>>>>+.Linvalid_leaf:
> >>>>
> >>>>Please set frame pointer back to %rbp here, or stack unwinding will fail.
> >>>
> >>>Sorry, coffee isn't doing it's job, what's getting crushed, and where?
> >>
> >>The frame pointer was %rbp but you changed it to %rsp 3 lines ago. That's
> >>correct after "leave" and execution won't pass "ret". But the unwinder
> >>doesn't know. So you have to restore frame pointer after "ret", by
> >>	.cfi_def_cfa		%rbp, 16
> >
> >Isn't the proper fix to move ".cfi_endproc" here?  Which I incorrectly
> >left after the RET for the retpoline.
> 
> No. .cfi_endproc is used by the unwinder to determine if an address falls
> within a function. Its location has nothing to do with where RET is but
> shall always be at the end of the whole function.
> 
> .cfi_def_cfa tells the unwinder where the call frame starts. At here, the
> call frame starts at %rbp+16 but not %rsp+8, so ".cfi_def_cfa %rbp, 16" is a
> must.

Ahh, I understand now, hopefully.  I was thinking the .cfi directives
would magically understand the control flow.  Thanks!

> >>>>>+.Lhandle_exception:
> >>>>>+	mov	0x18(%rbp), %rcx
> >>>>>+	test    %rcx, %rcx
> >>>>>+	je	.Lskip_exception_info
> >>>>

Re: [PATCH for_v23 00/16] x86/vdso: sgx: Major vDSO cleanup

[Jarkko Sakkinen]

On Mon, Oct 07, 2019 at 09:45:57PM -0700, Sean Christopherson wrote:
> The main purpose of this series is to implement feedback from the original
> RFC to expand the vDSO[*] that went unaddressed before the expanded
> function was rushed into v21.
> 
> The other half of the series is to overhaul the selftest to actually test
> the exit handler variation of the vDSO, with a bunch of prework to add an
> assertion framework to standardize the various assertions in the test and
> improve the readability of the code.
> 
> The basic ideas for the exit handler subtests are from Cedric's original
> RFC, but rewritten from scratch to take advantage of the new assertion
> framework.  I haven't yet implemented single-step subtest, ideally that
> too will get done before v23.
> 
> [*] https://lkml.kernel.org/r/20190426210017.GA24467@linux.intel.com
> 
> Sean Christopherson (16):
>   x86/vdso: sgx: Drop the pseudocode "documentation"
>   x86/vdso: sgx: Do not use exception info to pass success/failure
>   x86/vdso: sgx: Rename the enclave exit handler typedef
>   x86/vdso: sgx: Move enclave exit handler declaration to UAPI header
>   x86/vdso: sgx: Add comment regarding kernel-doc shenanigans
>   x86/vdso: sgx: Rewrite __vdso_sgx_enter_enclave() function comment
>   selftests/x86: Fix linker warning in SGX selftest
>   selftests/x86/sgx: Use getauxval() to retrieve the vDSO base address
>   selftests/x86/sgx: Add helper function and macros to assert results
>   selftests/x86/sgx: Handle setup failures via test assertions
>   selftests/x86/sgx: Sanitize the types for sgx_call()'s input params
>   selftests/x86/sgx: Move existing sub-test to a separate helper
>   selftests/x86/sgx: Add a test of the vDSO exception reporting
>     mechanism
>   selftests/x86/sgx: Add test of vDSO with basic exit handler
>   selftests/x86/sgx: Add sub-test for exception behavior with exit
>     handler
>   x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no
>     callback"
> 
>  arch/x86/entry/vdso/vsgx_enter_enclave.S  | 228 +++++++------
>  arch/x86/include/uapi/asm/sgx.h           |  18 +
>  tools/testing/selftests/x86/sgx/Makefile  |   2 +-
>  tools/testing/selftests/x86/sgx/defines.h |   6 +
>  tools/testing/selftests/x86/sgx/main.c    | 384 ++++++++++++++--------
>  5 files changed, 387 insertions(+), 251 deletions(-)
> 
> -- 
> 2.22.0
> 

vDSO changes look legit. I might do some minor edits (like coding
style tweaks only). Thanks.

/Jarkko

Re: [PATCH for_v23 00/16] x86/vdso: sgx: Major vDSO cleanup

[Sean Christopherson]

On Thu, Oct 10, 2019 at 11:10:19AM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 07, 2019 at 09:45:57PM -0700, Sean Christopherson wrote:
> > The main purpose of this series is to implement feedback from the original
> > RFC to expand the vDSO[*] that went unaddressed before the expanded
> > function was rushed into v21.
> > 
> > The other half of the series is to overhaul the selftest to actually test
> > the exit handler variation of the vDSO, with a bunch of prework to add an
> > assertion framework to standardize the various assertions in the test and
> > improve the readability of the code.
> > 
> > The basic ideas for the exit handler subtests are from Cedric's original
> > RFC, but rewritten from scratch to take advantage of the new assertion
> > framework.  I haven't yet implemented single-step subtest, ideally that
> > too will get done before v23.
> > 
> > [*] https://lkml.kernel.org/r/20190426210017.GA24467@linux.intel.com
> > 
> > Sean Christopherson (16):
> >   x86/vdso: sgx: Drop the pseudocode "documentation"
> >   x86/vdso: sgx: Do not use exception info to pass success/failure
> >   x86/vdso: sgx: Rename the enclave exit handler typedef
> >   x86/vdso: sgx: Move enclave exit handler declaration to UAPI header
> >   x86/vdso: sgx: Add comment regarding kernel-doc shenanigans
> >   x86/vdso: sgx: Rewrite __vdso_sgx_enter_enclave() function comment
> >   selftests/x86: Fix linker warning in SGX selftest
> >   selftests/x86/sgx: Use getauxval() to retrieve the vDSO base address
> >   selftests/x86/sgx: Add helper function and macros to assert results
> >   selftests/x86/sgx: Handle setup failures via test assertions
> >   selftests/x86/sgx: Sanitize the types for sgx_call()'s input params
> >   selftests/x86/sgx: Move existing sub-test to a separate helper
> >   selftests/x86/sgx: Add a test of the vDSO exception reporting
> >     mechanism
> >   selftests/x86/sgx: Add test of vDSO with basic exit handler
> >   selftests/x86/sgx: Add sub-test for exception behavior with exit
> >     handler
> >   x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no
> >     callback"
> > 
> >  arch/x86/entry/vdso/vsgx_enter_enclave.S  | 228 +++++++------
> >  arch/x86/include/uapi/asm/sgx.h           |  18 +
> >  tools/testing/selftests/x86/sgx/Makefile  |   2 +-
> >  tools/testing/selftests/x86/sgx/defines.h |   6 +
> >  tools/testing/selftests/x86/sgx/main.c    | 384 ++++++++++++++--------
> >  5 files changed, 387 insertions(+), 251 deletions(-)
> > 
> > -- 
> > 2.22.0
> > 
> 
> vDSO changes look legit. I might do some minor edits (like coding
> style tweaks only). Thanks.

Let me send v2 to fix the stack alignment issue Cedric pointed out, along
with a selftest enhancement to verify the alignment.

Re: [PATCH for_v23 00/16] x86/vdso: sgx: Major vDSO cleanup

[Jarkko Sakkinen]

On Thu, Oct 10, 2019 at 09:08:47AM -0700, Sean Christopherson wrote:
> On Thu, Oct 10, 2019 at 11:10:19AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 07, 2019 at 09:45:57PM -0700, Sean Christopherson wrote:
> > > The main purpose of this series is to implement feedback from the original
> > > RFC to expand the vDSO[*] that went unaddressed before the expanded
> > > function was rushed into v21.
> > > 
> > > The other half of the series is to overhaul the selftest to actually test
> > > the exit handler variation of the vDSO, with a bunch of prework to add an
> > > assertion framework to standardize the various assertions in the test and
> > > improve the readability of the code.
> > > 
> > > The basic ideas for the exit handler subtests are from Cedric's original
> > > RFC, but rewritten from scratch to take advantage of the new assertion
> > > framework.  I haven't yet implemented single-step subtest, ideally that
> > > too will get done before v23.
> > > 
> > > [*] https://lkml.kernel.org/r/20190426210017.GA24467@linux.intel.com
> > > 
> > > Sean Christopherson (16):
> > >   x86/vdso: sgx: Drop the pseudocode "documentation"
> > >   x86/vdso: sgx: Do not use exception info to pass success/failure
> > >   x86/vdso: sgx: Rename the enclave exit handler typedef
> > >   x86/vdso: sgx: Move enclave exit handler declaration to UAPI header
> > >   x86/vdso: sgx: Add comment regarding kernel-doc shenanigans
> > >   x86/vdso: sgx: Rewrite __vdso_sgx_enter_enclave() function comment
> > >   selftests/x86: Fix linker warning in SGX selftest
> > >   selftests/x86/sgx: Use getauxval() to retrieve the vDSO base address
> > >   selftests/x86/sgx: Add helper function and macros to assert results
> > >   selftests/x86/sgx: Handle setup failures via test assertions
> > >   selftests/x86/sgx: Sanitize the types for sgx_call()'s input params
> > >   selftests/x86/sgx: Move existing sub-test to a separate helper
> > >   selftests/x86/sgx: Add a test of the vDSO exception reporting
> > >     mechanism
> > >   selftests/x86/sgx: Add test of vDSO with basic exit handler
> > >   selftests/x86/sgx: Add sub-test for exception behavior with exit
> > >     handler
> > >   x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no
> > >     callback"
> > > 
> > >  arch/x86/entry/vdso/vsgx_enter_enclave.S  | 228 +++++++------
> > >  arch/x86/include/uapi/asm/sgx.h           |  18 +
> > >  tools/testing/selftests/x86/sgx/Makefile  |   2 +-
> > >  tools/testing/selftests/x86/sgx/defines.h |   6 +
> > >  tools/testing/selftests/x86/sgx/main.c    | 384 ++++++++++++++--------
> > >  5 files changed, 387 insertions(+), 251 deletions(-)
> > > 
> > > -- 
> > > 2.22.0
> > > 
> > 
> > vDSO changes look legit. I might do some minor edits (like coding
> > style tweaks only). Thanks.
> 
> Let me send v2 to fix the stack alignment issue Cedric pointed out, along
> with a selftest enhancement to verify the alignment.

Unfortunately I already applied these.

/Jarkko