[PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Sean Christopherson]

Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
close to being callable from C (with caveats and a cooperative enclave).
The missing pieces are preserving %rbx and taking @leaf as a standard
parameter.

v2:
  - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
  - Add CFI directive for RBX. [Cedric]

v1:
  - https://patchwork.kernel.org/cover/11446355/

Sean Christopherson (5):
  x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code
  x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave
  selftests/sgx: Pass EENTER to vDSO wrapper instead of hardcoding
  selftests/sgx: Stop clobbering non-volatile registers
  selftests/sgx: Add selftest to invoke __vsgx_enter_enclave() from C

 arch/x86/entry/vdso/vsgx_enter_enclave.S      | 64 ++-----------------
 arch/x86/include/uapi/asm/sgx.h               | 61 ++++++++++++++++++
 tools/testing/selftests/sgx/call.S            |  1 -
 tools/testing/selftests/sgx/defines.h         |  1 +
 tools/testing/selftests/sgx/main.c            | 20 ++++--
 tools/testing/selftests/sgx/main.h            |  2 +-
 .../selftests/sgx/test_encl_bootstrap.S       |  6 +-
 7 files changed, 84 insertions(+), 71 deletions(-)

-- 
2.24.1

[PATCH for_v29 v2 1/5] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Sean Christopherson]

Make __vdso_sgx_enter_enclave() callable from C by preserving %rbx
and taking @leaf in %rcx instead of %rax.  Being able to invoke the vDSO
from C reduces the overhead of runtimes that are tightly coupled with
their enclaves, e.g. that can rely on the enclave to save and restore
non-volatile registers, as the runtime doesn't need an assembly wrapper
to preserve non-volatile registers and/or shuffle stack arguments.

Note, both %rcx and %rbx are consumed by EENTER/ERESUME, i.e. consuming
them doesn't violate the primary tenet of __vdso_sgx_enter_enclave()
that "thou shalt not restrict how information is exchanged between an
enclave and its host process".

Suggested-by: Nathaniel McCallum <npmccallum@redhat.com>
Cc: Cedric Xing <cedric.xing@intel.com>
Cc: Jethro Beekman <jethro@fortanix.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: linux-sgx@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 30 ++++++++++++++----------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 34cee2b0ef09..c56064fb36bc 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -17,22 +17,22 @@
 
 /**
  * __vdso_sgx_enter_enclave() - Enter an SGX enclave
+ * @rdi:	Pass-through value for RDI
+ * @rsi:	Pass-through value for RSI
+ * @rdx:	Pass-through value for RDX
  * @leaf:	ENCLU leaf, must be EENTER or ERESUME
+ * @r8:		Pass-through value for R8
+ * @r9:		Pass-through value for R9
  * @tcs:	TCS, must be non-NULL
  * @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.
- *
- * Input ABI:
- *  @leaf	%eax
- *  @tcs	8(%rsp)
- *  @e 		0x10(%rsp)
- *  @handler	0x18(%rsp)
- *
- * Output ABI:
- *  @ret	%eax
+ * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance
+ * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
+ * Except for non-volatile general purpose registers, preserving/setting state
+ * in accordance with the x86-64 ABI is the responsibility of the enclave and
+ * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
+ * without careful consideration by both the enclave and its runtime.
  *
  * 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
@@ -71,7 +71,9 @@
  */
 #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,
+int __vdso_sgx_enter_enclave(unsigned long rdi, unsigned long rsi,
+			     unsigned long rdx, unsigned int leaf,
+			     unsigned long r8,  unsigned long r9, void *tcs,
 			     struct sgx_enclave_exception *e,
 			     sgx_enclave_exit_handler_t handler);
 #endif
@@ -83,7 +85,10 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	.cfi_rel_offset		%rbp, 0
 	mov	%rsp, %rbp
 	.cfi_def_cfa_register	%rbp
+	push	%rbx
+	.cfi_rel_offset		%rbx, -8
 
+	mov	%ecx, %eax
 .Lenter_enclave:
 	/* EENTER <= leaf <= ERESUME */
 	cmp	$EENTER, %eax
@@ -109,6 +114,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	jne	.Linvoke_userspace_handler
 
 .Lout:
+	pop	%rbx
 	leave
 	.cfi_def_cfa		%rsp, 8
 	ret
-- 
2.24.1

[PATCH for_v29 v2 2/5] x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave

[Sean Christopherson]

Define a typedef for the __vdso_sgx_enter_enclave() prototype and move
the entire function comment from the assembly code to the uAPI header,
dropping the kernel doc hack along the way.

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

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index c56064fb36bc..be7e467e1efb 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -15,68 +15,6 @@
 .code64
 .section .text, "ax"
 
-/**
- * __vdso_sgx_enter_enclave() - Enter an SGX enclave
- * @rdi:	Pass-through value for RDI
- * @rsi:	Pass-through value for RSI
- * @rdx:	Pass-through value for RDX
- * @leaf:	ENCLU leaf, must be EENTER or ERESUME
- * @r8:		Pass-through value for R8
- * @r9:		Pass-through value for R9
- * @tcs:	TCS, must be non-NULL
- * @e:		Optional struct sgx_enclave_exception instance
- * @handler:	Optional enclave exit handler
- *
- * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance
- * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
- * Except for non-volatile general purpose registers, preserving/setting state
- * in accordance with the x86-64 ABI is the responsibility of the enclave and
- * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
- * without careful consideration by both the enclave and its runtime.
- *
- * 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 exit handler may transfer control, e.g. via longjmp() or C++ exception,
- * without returning to __vdso_sgx_enter_enclave().
- *
- * Return:
- *  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. */
-int __vdso_sgx_enter_enclave(unsigned long rdi, unsigned long rsi,
-			     unsigned long rdx, unsigned int leaf,
-			     unsigned long r8,  unsigned long r9, void *tcs,
-			     struct sgx_enclave_exception *e,
-			     sgx_enclave_exit_handler_t handler);
-#endif
 SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	/* Prolog */
 	.cfi_startproc
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index e196cfd44b70..8ca9ef7ea50a 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -111,4 +111,65 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
 					  void *tcs, int ret,
 					  struct sgx_enclave_exception *e);
 
+/**
+ * __vdso_sgx_enter_enclave() - Enter an SGX enclave
+ * @rdi:	Pass-through value for RDI
+ * @rsi:	Pass-through value for RSI
+ * @rdx:	Pass-through value for RDX
+ * @leaf:	ENCLU leaf, must be EENTER or ERESUME
+ * @r8:		Pass-through value for R8
+ * @r9:		Pass-through value for R9
+ * @tcs:	TCS, must be non-NULL
+ * @e:		Optional struct sgx_enclave_exception instance
+ * @handler:	Optional enclave exit handler
+ *
+ * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance
+ * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
+ * Except for non-volatile general purpose registers, preserving/setting state
+ * in accordance with the x86-64 ABI is the responsibility of the enclave and
+ * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
+ * without careful consideration by both the enclave and its runtime.
+ *
+ * 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 exit handler may transfer control, e.g. via longjmp() or C++ exception,
+ * without returning to __vdso_sgx_enter_enclave().
+ *
+ * Return:
+ *  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
+ */
+typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
+					unsigned long rdx, unsigned int leaf,
+					unsigned long r8,  unsigned long r9,
+					void *tcs,
+					struct sgx_enclave_exception *e,
+					sgx_enclave_exit_handler_t handler);
+
 #endif /* _UAPI_ASM_X86_SGX_H */
-- 
2.24.1

[PATCH for_v29 v2 3/5] selftests/sgx: Pass EENTER to vDSO wrapper instead of hardcoding

[Sean Christopherson]

Pass EENTER to the vDSO wrapper via %ecx instead of hardcoding it to
make the wrapper compatible with the new vDSO calling convention.

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

diff --git a/tools/testing/selftests/sgx/call.S b/tools/testing/selftests/sgx/call.S
index c37f55a607c8..77131e83db42 100644
--- a/tools/testing/selftests/sgx/call.S
+++ b/tools/testing/selftests/sgx/call.S
@@ -37,7 +37,6 @@ sgx_call_vdso:
 	.cfi_adjust_cfa_offset	8
 	push	0x48(%rsp)
 	.cfi_adjust_cfa_offset	8
-	mov	$2, %eax
 	call	*eenter(%rip)
 	add	$0x20, %rsp
 	.cfi_adjust_cfa_offset	-0x20
diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h
index 1802cace7527..be8969922804 100644
--- a/tools/testing/selftests/sgx/defines.h
+++ b/tools/testing/selftests/sgx/defines.h
@@ -15,6 +15,7 @@
 #define __packed __attribute__((packed))
 
 #include "../../../../arch/x86/kernel/cpu/sgx/arch.h"
+#include "../../../../arch/x86/include/asm/enclu.h"
 #include "../../../../arch/x86/include/uapi/asm/sgx.h"
 
 #endif /* DEFINES_H */
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 9238cce47f77..f6bb40f22884 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -171,7 +171,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	eenter = addr + eenter_sym->st_value;
 
-	sgx_call_vdso((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
+	sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL,
 		      (void *)encl.encl_base, &exception, NULL);
 	if (result != MAGIC)
 		goto err;
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index 6e1ae292bd25..999422cc7343 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -32,7 +32,7 @@ bool encl_load(const char *path, struct encl *encl);
 bool encl_measure(struct encl *encl);
 bool encl_build(struct encl *encl);
 
-int sgx_call_vdso(void *rdi, void *rsi, long rdx, void *rcx, void *r8, void *r9,
+int sgx_call_vdso(void *rdi, void *rsi, long rdx, u32 leaf, void *r8, void *r9,
 		  void *tcs, struct sgx_enclave_exception *ei, void *cb);
 
 #endif /* MAIN_H */
-- 
2.24.1

[PATCH for_v29 v2 4/5] selftests/sgx: Stop clobbering non-volatile registers

[Sean Christopherson]

Stop clearing non-volatile registers in the enclave's trampoline code,
there are no secrets to preserve, and not clobbering the registers makes
the enclave compatible with calling the vDSO from C.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/sgx/test_encl_bootstrap.S | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
index 6a5d734cbf16..6836ea86126e 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -54,7 +54,7 @@ encl_entry:
 
 	pop	%rbx # pop the enclave base address
 
-	# Clear GPRs.
+	/* Clear volatile GPRs, except RAX (EEXIT leaf). */
 	xor     %rcx, %rcx
 	xor     %rdx, %rdx
 	xor     %rdi, %rdi
@@ -63,10 +63,6 @@ encl_entry:
 	xor     %r9, %r9
 	xor     %r10, %r10
 	xor     %r11, %r11
-	xor     %r12, %r12
-	xor     %r13, %r13
-	xor     %r14, %r14
-	xor     %r15, %r15
 
 	# Reset status flags.
 	add     %rdx, %rdx # OF = SF = AF = CF = 0; ZF = PF = 1
-- 
2.24.1

[PATCH for_v29 v2 5/5] selftests/sgx: Add selftest to invoke __vsgx_enter_enclave() from C

[Sean Christopherson]

Add a selftest to call __vsgx_enter_enclave() from C.

Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/sgx/main.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index f6bb40f22884..5394b2f6af8e 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -19,7 +19,7 @@
 #include "main.h"
 
 static const uint64_t MAGIC = 0x1122334455667788ULL;
-void *eenter;
+vdso_sgx_enter_enclave_t eenter;
 
 struct vdso_symtab {
 	Elf64_Sym *elf_symtab;
@@ -173,15 +173,27 @@ int main(int argc, char *argv[], char *envp[])
 
 	sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL,
 		      (void *)encl.encl_base, &exception, NULL);
-	if (result != MAGIC)
+	if (result != MAGIC) {
+		printf("FAIL: sgx_call_vdso(), expected: 0x%lx, got: 0x%lx\n",
+		       MAGIC, result);
 		goto err;
+	}
+
+	/* Invoke the vDSO directly. */
+	result = 0;
+	eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, 0, 0,
+	       (void *)encl.encl_base, &exception, NULL);
+	if (result != MAGIC) {
+		printf("FAIL: eenter(), expected: 0x%lx, got: 0x%lx\n",
+		       MAGIC, result);
+		goto err;
+	}
 
 	printf("SUCCESS\n");
 	encl_delete(&encl);
 	exit(0);
 
 err:
-	printf("FAILURE\n");
 	encl_delete(&encl);
 	exit(1);
 }
-- 
2.24.1

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Jarkko Sakkinen]

On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> close to being callable from C (with caveats and a cooperative enclave).
> The missing pieces are preserving %rbx and taking @leaf as a standard
> parameter.
> 
> v2:
>   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
>   - Add CFI directive for RBX. [Cedric]

I'm sorry for throwing stick's constantly but I think having a real
ELF loader is for better.

/Jarkko

Re: [PATCH for_v29 v2 1/5] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Jarkko Sakkinen]

On Mon, Mar 30, 2020 at 11:08:07AM -0700, Sean Christopherson wrote:
> Make __vdso_sgx_enter_enclave() callable from C by preserving %rbx
> and taking @leaf in %rcx instead of %rax.  Being able to invoke the vDSO
> from C reduces the overhead of runtimes that are tightly coupled with
> their enclaves, e.g. that can rely on the enclave to save and restore
> non-volatile registers, as the runtime doesn't need an assembly wrapper
> to preserve non-volatile registers and/or shuffle stack arguments.
> 
> Note, both %rcx and %rbx are consumed by EENTER/ERESUME, i.e. consuming
> them doesn't violate the primary tenet of __vdso_sgx_enter_enclave()
> that "thou shalt not restrict how information is exchanged between an
> enclave and its host process".
> 
> Suggested-by: Nathaniel McCallum <npmccallum@redhat.com>
> Cc: Cedric Xing <cedric.xing@intel.com>
> Cc: Jethro Beekman <jethro@fortanix.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: linux-sgx@vger.kernel.org
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 30 ++++++++++++++----------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index 34cee2b0ef09..c56064fb36bc 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -17,22 +17,22 @@
>  
>  /**
>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> + * @rdi:	Pass-through value for RDI
> + * @rsi:	Pass-through value for RSI
> + * @rdx:	Pass-through value for RDX
>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> + * @r8:		Pass-through value for R8
> + * @r9:		Pass-through value for R9
>   * @tcs:	TCS, must be non-NULL
>   * @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.
> - *
> - * Input ABI:
> - *  @leaf	%eax
> - *  @tcs	8(%rsp)
> - *  @e 		0x10(%rsp)
> - *  @handler	0x18(%rsp)
> - *
> - * Output ABI:
> - *  @ret	%eax
> + * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance

I'd simply put **NOTE** here instead of **Important!** as it is more
common.

> + * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
> + * Except for non-volatile general purpose registers, preserving/setting state
> + * in accordance with the x86-64 ABI is the responsibility of the enclave and
> + * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> + * without careful consideration by both the enclave and its runtime.

Instead "e.g. doesn't explcitly clear EFLAGS.DF after EEXIT" (which is
somewhat confusing statement) paragraph should be replaced with a simple
enumerated list of differences.

Something might be left out but that's cool. Just do your best and it
can refined over time to be more exact.

>   *
>   * 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
> @@ -71,7 +71,9 @@
>   */
>  #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,
> +int __vdso_sgx_enter_enclave(unsigned long rdi, unsigned long rsi,
> +			     unsigned long rdx, unsigned int leaf,
> +			     unsigned long r8,  unsigned long r9, void *tcs,
>  			     struct sgx_enclave_exception *e,
>  			     sgx_enclave_exit_handler_t handler);
>  #endif
> @@ -83,7 +85,10 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	.cfi_rel_offset		%rbp, 0
>  	mov	%rsp, %rbp
>  	.cfi_def_cfa_register	%rbp
> +	push	%rbx
> +	.cfi_rel_offset		%rbx, -8
>  
> +	mov	%ecx, %eax
>  .Lenter_enclave:
>  	/* EENTER <= leaf <= ERESUME */
>  	cmp	$EENTER, %eax
> @@ -109,6 +114,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	jne	.Linvoke_userspace_handler
>  
>  .Lout:
> +	pop	%rbx
>  	leave
>  	.cfi_def_cfa		%rsp, 8
>  	ret
> -- 
> 2.24.1
> 

Re: [PATCH for_v29 v2 3/5] selftests/sgx: Pass EENTER to vDSO wrapper instead of hardcoding

[Jarkko Sakkinen]

On Mon, Mar 30, 2020 at 11:08:09AM -0700, Sean Christopherson wrote:
> Pass EENTER to the vDSO wrapper via %ecx instead of hardcoding it to
> make the wrapper compatible with the new vDSO calling convention.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I think it'd be good if you merge 1/5-3/5 and 4/5-55 to have two patches.

Easier to review given the strong bounds, easier for you to update
and easier for me to merge later on.

/Jarko

Re: [PATCH for_v29 v2 2/5] x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave

[Jarkko Sakkinen]

On Mon, Mar 30, 2020 at 11:08:08AM -0700, Sean Christopherson wrote:
> Define a typedef for the __vdso_sgx_enter_enclave() prototype and move
> the entire function comment from the assembly code to the uAPI header,
> dropping the kernel doc hack along the way.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 62 ------------------------
>  arch/x86/include/uapi/asm/sgx.h          | 61 +++++++++++++++++++++++
>  2 files changed, 61 insertions(+), 62 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index c56064fb36bc..be7e467e1efb 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -15,68 +15,6 @@
>  .code64
>  .section .text, "ax"
>  
> -/**
> - * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> - * @rdi:	Pass-through value for RDI
> - * @rsi:	Pass-through value for RSI
> - * @rdx:	Pass-through value for RDX
> - * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> - * @r8:		Pass-through value for R8
> - * @r9:		Pass-through value for R9
> - * @tcs:	TCS, must be non-NULL
> - * @e:		Optional struct sgx_enclave_exception instance
> - * @handler:	Optional enclave exit handler
> - *
> - * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance
> - * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
> - * Except for non-volatile general purpose registers, preserving/setting state
> - * in accordance with the x86-64 ABI is the responsibility of the enclave and
> - * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> - * without careful consideration by both the enclave and its runtime.
> - *
> - * 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 exit handler may transfer control, e.g. via longjmp() or C++ exception,
> - * without returning to __vdso_sgx_enter_enclave().
> - *
> - * Return:
> - *  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. */
> -int __vdso_sgx_enter_enclave(unsigned long rdi, unsigned long rsi,
> -			     unsigned long rdx, unsigned int leaf,
> -			     unsigned long r8,  unsigned long r9, void *tcs,
> -			     struct sgx_enclave_exception *e,
> -			     sgx_enclave_exit_handler_t handler);
> -#endif
>  SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	/* Prolog */
>  	.cfi_startproc
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index e196cfd44b70..8ca9ef7ea50a 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -111,4 +111,65 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>  					  void *tcs, int ret,
>  					  struct sgx_enclave_exception *e);
>  
> +/**
> + * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> + * @rdi:	Pass-through value for RDI
> + * @rsi:	Pass-through value for RSI
> + * @rdx:	Pass-through value for RDX
> + * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> + * @r8:		Pass-through value for R8
> + * @r9:		Pass-through value for R9
> + * @tcs:	TCS, must be non-NULL
> + * @e:		Optional struct sgx_enclave_exception instance
> + * @handler:	Optional enclave exit handler
> + *
> + * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance
> + * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
> + * Except for non-volatile general purpose registers, preserving/setting state
> + * in accordance with the x86-64 ABI is the responsibility of the enclave and
> + * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> + * without careful consideration by both the enclave and its runtime.
> + *
> + * 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 exit handler may transfer control, e.g. via longjmp() or C++ exception,
> + * without returning to __vdso_sgx_enter_enclave().
> + *
> + * Return:
> + *  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
> + */
> +typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> +					unsigned long rdx, unsigned int leaf,
> +					unsigned long r8,  unsigned long r9,
> +					void *tcs,
> +					struct sgx_enclave_exception *e,
> +					sgx_enclave_exit_handler_t handler);
> +
>  #endif /* _UAPI_ASM_X86_SGX_H */
> -- 
> 2.24.1
> 

Most probaby agree with this.

/Jarkko

Re: [PATCH for_v29 v2 3/5] selftests/sgx: Pass EENTER to vDSO wrapper instead of hardcoding

[Jarkko Sakkinen]

On Tue, Mar 31, 2020 at 12:07:16AM +0300, Jarkko Sakkinen wrote:
> On Mon, Mar 30, 2020 at 11:08:09AM -0700, Sean Christopherson wrote:
> > Pass EENTER to the vDSO wrapper via %ecx instead of hardcoding it to
> > make the wrapper compatible with the new vDSO calling convention.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I think it'd be good if you merge 1/5-3/5 and 4/5-55 to have two patches.
> 
> Easier to review given the strong bounds, easier for you to update
> and easier for me to merge later on.

I mean {1, 2, 3} and {4, 5}

/Jarkko

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Nathaniel McCallum]

On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > close to being callable from C (with caveats and a cooperative enclave).
> > The missing pieces are preserving %rbx and taking @leaf as a standard
> > parameter.
> >
> > v2:
> >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> >   - Add CFI directive for RBX. [Cedric]
>
> I'm sorry for throwing stick's constantly but I think having a real
> ELF loader is for better.

Why is it one or the other?

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Jarkko Sakkinen]

On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > close to being callable from C (with caveats and a cooperative enclave).
> > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > parameter.
> > >
> > > v2:
> > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > >   - Add CFI directive for RBX. [Cedric]
> >
> > I'm sorry for throwing stick's constantly but I think having a real
> > ELF loader is for better.
> 
> Why is it one or the other?

Hmm... Not sure I understand what you want to know.

A raw binary requires a hardcoded way to interpret it. Obviously ELF
is for better.

/Jarkko

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Nathaniel McCallum]

On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > parameter.
> > > >
> > > > v2:
> > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > >   - Add CFI directive for RBX. [Cedric]
> > >
> > > I'm sorry for throwing stick's constantly but I think having a real
> > > ELF loader is for better.

This statement seems like you are juxtaposing having
__vdso_sgx_enter_enclave() be potentially C-compatible with having an
ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
be C-callable *and* you can have an ELF loader.

> > Why is it one or the other?
>
> Hmm... Not sure I understand what you want to know.
>
> A raw binary requires a hardcoded way to interpret it. Obviously ELF
> is for better.

We (Enarx) implement an ELF loader.

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Jarkko Sakkinen]

On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > parameter.
> > > > >
> > > > > v2:
> > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > >   - Add CFI directive for RBX. [Cedric]
> > > >
> > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > ELF loader is for better.
> 
> This statement seems like you are juxtaposing having
> __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> be C-callable *and* you can have an ELF loader.

I'm not honestly sure what this is about but my comment was about heavy
rebasing of the GIT tree as I rewrote the selftest last week.

/Jarkko

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Nathaniel McCallum]

On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > >
> > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > parameter.
> > > > > >
> > > > > > v2:
> > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > >
> > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > ELF loader is for better.
> >
> > This statement seems like you are juxtaposing having
> > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > be C-callable *and* you can have an ELF loader.
>
> I'm not honestly sure what this is about but my comment was about heavy
> rebasing of the GIT tree as I rewrote the selftest last week.

Okay. Let's chalk it up to miscommunication then. :)

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Sean Christopherson]

On Wed, Apr 01, 2020 at 09:06:38AM -0400, Nathaniel McCallum wrote:
> On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > > parameter.
> > > > > > >
> > > > > > > v2:
> > > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > > >
> > > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > > ELF loader is for better.
> > >
> > > This statement seems like you are juxtaposing having
> > > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > > be C-callable *and* you can have an ELF loader.
> >
> > I'm not honestly sure what this is about but my comment was about heavy
> > rebasing of the GIT tree as I rewrote the selftest last week.
> 
> Okay. Let's chalk it up to miscommunication then. :)

Ha, I was in the same boat as Nathaniel.  We thought the "having a real
ELF loader comment" was a comment on the patch itself, i.e. that you
disagreed with it in some way because it didn't support an ELF loader,
hence our confusion.

Now I realize you were refering to the rebase needed due to rewriting the
selftest to use an ELF loader.  Crisis aborted :-)

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Jarkko Sakkinen]

On Wed, Apr 01, 2020 at 09:06:38AM -0400, Nathaniel McCallum wrote:
> On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > > parameter.
> > > > > > >
> > > > > > > v2:
> > > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > > >
> > > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > > ELF loader is for better.
> > >
> > > This statement seems like you are juxtaposing having
> > > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > > be C-callable *and* you can have an ELF loader.
> >
> > I'm not honestly sure what this is about but my comment was about heavy
> > rebasing of the GIT tree as I rewrote the selftest last week.
> 
> Okay. Let's chalk it up to miscommunication then. :)

Yeah, lets bring a background :-)

Last week basically rewrote the self-test. It now directly loads ELF and
dynamically signs that. Thus, the codebase has been kind of moving
target for a while. I'm absolutely for these changes as soon as the
form fits to the existing codebase.

/Jarkko

Re: [PATCH for_v29 v2 0/5] x86/sgx: Make vDSO callable from C

[Jarkko Sakkinen]

On Wed, Apr 01, 2020 at 07:49:38AM -0700, Sean Christopherson wrote:
> On Wed, Apr 01, 2020 at 09:06:38AM -0400, Nathaniel McCallum wrote:
> > On Wed, Apr 1, 2020 at 4:18 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Tue, Mar 31, 2020 at 09:40:24AM -0400, Nathaniel McCallum wrote:
> > > > On Tue, Mar 31, 2020 at 7:58 AM Jarkko Sakkinen
> > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > >
> > > > > On Mon, Mar 30, 2020 at 05:42:29PM -0400, Nathaniel McCallum wrote:
> > > > > > On Mon, Mar 30, 2020 at 4:48 PM Jarkko Sakkinen
> > > > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Mon, Mar 30, 2020 at 11:08:06AM -0700, Sean Christopherson wrote:
> > > > > > > > Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> > > > > > > > close to being callable from C (with caveats and a cooperative enclave).
> > > > > > > > The missing pieces are preserving %rbx and taking @leaf as a standard
> > > > > > > > parameter.
> > > > > > > >
> > > > > > > > v2:
> > > > > > > >   - Rebase to Jarkko's latest master, commit 402fb35a477a, "docs: ...")
> > > > > > > >   - Add CFI directive for RBX. [Cedric]
> > > > > > >
> > > > > > > I'm sorry for throwing stick's constantly but I think having a real
> > > > > > > ELF loader is for better.
> > > >
> > > > This statement seems like you are juxtaposing having
> > > > __vdso_sgx_enter_enclave() be potentially C-compatible with having an
> > > > ELF-loader. These are not incompabile. __vdso_sgx_enter_enclave() can
> > > > be C-callable *and* you can have an ELF loader.
> > >
> > > I'm not honestly sure what this is about but my comment was about heavy
> > > rebasing of the GIT tree as I rewrote the selftest last week.
> > 
> > Okay. Let's chalk it up to miscommunication then. :)
> 
> Ha, I was in the same boat as Nathaniel.  We thought the "having a real
> ELF loader comment" was a comment on the patch itself, i.e. that you
> disagreed with it in some way because it didn't support an ELF loader,
> hence our confusion.
> 
> Now I realize you were refering to the rebase needed due to rewriting the
> selftest to use an ELF loader.  Crisis aborted :-)

Awesome :-)

/Jarkko

Re: [PATCH for_v29 v2 1/5] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Sean Christopherson]

On Tue, Mar 31, 2020 at 12:04:46AM +0300, Jarkko Sakkinen wrote:
> On Mon, Mar 30, 2020 at 11:08:07AM -0700, Sean Christopherson wrote:
> > Make __vdso_sgx_enter_enclave() callable from C by preserving %rbx
> > and taking @leaf in %rcx instead of %rax.  Being able to invoke the vDSO
> > from C reduces the overhead of runtimes that are tightly coupled with
> > their enclaves, e.g. that can rely on the enclave to save and restore
> > non-volatile registers, as the runtime doesn't need an assembly wrapper
> > to preserve non-volatile registers and/or shuffle stack arguments.
> > 
> > Note, both %rcx and %rbx are consumed by EENTER/ERESUME, i.e. consuming
> > them doesn't violate the primary tenet of __vdso_sgx_enter_enclave()
> > that "thou shalt not restrict how information is exchanged between an
> > enclave and its host process".
> > 
> > Suggested-by: Nathaniel McCallum <npmccallum@redhat.com>
> > Cc: Cedric Xing <cedric.xing@intel.com>
> > Cc: Jethro Beekman <jethro@fortanix.com>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: linux-sgx@vger.kernel.org
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/entry/vdso/vsgx_enter_enclave.S | 30 ++++++++++++++----------
> >  1 file changed, 18 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> > index 34cee2b0ef09..c56064fb36bc 100644
> > --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> > +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> > @@ -17,22 +17,22 @@
> >  
> >  /**
> >   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> > + * @rdi:	Pass-through value for RDI
> > + * @rsi:	Pass-through value for RSI
> > + * @rdx:	Pass-through value for RDX
> >   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> > + * @r8:		Pass-through value for R8
> > + * @r9:		Pass-through value for R9
> >   * @tcs:	TCS, must be non-NULL
> >   * @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.
> > - *
> > - * Input ABI:
> > - *  @leaf	%eax
> > - *  @tcs	8(%rsp)
> > - *  @e 		0x10(%rsp)
> > - *  @handler	0x18(%rsp)
> > - *
> > - * Output ABI:
> > - *  @ret	%eax
> > + * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance
> 
> I'd simply put **NOTE** here instead of **Important!** as it is more
> common.
> 
> > + * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
> > + * Except for non-volatile general purpose registers, preserving/setting state
> > + * in accordance with the x86-64 ABI is the responsibility of the enclave and
> > + * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> > + * without careful consideration by both the enclave and its runtime.
> 
> Instead "e.g. doesn't explcitly clear EFLAGS.DF after EEXIT" (which is
> somewhat confusing statement) paragraph should be replaced with a simple
> enumerated list of differences.

I don't think the list is that simple, e.g. there is a lot of state that
is defined by the ABI that isn't touched, IMO talking about that state will
add confusion.

I also don't understand what's confusing about stating EFLAGS.DF isn't
cleared.

> Something might be left out but that's cool. Just do your best and it
> can refined over time to be more exact.
> 
> >   *
> >   * 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
> > @@ -71,7 +71,9 @@
> >   */
> >  #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,
> > +int __vdso_sgx_enter_enclave(unsigned long rdi, unsigned long rsi,
> > +			     unsigned long rdx, unsigned int leaf,
> > +			     unsigned long r8,  unsigned long r9, void *tcs,
> >  			     struct sgx_enclave_exception *e,
> >  			     sgx_enclave_exit_handler_t handler);
> >  #endif
> > @@ -83,7 +85,10 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
> >  	.cfi_rel_offset		%rbp, 0
> >  	mov	%rsp, %rbp
> >  	.cfi_def_cfa_register	%rbp
> > +	push	%rbx
> > +	.cfi_rel_offset		%rbx, -8
> >  
> > +	mov	%ecx, %eax
> >  .Lenter_enclave:
> >  	/* EENTER <= leaf <= ERESUME */
> >  	cmp	$EENTER, %eax
> > @@ -109,6 +114,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
> >  	jne	.Linvoke_userspace_handler
> >  
> >  .Lout:
> > +	pop	%rbx
> >  	leave
> >  	.cfi_def_cfa		%rsp, 8
> >  	ret
> > -- 
> > 2.24.1
> > 

Re: [PATCH for_v29 v2 1/5] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Jarkko Sakkinen]

On Fri, Apr 17, 2020 at 08:05:01AM -0700, Sean Christopherson wrote:
> On Tue, Mar 31, 2020 at 12:04:46AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Mar 30, 2020 at 11:08:07AM -0700, Sean Christopherson wrote:
> > > Make __vdso_sgx_enter_enclave() callable from C by preserving %rbx
> > > and taking @leaf in %rcx instead of %rax.  Being able to invoke the vDSO
> > > from C reduces the overhead of runtimes that are tightly coupled with
> > > their enclaves, e.g. that can rely on the enclave to save and restore
> > > non-volatile registers, as the runtime doesn't need an assembly wrapper
> > > to preserve non-volatile registers and/or shuffle stack arguments.
> > > 
> > > Note, both %rcx and %rbx are consumed by EENTER/ERESUME, i.e. consuming
> > > them doesn't violate the primary tenet of __vdso_sgx_enter_enclave()
> > > that "thou shalt not restrict how information is exchanged between an
> > > enclave and its host process".
> > > 
> > > Suggested-by: Nathaniel McCallum <npmccallum@redhat.com>
> > > Cc: Cedric Xing <cedric.xing@intel.com>
> > > Cc: Jethro Beekman <jethro@fortanix.com>
> > > Cc: Andy Lutomirski <luto@amacapital.net>
> > > Cc: linux-sgx@vger.kernel.org
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > ---
> > >  arch/x86/entry/vdso/vsgx_enter_enclave.S | 30 ++++++++++++++----------
> > >  1 file changed, 18 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> > > index 34cee2b0ef09..c56064fb36bc 100644
> > > --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> > > +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> > > @@ -17,22 +17,22 @@
> > >  
> > >  /**
> > >   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> > > + * @rdi:	Pass-through value for RDI
> > > + * @rsi:	Pass-through value for RSI
> > > + * @rdx:	Pass-through value for RDX
> > >   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> > > + * @r8:		Pass-through value for R8
> > > + * @r9:		Pass-through value for R9
> > >   * @tcs:	TCS, must be non-NULL
> > >   * @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.
> > > - *
> > > - * Input ABI:
> > > - *  @leaf	%eax
> > > - *  @tcs	8(%rsp)
> > > - *  @e 		0x10(%rsp)
> > > - *  @handler	0x18(%rsp)
> > > - *
> > > - * Output ABI:
> > > - *  @ret	%eax
> > > + * **Important!**  __vdso_sgx_enter_enclave() does not ensure full compliance
> > 
> > I'd simply put **NOTE** here instead of **Important!** as it is more
> > common.
> > 
> > > + * with the x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.
> > > + * Except for non-volatile general purpose registers, preserving/setting state
> > > + * in accordance with the x86-64 ABI is the responsibility of the enclave and
> > > + * its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> > > + * without careful consideration by both the enclave and its runtime.
> > 
> > Instead "e.g. doesn't explcitly clear EFLAGS.DF after EEXIT" (which is
> > somewhat confusing statement) paragraph should be replaced with a simple
> > enumerated list of differences.
> 
> I don't think the list is that simple, e.g. there is a lot of state that
> is defined by the ABI that isn't touched, IMO talking about that state will
> add confusion.
> 
> I also don't understand what's confusing about stating EFLAGS.DF isn't
> cleared.

Too much time was has passed since the last version, and too many
patches have been reviewed since.

Please just refresh the way you feel best and I will make conclusions
based on that. It's faster that way in the end.

/Jarkko