[PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Jarkko Sakkinen]

Reduce the struct size to 64 bytes. It makes sense to have a naturally
aligning struct size and it leaves 24 bytes of spare space for possible
future expansion. Having 256 bytes is over the top without any reasonable
explanation, which does not exist in the commit for the SGX vDSO.

Drop struct sgx_exception. It does not serve any semantic purpose in the
context of this vDSO. Doing this is also consistent with user_handler and
user_data fields, as neither are they encapsulated in an embedded data
structure.

Drop 'flags' from sgx_enclave_run as it is an unused value.'vsgx.S'
literally states this in an inline comment.

Pick more reasonable names for the four fields of the exception. For
example, 'trapnr' should rather be just 'number' because in practice we
deliver only trap and the rest are faults. Also 'vector' would be an
adequate name for this field. Document the fields properly in order to
better explain the values that they have inherited.

Remove 'exit_reason' as it is really a redundant field to save some space.
Instead, have just a field called 'leaf' that tells the last seen ENCLU.

The documentation, selftest and the kernel source code use the terms "user
handler" and "exit handler" in somewhat inconsistent manner. Start using
"AEX handler" consistently as this is the term that Intel SDM volume D uses
for these events.

Cc: Nathaniel McCallum <npmccallum@redhat.com>
Cc: Haitao Huang <haitao.huang@linux.intel.com>
CC: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Cedric Xing <cedric.xing@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/entry/vdso/vsgx.S         | 54 ++++++++++----------------
 arch/x86/include/asm/enclu.h       |  1 +
 arch/x86/include/uapi/asm/sgx.h    | 61 +++++++++++-------------------
 tools/testing/selftests/sgx/main.c | 24 ++++++------
 4 files changed, 55 insertions(+), 85 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S
index 8f8190ab9ed5..4bbb71253e17 100644
--- a/arch/x86/entry/vdso/vsgx.S
+++ b/arch/x86/entry/vdso/vsgx.S
@@ -7,25 +7,17 @@
 
 #include "extable.h"
 
-/* Offset of 'struct sgx_enclave_run' relative to %rbp. */
-#define SGX_ENCLAVE_RUN_PTR	2*8
-
-/* Offsets into 'struct sgx_enclave_run'. */
-#define SGX_ENCLAVE_RUN_TCS		0*8
-#define SGX_ENCLAVE_RUN_FLAGS		1*8
-#define SGX_ENCLAVE_RUN_EXIT_REASON	1*8 + 4
-#define SGX_ENCLAVE_RUN_USER_HANDLER	2*8
-/* #define SGX_ENCLAVE_RUN_USER_DATA	3*8 */
-#define SGX_ENCLAVE_RUN_EXCEPTION	4*8
-
-#define SGX_SYNCHRONOUS_EXIT		0
-#define SGX_EXCEPTION_EXIT		1
-
-/* Offsets into sgx_enter_enclave.exception. */
-#define SGX_EX_LEAF			0*8
-#define SGX_EX_TRAPNR			0*8+4
-#define SGX_EX_ERROR_CODE		0*8+6
-#define SGX_EX_ADDRESS			1*8
+/* The offset relative to %rbp. */
+#define SGX_ENCLAVE_RUN_OFFSET	2*8
+
+/* The offsets relative to struct sgx_enclave_run. */
+#define SGX_ENCLAVE_RUN_TCS			0*8
+#define SGX_ENCLAVE_RUN_USER_HANDLER		1*8
+
+#define SGX_ENCLAVE_RUN_LEAF			3*8
+#define SGX_ENCLAVE_RUN_EXCEPTION_NUMBER	4*8
+#define SGX_ENCLAVE_RUN_EXCEPTION_INFO		5*8
+#define SGX_ENCLAVE_RUN_EXCEPTION_ADDR		6*8
 
 .code64
 .section .text, "ax"
@@ -49,11 +41,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	cmp	$ERESUME, %eax
 	ja	.Linvalid_input
 
-	mov	SGX_ENCLAVE_RUN_PTR(%rbp), %rcx
-
-	/* No flags are currently defined/supported. */
-	cmpl	$0, SGX_ENCLAVE_RUN_FLAGS(%rcx)
-	jne	.Linvalid_input
+	mov	SGX_ENCLAVE_RUN_OFFSET(%rbp), %rcx
 
 	/* Load TCS and AEP */
 	mov	SGX_ENCLAVE_RUN_TCS(%rcx), %rbx
@@ -65,10 +53,10 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	enclu
 
 	/* EEXIT jumps here unless the enclave is doing something fancy. */
-	mov	SGX_ENCLAVE_RUN_PTR(%rbp), %rbx
+	mov	SGX_ENCLAVE_RUN_OFFSET(%rbp), %rbx
 
 	/* Set exit_reason. */
-	movl	$SGX_SYNCHRONOUS_EXIT, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
+	movl	$EEXIT, SGX_ENCLAVE_RUN_LEAF(%rbx)
 
 	/* Invoke userspace's exit handler if one was provided. */
 .Lhandle_exit:
@@ -92,15 +80,13 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	jmp	.Lout
 
 .Lhandle_exception:
-	mov	SGX_ENCLAVE_RUN_PTR(%rbp), %rbx
-
-	/* Set the exit_reason and exception info. */
-	movl	$SGX_EXCEPTION_EXIT, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
+	mov	SGX_ENCLAVE_RUN_OFFSET(%rbp), %rbx
 
-	mov	%eax, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_LEAF)(%rbx)
-	mov	%di,  (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_TRAPNR)(%rbx)
-	mov	%si,  (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_ERROR_CODE)(%rbx)
-	mov	%rdx, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_ADDRESS)(%rbx)
+	/* Set the exception info. */
+	mov	%eax, (SGX_ENCLAVE_RUN_LEAF)(%rbx)
+	mov	%di,  (SGX_ENCLAVE_RUN_EXCEPTION_NUMBER)(%rbx)
+	mov	%si,  (SGX_ENCLAVE_RUN_EXCEPTION_INFO)(%rbx)
+	mov	%rdx, (SGX_ENCLAVE_RUN_EXCEPTION_ADDR)(%rbx)
 	jmp	.Lhandle_exit
 
 .Linvoke_userspace_handler:
diff --git a/arch/x86/include/asm/enclu.h b/arch/x86/include/asm/enclu.h
index 06157b3e9ede..b1314e41a744 100644
--- a/arch/x86/include/asm/enclu.h
+++ b/arch/x86/include/asm/enclu.h
@@ -4,5 +4,6 @@
 
 #define EENTER	0x02
 #define ERESUME	0x03
+#define EEXIT	0x04
 
 #endif /* _ASM_X86_ENCLU_H */
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 236dd7190431..71e44406da13 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -96,42 +96,25 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
 					  struct sgx_enclave_run *run);
 
 /**
- * struct sgx_enclave_exception - structure to report exceptions encountered in
- *				  __vdso_sgx_enter_enclave()
- * @leaf:	ENCLU leaf from \%eax at time of exception
- * @trapnr:	exception trap number, a.k.a. fault vector
- * @error_code:	exception error code
- * @address:	exception address, e.g. CR2 on a #PF
- */
-struct sgx_enclave_exception {
-	__u32 leaf;
-	__u16 trapnr;
-	__u16 error_code;
-	__u64 address;
-};
-
-/**
- * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave()
- * @tcs:		Thread Control Structure used to enter enclave
- * @flags:		Control flags
- * @exit_reason:	Cause of exit from enclave, e.g. EEXIT vs. exception
- * @user_handler:	User provided exit handler (optional)
- * @user_data:		User provided opaque value (optional)
- * @exception:		Valid on exit due to exception
+ * struct sgx_enclave_run - the execution context of __vdso_sgx_enter_enclave()
+ * @tcs:			TCS used to enter the enclave
+ * @aex_handler:		Handler for asynchronous exit (AEX)
+ * @aex_data:			Data passed to the AEX handler
+ * @leaf:			The ENCLU leaf we were at (EENTER/ERESUME/EEXIT)
+ * @exception_number:		The exception number that triggered AEX
+ * @exception_info:		The associated info pulled out from the stack
+ * @exception_addr:		The address that triggered the exception
+ * @reserved			Reserved for possible future use
  */
 struct sgx_enclave_run {
 	__u64 tcs;
-	__u32 flags;
-	__u32 exit_reason;
-	__u64 user_handler;
-	__u64 user_data;
-
-	union {
-		struct sgx_enclave_exception exception;
-
-		/* Pad the entire struct to 256 bytes. */
-		__u8 pad[256 - 32];
-	};
+	__u64 aex_handler;
+	__u64 aex_data;
+	__u32 leaf;
+	__u16 exception_number;
+	__u16 exception_info;
+	__u64 exception_addr;
+	__u64 reserved[3];
 };
 
 /**
@@ -157,10 +140,10 @@ struct sgx_enclave_run {
  * loaded with @leaf, asynchronous exit pointer, and @run.tcs respectively.
  *
  * RBP and the stack are used to anchor __vdso_sgx_enter_enclave() to the
- * pre-enclave state, e.g. to retrieve @run.exception and @run.user_handler
+ * pre-enclave state, e.g. to retrieve @run.exception and @run.aex_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
+ * stack (and modify RSP) to pass information to the optional AEX handler (see
  * below).
  *
  * Most exceptions reported on ENCLU, including those that occur within the
@@ -170,22 +153,22 @@ struct sgx_enclave_run {
  * reported exceptions, -EFAULT is returned and details about the exception are
  * recorded in @run.exception, the optional sgx_enclave_exception struct.
  *
- * If an exit handler is provided, the handler will be invoked on synchronous
+ * If an AEX handler is provided, the handler will be invoked on synchronous
  * exits from the enclave and for all synchronously reported exceptions. In
  * latter case, @run.exception is filled prior to invoking the handler.
  *
- * The exit handler's return value is interpreted as follows:
+ * The AEX 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,
+ * The AEX handler may transfer control, e.g. via longjmp() or C++ exception,
  * without returning to __vdso_sgx_enter_enclave().
  *
  * Return:
  *  0 on success (ENCLU reached),
  *  -EINVAL if ENCLU leaf is not allowed,
- *  -errno for all other negative values returned by the userspace exit handler
+ *  -errno for all other negative values returned by the userspace AEX handler
  */
 typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
 					unsigned long rdx, unsigned int leaf,
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 0316701d6bbd..cc9e71b29861 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -129,26 +129,26 @@ int check_result(struct sgx_enclave_run *run, int ret, uint64_t result,
 	if (ret) {
 		printf("FAIL: %s() returned: %d\n", test, ret);
 		return ret;
-	} else if (run->exit_reason != SGX_SYNCHRONOUS_EXIT) {
-		printf("FAIL: %s() exit reason, expected: %u, got: %u\n",
-		       test, SGX_SYNCHRONOUS_EXIT, run->exit_reason);
+	} else if (run->leaf != EEXIT) {
+		printf("FAIL: %s() leaf, expected: %u, got: %u\n", test, EEXIT,
+		       run->leaf);
 		return -EIO;
 	} else if (result != MAGIC) {
-		printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n",
-		       test, MAGIC, result);
+		printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n", test, MAGIC,
+		       result);
 		return -EIO;
-	} else if (run->user_data) {
+	} else if (run->aex_data) {
 		printf("FAIL: %s() user data, expected: 0x0, got: 0x%llx\n",
-		       test, run->user_data);
+		       test, run->aex_data);
 		return -EIO;
 	}
 	return 0;
 }
 
-static int exit_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
+static int aex_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
 			struct sgx_enclave_run *run)
 {
-	run->user_data = 0;
+	run->aex_data = 0;
 	return 0;
 }
 
@@ -215,11 +215,11 @@ int main(int argc, char *argv[], char *envp[])
 		goto err;
 
 	/* And with an exit handler. */
-	run.user_handler = (__u64)exit_handler;
-	run.user_data = 0xdeadbeef;
+	run.aex_handler = (__u64)aex_handler;
+	run.aex_data = 0xdeadbeef;
 	ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER,
 		     0, 0, &run);
-	if (check_result(&run, ret, result, "exit_handler"))
+	if (check_result(&run, ret, result, "aex_handler"))
 		goto err;
 
 	printf("SUCCESS\n");
-- 
2.25.1

Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Sean Christopherson]

Can you split this into a proper series?  There are many different things
going on here, reviewing everything at once is difficult.

On Thu, Oct 01, 2020 at 07:16:42PM +0300, Jarkko Sakkinen wrote:
> Reduce the struct size to 64 bytes. It makes sense to have a naturally
> aligning struct size and it leaves 24 bytes of spare space for possible
> future expansion. Having 256 bytes is over the top without any reasonable
> explanation, which does not exist in the commit for the SGX vDSO.

I'm all for dropping the padding, but we should drop reserved fields
altogether and instead reserve space for future features by keeping 'flags',
enforcing it to be zero, and conditioning consumption of future fields on
opt-in flags being set.
 
> Drop struct sgx_exception. It does not serve any semantic purpose in the
> context of this vDSO. Doing this is also consistent with user_handler and

IMO it does serve a purpose, it helps convey that it's a package deal, i.e.
either all fields are valid or none are valid.

> user_data fields, as neither are they encapsulated in an embedded data
> structure.
> 
> Drop 'flags' from sgx_enclave_run as it is an unused value.'vsgx.S'
> literally states this in an inline comment.

It's not unused, it's reserved to zero.

> Pick more reasonable names for the four fields of the exception. For
> example, 'trapnr' should rather be just 'number' because in practice we
> deliver only trap and the rest are faults.

No, in Intel terminology both faults and traps are delivered.  E.g. #DBs
are traps (except for code breakpoints and INT1), #PF and #GP are faults.

That being said, I'm all for changing the 'trapnr' name.  I grabbed it from
from the kernel's exception fixup, it wasn't deliberately chosen to imply
that only trap-like exceptions are reported to userspace.

> Also 'vector' would be an
> adequate name for this field. Document the fields properly in order to
> better explain the values that they have inherited.

My vote is for 'vector'.

> Remove 'exit_reason' as it is really a redundant field to save some space.
> Instead, have just a field called 'leaf' that tells the last seen ENCLU.

This justification contradicts the removal of flags.  If you want to shave
bytes, keep flags and enforce that flags is zero so that you don't need to
have reserved bytes at the end of the struct.

IMO, having a separate exit reason provides saner code for userspace, e.g. 

  if (run->exit_reason == SGX_EXCEPTION_EXIT)
	<do exception stuff>

versus

  if (run->leaf == EENTER || run->leaf == ERESUME)
        <do exception stuff>

I would prefer to be explicit about why the vDSO is transferring control to
the caller or its handler.  It also gives us line of sight to supporting
exit types other than EEXIT and exceptions.  Maybe we never end up with
another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of
ending up with a mess of an API if another exit reason comes along.

> The documentation, selftest and the kernel source code use the terms "user
> handler" and "exit handler" in somewhat inconsistent manner. Start using
> "AEX handler" consistently as this is the term that Intel SDM volume D uses
> for these events.

No, AEX is specifically for "asynchronus" exits.  In this context, that is
limited to the exception path.  The vDSO invokes the user handler for EEXIT,
i.e. the synchrous path, which is not an AEX.

Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Jarkko Sakkinen]

On Thu, Oct 01, 2020 at 09:51:51AM -0700, Sean Christopherson wrote:
> Can you split this into a proper series?  There are many different things
> going on here, reviewing everything at once is difficult.
> 
> On Thu, Oct 01, 2020 at 07:16:42PM +0300, Jarkko Sakkinen wrote:
> > Reduce the struct size to 64 bytes. It makes sense to have a naturally
> > aligning struct size and it leaves 24 bytes of spare space for possible
> > future expansion. Having 256 bytes is over the top without any reasonable
> > explanation, which does not exist in the commit for the SGX vDSO.
> 
> I'm all for dropping the padding, but we should drop reserved fields
> altogether and instead reserve space for future features by keeping 'flags',
> enforcing it to be zero, and conditioning consumption of future fields on
> opt-in flags being set.

I do not want to maintain ABI where structs change their size based on
some field. It is way way too flakky. I fullly get some padding. I
rather keep the padding as large as it was than do that.

But you did catch a critical bug in my patch: the padding must be zero
checked through, and return -EINVAL if not all bytes are zero. Then that
space is a future playground.

I scaled down to 64 bytes because it is the rounding to the next nicely
alignable size (40 -> 64) and because three qwords is a lot of space
to play for future expansion.

'flags' wastes qword of space if we end up not needing such field.

With a good argument I can grow the struct to 128 or 256 bytes but
it has to be a constant. Without argument nearest 2**n is the most
logical choice.

What I should add vsgx.S, is a zero validation of the reserved area.

> > Drop struct sgx_exception. It does not serve any semantic purpose in the
> > context of this vDSO. Doing this is also consistent with user_handler and
> 
> IMO it does serve a purpose, it helps convey that it's a package deal, i.e.
> either all fields are valid or none are valid.

If there is no semantics that require such structure, then it is not
important enough to exist.

The 'leaf' will tell you whether you need to ignore exception fields or
not.

And not all fields are valid for all expections (e.g. the info put into
stack).

> > user_data fields, as neither are they encapsulated in an embedded data
> > structure.
> > 
> > Drop 'flags' from sgx_enclave_run as it is an unused value.'vsgx.S'
> > literally states this in an inline comment.
> 
> It's not unused, it's reserved to zero.

My response above.

> > Pick more reasonable names for the four fields of the exception. For
> > example, 'trapnr' should rather be just 'number' because in practice we
> > deliver only trap and the rest are faults.
> 
> No, in Intel terminology both faults and traps are delivered.  E.g. #DBs
> are traps (except for code breakpoints and INT1), #PF and #GP are faults.
> 
> That being said, I'm all for changing the 'trapnr' name.  I grabbed it from
> from the kernel's exception fixup, it wasn't deliberately chosen to imply
> that only trap-like exceptions are reported to userspace.

I based my information to Table 6-1 in Intel SDM volume 3A.

Unfortunately, the quoted sentence above is missing one word, overflow,
it is a typo. It should have been:

"... we deliver only #OD trap and the rest are faults"

The other traps, I think, were not fixed up but I could be wrong if I
misread something from the table (was quite tired at that point).

If I recheck that table in the SDM, the majority of stuff is categorized
as "Fault". From this I deduced that trapnr is not aligned with the SDM
terminology too well (being fully aware that kernel uses this
internally).

Anyway, I guess we go with 'vector', right? Probably there is some
miunderstanding but with the name 'vector' we are in the same page :-)

> > Also 'vector' would be an
> > adequate name for this field. Document the fields properly in order to
> > better explain the values that they have inherited.
> 
> My vote is for 'vector'.
> 
> > Remove 'exit_reason' as it is really a redundant field to save some space.
> > Instead, have just a field called 'leaf' that tells the last seen ENCLU.
> 
> This justification contradicts the removal of flags.  If you want to shave
> bytes, keep flags and enforce that flags is zero so that you don't need to
> have reserved bytes at the end of the struct.
> 
> IMO, having a separate exit reason provides saner code for userspace, e.g. 
> 
>   if (run->exit_reason == SGX_EXCEPTION_EXIT)
> 	<do exception stuff>
> 
> versus
> 
>   if (run->leaf == EENTER || run->leaf == ERESUME)
>         <do exception stuff>
> 
> I would prefer to be explicit about why the vDSO is transferring control to
> the caller or its handler.  It also gives us line of sight to supporting
> exit types other than EEXIT and exceptions.  Maybe we never end up with
> another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of
> ending up with a mess of an API if another exit reason comes along.

'exit_reason' is just waste of expensive memory space that we have
limited fixed amount of. Using the same field in a *guaranteed*
non-conflicting manner saves space. There is absolutely no question
that what I did should not be done.

And the semantics are intuitive: it will tell what the latest E-thing
before arriving here (or subset of E's that transfrom from context to
context).

Maybe it should be prefixed with something like 'latest_', 'prev_'
or perhaps 'last_', Not sure which would be the most adequate prefix.

> > The documentation, selftest and the kernel source code use the terms "user
> > handler" and "exit handler" in somewhat inconsistent manner. Start using
> > "AEX handler" consistently as this is the term that Intel SDM volume D uses
> > for these events.
> 
> No, AEX is specifically for "asynchronus" exits.  In this context, that is
> limited to the exception path.  The vDSO invokes the user handler for EEXIT,
> i.e. the synchrous path, which is not an AEX.

OK, I but this one.

I got gray hairs because:

- API called it user handler.
- Documentation called it exit handler.
- Selftest called it exit handler.

I think user_handler is too generic and abstract but I'm happy to hear
other proposals than 'exit_' to which I will change given your feedback.

I think my flattening patch is doing the right thing overally. E.g.
packing exception and eexit to leaf came obvious when I did it. It also
makes doing all sorts of bindings to all sorts of languages a bit
easier, especially if you generate the bindings. It's simply the right
thing to do.

I only did this patch because I really now concertrated what I'm seeing
because of documenting things and came into conclusion that the existing
API was such that I simply cannot commit to maintain for years ahead.

/Jarkko

Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Sean Christopherson]

On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 01, 2020 at 09:51:51AM -0700, Sean Christopherson wrote:
> > > Remove 'exit_reason' as it is really a redundant field to save some space.
> > > Instead, have just a field called 'leaf' that tells the last seen ENCLU.
> > 
> > This justification contradicts the removal of flags.  If you want to shave
> > bytes, keep flags and enforce that flags is zero so that you don't need to
> > have reserved bytes at the end of the struct.
> > 
> > IMO, having a separate exit reason provides saner code for userspace, e.g. 
> > 
> >   if (run->exit_reason == SGX_EXCEPTION_EXIT)
> > 	<do exception stuff>
> > 
> > versus
> > 
> >   if (run->leaf == EENTER || run->leaf == ERESUME)
> >         <do exception stuff>
> > 
> > I would prefer to be explicit about why the vDSO is transferring control to
> > the caller or its handler.  It also gives us line of sight to supporting
> > exit types other than EEXIT and exceptions.  Maybe we never end up with
> > another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of
> > ending up with a mess of an API if another exit reason comes along.
> 
> 'exit_reason' is just waste of expensive memory space that we have
> limited fixed amount of.

Huh?  4 bytes on the user stack can't possibly be a real concern, the default
stack size on x86-64 is 8MB.  Golang shaves that down to 2kb but dynamically
adjusts the stack as needed.  The "allocation" is completely free, the caller
will already be adjusting RSP to create space for the struct, and it doesn't
need to be initialized.

> Using the same field in a *guaranteed*
> non-conflicting manner saves space. There is absolutely no question
> that what I did should not be done.

But you can't guarantee that it doesn't conflict with future changes to the
SGX architecture.  Hell, you can't even guarantee that it doesn't conflict
with the current SGX architecture, e.g. see Jethro's request to invoke the
handler on AEX's due to interrupts.

Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Jarkko Sakkinen]

On Thu, Oct 01, 2020 at 03:50:29PM -0700, Sean Christopherson wrote:
> On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 01, 2020 at 09:51:51AM -0700, Sean Christopherson wrote:
> > > > Remove 'exit_reason' as it is really a redundant field to save some space.
> > > > Instead, have just a field called 'leaf' that tells the last seen ENCLU.
> > > 
> > > This justification contradicts the removal of flags.  If you want to shave
> > > bytes, keep flags and enforce that flags is zero so that you don't need to
> > > have reserved bytes at the end of the struct.
> > > 
> > > IMO, having a separate exit reason provides saner code for userspace, e.g. 
> > > 
> > >   if (run->exit_reason == SGX_EXCEPTION_EXIT)
> > > 	<do exception stuff>
> > > 
> > > versus
> > > 
> > >   if (run->leaf == EENTER || run->leaf == ERESUME)
> > >         <do exception stuff>
> > > 
> > > I would prefer to be explicit about why the vDSO is transferring control to
> > > the caller or its handler.  It also gives us line of sight to supporting
> > > exit types other than EEXIT and exceptions.  Maybe we never end up with
> > > another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of
> > > ending up with a mess of an API if another exit reason comes along.
> > 
> > 'exit_reason' is just waste of expensive memory space that we have
> > limited fixed amount of.
> 
> Huh?  4 bytes on the user stack can't possibly be a real concern, the default
> stack size on x86-64 is 8MB.  Golang shaves that down to 2kb but dynamically
> adjusts the stack as needed.  The "allocation" is completely free, the caller
> will already be adjusting RSP to create space for the struct, and it doesn't
> need to be initialized.

It's unnecessary in the space that is chosen for the run structure. 64
bytes takes only one cacheline and the padding is also fast to validate
to contain only zeros (check three qwords).

Maybe I'm just weird but I'd just store all the leafs numbers in the
field that is designated to store leaf numbers.

> > Using the same field in a *guaranteed*
> > non-conflicting manner saves space. There is absolutely no question
> > that what I did should not be done.
> 
> But you can't guarantee that it doesn't conflict with future changes to the
> SGX architecture.  Hell, you can't even guarantee that it doesn't conflict
> with the current SGX architecture, e.g. see Jethro's request to invoke the
> handler on AEX's due to interrupts.

Reserved space can be used for any sort of new fields as long as the
whole structure is validated, including the padding. With or without
'flags' this should be always done.

Then there is no issue to add a new field after the merge. And thus,
there is no issue resolving [*], even if the 'flags' is not added
right now. You don't simply add stuff that is useless before it gets
used. You ensure that your data is in stable state. That's all one
hould do.

[*] https://lore.kernel.org/linux-sgx/0f88f6d7-fa2d-59b0-c90f-4aa9e04d0af5@fortanix.com/

/Jarkko

Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Jarkko Sakkinen]

On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote:
> > > The documentation, selftest and the kernel source code use the terms "user
> > > handler" and "exit handler" in somewhat inconsistent manner. Start using
> > > "AEX handler" consistently as this is the term that Intel SDM volume D uses
> > > for these events.
> > 
> > No, AEX is specifically for "asynchronus" exits.  In this context, that is
> > limited to the exception path.  The vDSO invokes the user handler for EEXIT,
> > i.e. the synchrous path, which is not an AEX.
> 
> OK, I but this one.
> 
> I got gray hairs because:
> 
> - API called it user handler.
> - Documentation called it exit handler.
> - Selftest called it exit handler.
> 
> I think user_handler is too generic and abstract but I'm happy to hear
> other proposals than 'exit_' to which I will change given your feedback.

OK, so here I use my "too generic" argument pick user handler anyhow
because we don't know what the heck the handler is doing.

'exit_handler' implies that not only it would be called for enclave
exit resulting from an exception, but also for EEXIT path.

/Jarkko

Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Jarkko Sakkinen]

On Fri, Oct 02, 2020 at 07:25:05AM +0300, Jarkko Sakkinen wrote:
> On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote:
> > > > The documentation, selftest and the kernel source code use the terms "user
> > > > handler" and "exit handler" in somewhat inconsistent manner. Start using
> > > > "AEX handler" consistently as this is the term that Intel SDM volume D uses
> > > > for these events.
> > > 
> > > No, AEX is specifically for "asynchronus" exits.  In this context, that is
> > > limited to the exception path.  The vDSO invokes the user handler for EEXIT,
> > > i.e. the synchrous path, which is not an AEX.
> > 
> > OK, I but this one.
> > 
> > I got gray hairs because:
> > 
> > - API called it user handler.
> > - Documentation called it exit handler.
> > - Selftest called it exit handler.
> > 
> > I think user_handler is too generic and abstract but I'm happy to hear
> > other proposals than 'exit_' to which I will change given your feedback.
> 
> OK, so here I use my "too generic" argument pick user handler anyhow
> because we don't know what the heck the handler is doing.
> 
> 'exit_handler' implies that not only it would be called for enclave
> exit resulting from an exception, but also for EEXIT path.

Wile editing the commit message I read what I had written about flags.
It was quite terrible, to say the least.

I'll send update soon.

/Jarkko

Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

[Jarkko Sakkinen]

On Fri, Oct 02, 2020 at 05:27:56PM +0300, Jarkko Sakkinen wrote:
> On Fri, Oct 02, 2020 at 07:25:05AM +0300, Jarkko Sakkinen wrote:
> > On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote:
> > > > > The documentation, selftest and the kernel source code use the terms "user
> > > > > handler" and "exit handler" in somewhat inconsistent manner. Start using
> > > > > "AEX handler" consistently as this is the term that Intel SDM volume D uses
> > > > > for these events.
> > > > 
> > > > No, AEX is specifically for "asynchronus" exits.  In this context, that is
> > > > limited to the exception path.  The vDSO invokes the user handler for EEXIT,
> > > > i.e. the synchrous path, which is not an AEX.
> > > 
> > > OK, I but this one.
> > > 
> > > I got gray hairs because:
> > > 
> > > - API called it user handler.
> > > - Documentation called it exit handler.
> > > - Selftest called it exit handler.
> > > 
> > > I think user_handler is too generic and abstract but I'm happy to hear
> > > other proposals than 'exit_' to which I will change given your feedback.
> > 
> > OK, so here I use my "too generic" argument pick user handler anyhow
> > because we don't know what the heck the handler is doing.
> > 
> > 'exit_handler' implies that not only it would be called for enclave
> > exit resulting from an exception, but also for EEXIT path.
> 
> Wile editing the commit message I read what I had written about flags.
> It was quite terrible, to say the least.
> 
> I'll send update soon.

When I added validation, I noticed that the offsets were wrong. Fixing
this but I think this is also one good reason for always validate the
full struct and not just some flags-field. It helps to catch bugs early
on [*].

Not going to do selftest improvements before sending v39 but the
selftest needs even more exception test than provision. I think I work
on both of these after the patch set has been sent.

For exception, wouldn't be easiest to add a 2nd TCS and code path that
trigger's #PF / EPCM conflict? I could add a write operation on a
read-only page.

[*] Proudly admitting that in v1 of this patch everything was wrong
    as far as flags-field is considered: no flags, no padding
    validation and no legit reasoning.

/Jarkko