Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX enclave call

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Fri, Mar 13, 2020 at 11:48:54AM -0400, Nathaniel McCallum wrote:
> Thinking about this more carefully, I still think that at least part
> of my critique still stands.
> 
> __vdso_sgx_enter_enclave() doesn't use the x86-64 ABI. This means that
> there will always be an assembly wrapper for
> __vdso_sgx_enter_enclave(). But because __vdso_sgx_enter_enclave()
> doesn't save %rbx, the wrapper is forced to in order to be called from
> C.
> 
> A common pattern for the wrapper will be to do something like this:
> 
> # void enter_enclave(rdi, rsi, rdx, unused, r8, r9, @tcs, @e,
> @handler, @leaf, @vdso)
> enter_enclave:
>     push %rbx
>     push $0 /* align */
>     push 0x48(%rsp)
>     push 0x48(%rsp)
>     push 0x48(%rsp)
> 
>     mov 0x70(%rsp), %eax
>     call *0x68(%rsp)
> 
>     add $0x20, %rsp
>     pop %rbx
>     ret
> 
> Because __vdso_sgx_enter_enclave() doesn't preserve %rbx, the wrapper
> is forced to reposition stack parameters in a performance-critical
> path. On the other hand, if __vdso_sgx_enter_enclave() preserved %rbx,
> you could implement the above as:
> 
> # void enter_enclave(rdi, rsi, rdx, unused, r8, r9, @tcs, @e,
> @handler, @leaf, @vdso)
> enter_enclave:
>     mov 0x20(%rsp), %eax
>     jmp *0x28(%rsp)
> 
> This also implies that if __vdso_sgx_enter_enclave() took @leaf as a
> stack parameter and preserved %rbx, it would be x86-64 ABI compliant
> enough to call from C if the enclave preserves all callee-saved
> registers besides %rbx (Enarx does).
> 
> What are the downsides of this approach? It also doesn't harm the more
> extended case when you need to use an assembly wrapper to setup
> additional registers. This can still be done. It does imply an extra
> push and mov instruction. But because there are currently an odd
> number of stack function parameters, the push also removes an
> alignment instruction where the stack is aligned before the call to
> __vdso_sgx_enter_enclave() (likely). Further, the push and mov are
> going to be performed by *someone* in order to call
> __vdso_sgx_enter_enclave() from C.
> 
> Therefore, I'd like to propose that __vdso_sgx_enter_enclave():
>   * Preserve %rbx.

At first glance, that looks sane.  Being able to call __vdso... from C
would certainly be nice.

>   * Take the leaf as an additional stack parameter instead of passing
> it in %rax.

Does the leaf even need to be a stack param?  Wouldn't it be possible to
use %rcx as @leaf instead of @unusued?  E.g.

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)
{
	push	%rbp
	mov	%rsp, %rbp
	push	%rbx

	mov	%ecx, %eax
.Lenter_enclave
	cmp	$0x2, %eax
	jb	.Linvalid_leaf
	cmp	$0x3, %eax
	ja	.Linvalid_leaf

	mov	0x0x10(%rbp), %rbx
	lea	.Lasync_exit_pointer(%rip), %rcx

.Lasync_exit_pointer:
.Lenclu_eenter_eresume:
	enclu

	xor	%eax, %eax

.Lhandle_exit:
	cmp	$0, 0x20(%rbp)
	jne	.Linvoke_userspace_handler

.Lout
	pop	%rbx
	leave
	ret
}
		

> Then C can call it without additional overhead. And people who need to
> "extend" the C ABI can still do so.
>