Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Nathaniel McCallum <npmccallum@redhat.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Jethro Beekman <jethro@fortanix.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
	dave.hansen@intel.com, Neil Horman <nhorman@redhat.com>,
	"Huang, Haitao" <haitao.huang@intel.com>,
	andriy.shevchenko@linux.intel.com, tglx@linutronix.de, "Svahn,
	Kai" <kai.svahn@intel.com>,
	bp@alien8.de, Josh Triplett <josh@joshtriplett.org>,
	luto@kernel.org, kai.huang@intel.com,
	David Rientjes <rientjes@google.com>,
	cedric.xing@intel.com, Patrick Uiterwijk <puiterwijk@redhat.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Connor Kuehl <ckuehl@redhat.com>,
	Harald Hoyer <harald@redhat.com>,
	Lily Sturmann <lsturman@redhat.com>
Subject: Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX enclave call
Date: Fri, 13 Mar 2020 14:32:29 -0400
Message-ID: <CAOASepN1hxSgxVJAJiAbSmuCTCHd=95Mnvh6BKNSPJs=EpAmbQ@mail.gmail.com> (raw)
In-Reply-To: <20200313164622.GC5181@linux.intel.com>

On Fri, Mar 13, 2020 at 12:46 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> 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.

Agreed. I think ergonomically we want __vdso...() to be called from C
and the handler to be implemented in asm (optionally); without
breaking the ability to call __vdso..() from asm in special cases.

I think all ergonomic issues get solved by the following:
   * Pass a void * into the handler from C through __vdso...().
   * Allow the handler to pop parameters off of the output stack without hacks.

This allows the handler to pop extra arguments off the stack and write
them into the memory at the void *. Then the handler can be very small
and pass logic back to the caller of __vdso...().

Here's what this all means for the enclave. For maximum usability, the
enclave should preserve all callee-saved registers (except %rbx, which
is preserved by __vdso..()). For each ABI rule that the enclave
breaks, you need logic in a handler to fix it. So if you push return
params on the stack, the handler needs to undo that.

This doesn't compromise the ability to treat __vsdo...() like ENCLU if
you need the full power. But it does make it significantly easier to
consume when you don't have special needs. So as I see it, __vdso...()
should:

1. preserve %rbx
2. take leaf in %rcx
3. gain a void* stack param which is passed to the handler
4. sub/add to %rsp rather than save/restore

That would make this a very usable and fast interface without
sacrificing any of its current power.

> >   * 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.

Even better!

> 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.
> >
>


  reply index

Thread overview: 109+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-03 23:35 [PATCH v28 00/22] Intel SGX foundations Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 01/22] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 02/22] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 03/22] x86/cpufeatures: x86/msr: Intel SGX Launch Control " Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 04/22] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 05/22] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 06/22] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2020-03-09 21:14   ` Sean Christopherson
2020-03-03 23:35 ` [PATCH v28 07/22] x86/cpu/intel: Detect SGX supprt Jarkko Sakkinen
2020-03-09 21:56   ` Sean Christopherson
2020-03-11 17:03     ` Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 08/22] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 09/22] x86/sgx: Add functions to allocate and free EPC pages Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 10/22] mm: Introduce vm_ops->may_mprotect() Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 11/22] x86/sgx: Linux Enclave Driver Jarkko Sakkinen
2020-03-05 17:40   ` Sean Christopherson
2020-03-05 18:24     ` Jethro Beekman
2020-03-05 19:04       ` Sean Christopherson
2020-03-06 19:00       ` Jarkko Sakkinen
2020-03-19 18:22         ` Dr. Greg
2020-03-06 18:58     ` Jarkko Sakkinen
2020-03-03 23:35 ` [PATCH v28 12/22] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 13/22] selftests/x86: Recurse into subdirectories Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 14/22] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-03-04 19:27   ` Nathaniel McCallum
2020-03-05 11:33     ` Jarkko Sakkinen
2020-03-06 15:42       ` Dr. Greg
2020-03-06 19:07         ` Jarkko Sakkinen
2020-03-07 17:42           ` Dr. Greg
2020-03-10 13:08             ` Jarkko Sakkinen
2020-03-11 13:28       ` Jarkko Sakkinen
     [not found]         ` <20200311164047.GG21852@linux.intel.com>
2020-03-13 19:24           ` Jarkko Sakkinen
2020-03-04 19:44   ` Nathaniel McCallum
2020-03-04 19:51   ` Nathaniel McCallum
2020-03-06  5:32   ` Dr. Greg
2020-03-06 19:04     ` Jarkko Sakkinen
2020-03-10 19:29     ` Haitao Huang
2020-03-11  9:13       ` Dr. Greg
2020-03-11 17:15         ` Haitao Huang
2020-03-17  1:07       ` Dr. Greg
2020-03-03 23:36 ` [PATCH v28 15/22] x86/sgx: Add provisioning Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 16/22] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-03-05 19:03   ` Sean Christopherson
2020-03-06 18:47     ` Jarkko Sakkinen
2020-03-12 18:38       ` Sean Christopherson
2020-03-15  0:27         ` Jarkko Sakkinen
2020-03-15  1:17           ` Jarkko Sakkinen
2020-03-09 21:16   ` Sean Christopherson
2020-03-03 23:36 ` [PATCH v28 17/22] x86/sgx: ptrace() support for the SGX driver Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 18/22] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 19/22] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 20/22] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-03-03 23:36 ` [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-03-11 17:30   ` Nathaniel McCallum
2020-03-11 17:38     ` Jethro Beekman
2020-03-11 19:15       ` Nathaniel McCallum
2020-03-13 15:48       ` Nathaniel McCallum
2020-03-13 16:46         ` Sean Christopherson
2020-03-13 18:32           ` Nathaniel McCallum [this message]
2020-03-13 18:44             ` Sean Christopherson
2020-03-13 20:14               ` Nathaniel McCallum
2020-03-13 22:08                 ` Sean Christopherson
2020-03-14 14:10                   ` Nathaniel McCallum
2020-03-18 23:40                     ` Sean Christopherson
2020-03-19  0:38                       ` Xing, Cedric
2020-03-19  1:03                         ` Sean Christopherson
2020-03-20 13:55                       ` Nathaniel McCallum
2020-03-15  1:25     ` Jarkko Sakkinen
2020-03-15 17:53       ` Nathaniel McCallum
2020-03-16 13:31         ` Jethro Beekman
2020-03-16 13:57           ` Nathaniel McCallum
2020-03-16 13:59             ` Jethro Beekman
2020-03-16 14:03               ` Nathaniel McCallum
2020-03-16 17:17                 ` Sean Christopherson
2020-03-16 21:27             ` Jarkko Sakkinen
2020-03-16 21:29               ` Jarkko Sakkinen
2020-03-16 22:55           ` Sean Christopherson
2020-03-16 23:56             ` Xing, Cedric
2020-03-18 22:01               ` Jarkko Sakkinen
2020-03-18 22:18                 ` Jarkko Sakkinen
2020-03-16 13:56         ` Jarkko Sakkinen
2020-03-16 14:01           ` Nathaniel McCallum
2020-03-16 21:38             ` Jarkko Sakkinen
2020-03-16 22:53               ` Sean Christopherson
2020-03-16 23:50                 ` Xing, Cedric
2020-03-16 23:59                   ` Sean Christopherson
2020-03-17  0:18                     ` Xing, Cedric
2020-03-17  0:27                       ` Sean Christopherson
2020-03-17 16:37                         ` Nathaniel McCallum
2020-03-17 16:50                       ` Nathaniel McCallum
2020-03-17 21:40                         ` Xing, Cedric
2020-03-17 22:09                           ` Sean Christopherson
2020-03-17 22:36                             ` Xing, Cedric
2020-03-17 23:57                               ` Sean Christopherson
2020-03-17 22:23                         ` Xing, Cedric
2020-03-18 13:01                           ` Nathaniel McCallum
2020-03-20 15:53                             ` Nathaniel McCallum
2020-03-17 16:28                 ` Nathaniel McCallum
2020-03-18 22:58                   ` Jarkko Sakkinen
2020-03-18 22:39                 ` Jarkko Sakkinen
2020-03-11 19:30   ` Nathaniel McCallum
2020-03-13  0:52     ` Sean Christopherson
2020-03-13 16:07       ` Nathaniel McCallum
2020-03-13 16:33         ` Sean Christopherson
2020-03-03 23:36 ` [PATCH v28 22/22] selftests/x86: Add vDSO selftest for SGX Jarkko Sakkinen
2020-03-04 19:24 ` [PATCH v28 00/22] Intel SGX foundations Nathaniel McCallum
2020-03-17 16:00 ` Jordan Hand
2020-03-18 21:56   ` Jarkko Sakkinen
2020-03-19 17:16   ` Dr. Greg

Reply instructions:

You may reply publicly 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='CAOASepN1hxSgxVJAJiAbSmuCTCHd=95Mnvh6BKNSPJs=EpAmbQ@mail.gmail.com' \
    --to=npmccallum@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=ckuehl@redhat.com \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=harald@redhat.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=josh@joshtriplett.org \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=lsturman@redhat.com \
    --cc=luto@amacapital.net \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@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