From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> To: Sean Christopherson <sean.j.christopherson@intel.com> Cc: Jethro Beekman <jethro@fortanix.com>, x86@kernel.org, linux-sgx@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Lutomirski <luto@amacapital.net>, Cedric Xing <cedric.xing@intel.com>, akpm@linux-foundation.org, andriy.shevchenko@linux.intel.com, asapek@google.com, bp@alien8.de, chenalexchen@google.com, conradparker@google.com, cyhanish@google.com, dave.hansen@intel.com, haitao.huang@intel.com, kai.huang@intel.com, kai.svahn@intel.com, kmoy@google.com, ludloff@google.com, luto@kernel.org, nhorman@redhat.com, npmccallum@redhat.com, puiterwijk@redhat.com, rientjes@google.com, tglx@linutronix.de, yaozhangx@google.com, mikko.ylinen@intel.com Subject: Re: [PATCH v39 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Date: Wed, 7 Oct 2020 10:39:23 +0300 [thread overview] Message-ID: <20201007073923.GA3632@linux.intel.com> (raw) In-Reply-To: <20201007043418.GG28981@linux.intel.com> On Tue, Oct 06, 2020 at 09:34:19PM -0700, Sean Christopherson wrote: > On Wed, Oct 07, 2020 at 06:14:02AM +0300, Jarkko Sakkinen wrote: > > On Tue, Oct 06, 2020 at 06:17:38PM -0700, Sean Christopherson wrote: > > > On Wed, Oct 07, 2020 at 03:22:36AM +0300, Jarkko Sakkinen wrote: > > > > > And then a third flavor comes along, e.g. Jethro's request interrupt case, > > > > > and exit_reason can also return '2'. How do you handle that with only the > > > > > leaf? > > > > > > > > I'm listening. How was that handled before? I saw only '0' and '1'. Can > > > > you bring some context on that? I did read the emails that were swapped > > > > when the run structure was added but I'm not sure what is the exact > > > > differentiator. Maybe I'm missing something. > > > > > > https://patchwork.kernel.org/patch/11719889/ > > > > Thank you. > > > > There's aboslutely nothing that is blocking adding such support for such > > AEP handling in the current implementation. SGX_SYNCHRONOUS_EXIT is just > > another name for EEXIT. > > Sure. And SGX_EXCEPTION_EXIT is just another name for EENTER|ERESUME. Kind of yes. > > Even if that was in place, you'd need to separate normal and interrupt. > > Tristate is useless here. > > Huh? You mean like adding SGX_INTERRUPT_EXIT and SGX_EXCEPTION_EXIT? OK, so I'll throw something. 1. "normal" is either exception from either EENTER or ERESUME, or just EEXIT. 2. "interrupt" is something where you want to tailor AEP path. > > As far as I'm concerned, no bottlenecks have been created. > > There's no bottleneck, just an inflexible and kludgy API for userspace. > > if (run->leaf == EEXIT) > return handle_eexit(); > > if (run->leaf == EENTER || run->leaf == ERESUME) > return handle_exception(run->leaf); > > return -EIO; I think that's quite intuitive to have just one state variable. > Let's say we come up with a clever opt-in scheme that allows exception fixup > to inform the vDSO that the enclave was invalid, even on SGX1. Now we're in > a scenario where we want to tell userspace that the enclave is lost, but > userspace assumes any exit EENTER or ERESUME is an exception. > > if (run->leaf == EEXIT) > return handle_eexit(); > > if (run->leaf == EENTER || run->leaf == ERESUME) > return handle_invalid_enclave_or_maybe_exception(); > > return -EIO; What I'd do would be to add a 'flags' field. It could have a bit for interrupt, let's call it for the sake of an example as SGX_ENCLAVE_RUN_FLAG_INT. Then you'd do this if you want to exit from the vDSO instead of doing ERESUME: run->flags |= SGX_ENCLAVE_RUN_FLAG_INT The vDSO would check this bit on AEP and: 1. If it's cleared, it would ERESUME. 2. If it's set, it would clear it and exit from vDSO. > We could add a new exit reason, but we'd still need to ensure EENTER|ERESUME > means "exception" for old userspace. Or we could add exit_reason now and end > up with (IMO) a sane and extensible interface. > > if (run->exit_reason == SGX_ENCLAVE_INVALID) > return handle_invalid_enclave(); > > if (run->exit_reason == SGX_SYNCHRONOUS_EXIT) > return handle_eexit(); > > if (run->exit_reason == SGX_EXCEPTION) > return handle_exception(); > > return -EIO; > > And maybe we get really clever and figure out a way to (deterministically) > redirect SIGALRM to the vDSO. Then we'd want: > > if (run->exit_reason == SGX_ENCLAVE_INVALID) > return handle_invalid_enclave(); > > if (run->exit_reason == SGX_SYNCHRONOUS_EXIT) > return handle_eexit(); > > if (run->exit_reason == SGX_ALARM) > return handle_reschedule(); > > if (run->exit_reason == SGX_EXCEPTION) > return handle_exception(); > > return -EIO; > > Even more hypothetical would be if Andy gets one of his wishes, and EENTER2 > comes along that doesn't allow the enclave to dictate the exit point, > "returns" an error code on enclave failure, and allows the kernel to > auto-restart the enclave on IRQs/NMIs. That (very hypothetical) scenario > fits nicely into the exit_reason handling. > > I'm not arguing that any of the above is even remotely likely. I just don't > understand why we'd want an API that at best requires heuristics in userspace > to determine why the enclave stopped running, and at worst will saddle us with > an ugly mess in the future. All to save 4 bytes that no one cares about (they > literally cost nothing), and a single MOV in a flow that is hundreds, if not > thousands, of cycles. I don't care as much as saving bytes as defining API, which has zero ambiguous state variables. And since the field 'leaf' is there, and was before too, no degrees of freedom are lost. Removing one variable does not make more of a mess. /Jarkko
next prev parent reply other threads:[~2020-10-07 7:39 UTC|newest] Thread overview: 117+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-03 4:50 [PATCH v39 00/24] Intel SGX foundations Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 01/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen 2020-10-19 14:10 ` Dave Hansen 2020-10-19 17:49 ` Sean Christopherson 2020-10-03 4:50 ` [PATCH v39 02/24] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 03/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 04/24] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 05/24] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen 2020-10-19 14:30 ` Dave Hansen 2020-10-19 17:38 ` Sean Christopherson 2020-10-19 17:48 ` Dave Hansen 2020-10-19 17:53 ` Sean Christopherson 2020-10-19 17:58 ` Dave Hansen 2020-10-03 4:50 ` [PATCH v39 06/24] x86/cpu/intel: Detect SGX support Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 07/24] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 08/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen 2020-10-19 8:45 ` Jarkko Sakkinen 2020-10-19 12:39 ` Borislav Petkov 2020-10-23 9:01 ` Jarkko Sakkinen 2020-10-19 13:40 ` Dave Hansen 2020-10-23 9:03 ` Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 09/24] x86/sgx: Add __sgx_alloc_epc_page() and sgx_free_epc_page() Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 11/24] x86/sgx: Add SGX enclave driver Jarkko Sakkinen 2020-10-03 14:39 ` Greg KH 2020-10-04 14:32 ` Jarkko Sakkinen 2020-10-04 15:01 ` Jarkko Sakkinen 2020-10-05 9:42 ` Greg KH 2020-10-05 12:42 ` Jarkko Sakkinen 2020-10-07 18:09 ` Haitao Huang 2020-10-07 19:26 ` Greg KH 2020-10-09 6:44 ` Jarkko Sakkinen 2020-10-14 20:16 ` Dave Hansen 2020-10-05 8:45 ` Christoph Hellwig 2020-10-05 11:42 ` Jarkko Sakkinen 2020-10-05 11:50 ` Greg KH 2020-10-05 14:23 ` Jarkko Sakkinen 2020-10-05 15:02 ` Greg KH 2020-10-05 16:40 ` Dave Hansen 2020-10-05 20:02 ` Jarkko Sakkinen 2020-10-09 7:10 ` Pavel Machek 2020-10-09 7:21 ` Greg KH 2020-10-09 8:21 ` Pavel Machek 2020-10-03 19:54 ` Matthew Wilcox 2020-10-04 21:50 ` Jarkko Sakkinen 2020-10-04 22:02 ` Jarkko Sakkinen 2020-10-04 22:27 ` Matthew Wilcox 2020-10-04 23:41 ` Jarkko Sakkinen 2020-10-05 1:30 ` Matthew Wilcox 2020-10-05 3:06 ` Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Jarkko Sakkinen 2020-10-16 17:07 ` Dave Hansen 2020-10-18 4:26 ` Jarkko Sakkinen 2020-10-19 20:21 ` Dave Hansen 2020-10-19 20:48 ` Sean Christopherson 2020-10-03 4:50 ` [PATCH v39 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen 2020-10-16 21:25 ` Dave Hansen 2020-10-18 5:03 ` Jarkko Sakkinen 2020-10-19 7:03 ` Jarkko Sakkinen 2020-10-19 20:48 ` Dave Hansen 2020-10-19 21:15 ` Sean Christopherson 2020-10-19 21:44 ` Dave Hansen 2020-10-23 10:11 ` Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION Jarkko Sakkinen 2020-10-20 15:48 ` Dave Hansen 2020-10-23 10:14 ` Jarkko Sakkinen 2020-10-20 21:19 ` Dave Hansen 2020-10-23 10:17 ` Jarkko Sakkinen 2020-10-23 14:19 ` Dave Hansen 2020-10-24 11:34 ` Jarkko Sakkinen 2020-10-24 15:47 ` Andy Lutomirski 2020-10-24 20:23 ` Jarkko Sakkinen 2020-10-27 10:38 ` Dr. Greg 2020-10-23 14:23 ` Jethro Beekman 2020-10-24 11:40 ` Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 16/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen 2020-10-03 5:22 ` Haitao Huang 2020-10-03 13:32 ` Jarkko Sakkinen 2020-10-03 18:23 ` Haitao Huang 2020-10-04 22:39 ` Jarkko Sakkinen 2020-10-07 17:25 ` Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 17/24] x86/sgx: Add ptrace() support for the SGX driver Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 18/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 19/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 20/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen 2020-10-06 2:57 ` Sean Christopherson 2020-10-06 8:30 ` Jethro Beekman 2020-10-06 15:15 ` Sean Christopherson 2020-10-06 17:28 ` Jarkko Sakkinen 2020-10-06 23:21 ` Sean Christopherson 2020-10-07 0:22 ` Jarkko Sakkinen 2020-10-07 1:17 ` Sean Christopherson 2020-10-07 3:14 ` Jarkko Sakkinen 2020-10-07 4:34 ` Sean Christopherson 2020-10-07 7:39 ` Jarkko Sakkinen [this message] 2020-10-07 8:04 ` Jarkko Sakkinen 2020-10-07 15:25 ` Sean Christopherson 2020-10-07 17:08 ` Jarkko Sakkinen 2020-10-07 17:13 ` Jarkko Sakkinen 2020-10-06 15:49 ` Jarkko Sakkinen 2020-10-06 15:36 ` Jarkko Sakkinen 2020-10-06 21:39 ` Jarkko Sakkinen 2020-10-07 0:23 ` Jarkko Sakkinen 2020-10-17 1:48 ` Andy Lutomirski 2020-10-17 21:02 ` Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 22/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen 2020-10-12 16:50 ` Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 23/24] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen 2020-10-03 4:50 ` [PATCH v39 24/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen 2020-10-16 21:04 ` Dave Hansen 2020-10-18 4:27 ` Jarkko Sakkinen 2020-10-03 14:32 ` [PATCH v39 00/24] Intel SGX foundations Greg KH 2020-10-03 14:53 ` Jarkko Sakkinen 2020-10-15 19:06 ` Dave Hansen 2020-10-17 20:43 ` Jarkko Sakkinen
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=20201007073923.GA3632@linux.intel.com \ --to=jarkko.sakkinen@linux.intel.com \ --cc=akpm@linux-foundation.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=asapek@google.com \ --cc=bp@alien8.de \ --cc=cedric.xing@intel.com \ --cc=chenalexchen@google.com \ --cc=conradparker@google.com \ --cc=cyhanish@google.com \ --cc=dave.hansen@intel.com \ --cc=haitao.huang@intel.com \ --cc=jethro@fortanix.com \ --cc=kai.huang@intel.com \ --cc=kai.svahn@intel.com \ --cc=kmoy@google.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sgx@vger.kernel.org \ --cc=ludloff@google.com \ --cc=luto@amacapital.net \ --cc=luto@kernel.org \ --cc=mikko.ylinen@intel.com \ --cc=nhorman@redhat.com \ --cc=npmccallum@redhat.com \ --cc=puiterwijk@redhat.com \ --cc=rientjes@google.com \ --cc=sean.j.christopherson@intel.com \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ --cc=yaozhangx@google.com \ --subject='Re: [PATCH v39 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call' \ /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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).