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

["Xing, Cedric" <cedric.xing@intel.com>]

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

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

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

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

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

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

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

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

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

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

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

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

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

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