Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: "Xing, Cedric" <cedric.xing@intel.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	linux-sgx@vger.kernel.org
Subject: Re: [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback"
Date: Thu, 10 Oct 2019 16:59:31 -0700
Message-ID: <20191010235931.GI23798@linux.intel.com> (raw)
In-Reply-To: <ff6afbe4-1fa9-a632-b5bf-2cdadbb50f37@intel.com>

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

  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-08  4:45 [PATCH for_v23 00/16] x86/vdso: sgx: Major vDSO cleanup Sean Christopherson
2019-10-08  4:45 ` [PATCH for_v23 01/16] x86/vdso: sgx: Drop the pseudocode "documentation" Sean Christopherson
2019-10-08  4:45 ` [PATCH for_v23 02/16] x86/vdso: sgx: Do not use exception info to pass success/failure Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 03/16] x86/vdso: sgx: Rename the enclave exit handler typedef Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 04/16] x86/vdso: sgx: Move enclave exit handler declaration to UAPI header Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 05/16] x86/vdso: sgx: Add comment regarding kernel-doc shenanigans Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 06/16] x86/vdso: sgx: Rewrite __vdso_sgx_enter_enclave() function comment Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 07/16] selftests/x86: Fix linker warning in SGX selftest Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 08/16] selftests/x86/sgx: Use getauxval() to retrieve the vDSO base address Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 09/16] selftests/x86/sgx: Add helper function and macros to assert results Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 10/16] selftests/x86/sgx: Handle setup failures via test assertions Sean Christopherson
2019-10-15 10:16   ` Jarkko Sakkinen
2019-10-15 10:24     ` Jarkko Sakkinen
2019-10-15 10:25       ` Jarkko Sakkinen
2019-10-15 11:03         ` Jarkko Sakkinen
2019-10-15 16:27           ` Sean Christopherson
2019-10-16 10:20             ` Jarkko Sakkinen
2019-10-16 20:21         ` Sean Christopherson
2019-10-15 16:18     ` Sean Christopherson
2019-10-16 10:19       ` Jarkko Sakkinen
2019-10-08  4:46 ` [PATCH for_v23 11/16] selftests/x86/sgx: Sanitize the types for sgx_call()'s input params Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 12/16] selftests/x86/sgx: Move existing sub-test to a separate helper Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 13/16] selftests/x86/sgx: Add a test of the vDSO exception reporting mechanism Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 14/16] selftests/x86/sgx: Add test of vDSO with basic exit handler Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 15/16] selftests/x86/sgx: Add sub-test for exception behavior with " Sean Christopherson
2019-10-08  4:46 ` [PATCH for_v23 16/16] x86/vdso: sgx: Rework __vdso_sgx_enter_enclave() to prefer "no callback" Sean Christopherson
2019-10-09 18:00   ` Xing, Cedric
2019-10-09 19:10     ` Sean Christopherson
2019-10-10  0:21       ` Sean Christopherson
2019-10-10 17:49       ` Xing, Cedric
2019-10-10 23:59         ` Sean Christopherson [this message]
2019-10-16 22:18           ` Xing, Cedric
2019-10-16 22:53             ` Sean Christopherson
2019-10-10  8:10 ` [PATCH for_v23 00/16] x86/vdso: sgx: Major vDSO cleanup Jarkko Sakkinen
2019-10-10 16:08   ` Sean Christopherson
2019-10-14 21:04     ` Jarkko Sakkinen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191010235931.GI23798@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=cedric.xing@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=linux-sgx@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org
	public-inbox-index linux-sgx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git