Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: x86@kernel.org, linux-sgx@vger.kernel.org,
	akpm@linux-foundation.org, dave.hansen@intel.com,
	nhorman@redhat.com, npmccallum@redhat.com, serge.ayoun@intel.com,
	shay.katz-zamir@intel.com, haitao.huang@intel.com,
	andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
	kai.svahn@intel.com, bp@alien8.de, josh@joshtriplett.org,
	luto@kernel.org, kai.huang@intel.com, rientjes@google.com
Subject: Re: [PATCH v19 13/27] x86/sgx: Add wrappers for ENCLS leaf functions
Date: Fri, 22 Mar 2019 09:43:48 -0700
Message-ID: <20190322164348.GB12666@linux.intel.com> (raw)
In-Reply-To: <20190322110014.GF3122@linux.intel.com>

On Fri, Mar 22, 2019 at 01:00:14PM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 21, 2019 at 08:40:11AM -0700, Sean Christopherson wrote:
> > On Thu, Mar 21, 2019 at 04:51:53PM +0200, Jarkko Sakkinen wrote:
> > > On Tue, Mar 19, 2019 at 12:59:07PM -0700, Sean Christopherson wrote:
> > > > On Sun, Mar 17, 2019 at 11:14:42PM +0200, Jarkko Sakkinen wrote:
> > > > > +/**
> > > > > + * ENCLS_FAULT_FLAG - flag signifying an ENCLS return code is a trapnr
> > > > > + *
> > > > > + * ENCLS has its own (positive value) error codes and also generates
> > > > > + * ENCLS specific #GP and #PF faults.  And the ENCLS values get munged
> > > > > + * with system error codes as everything percolates back up the stack.
> > > > > + * Unfortunately (for us), we need to precisely identify each unique
> > > > > + * error code, e.g. the action taken if EWB fails varies based on the
> > > > > + * type of fault and on the exact SGX error code, i.e. we can't simply
> > > > > + * convert all faults to -EFAULT.
> > > > > + *
> > > > > + * To make all three error types coexist, we set bit 30 to identify an
> > > > > + * ENCLS fault.  Bit 31 (technically bits N:31) is used to differentiate
> > > > > + * between positive (faults and SGX error codes) and negative (system
> > > > > + * error codes) values.
> > > > > + */
> > > > > +#define ENCLS_FAULT_FLAG 0x40000000
> > > > > +
> > > > > +/**
> > > > > + * Retrieve the encoded trapnr from the specified return code.
> > > > > + */
> > > > > +#define ENCLS_TRAPNR(r) ((r) & ~ENCLS_FAULT_FLAG)
> > > > 
> > > > I honestly can't remember, is there a reason ENCLS_TRAPNR is still a macro
> > > > and not an inline function?
> > > 
> > > Not at all. And also the value should be unsigned and fault flag
> > > should be 0x80* because we never should get negative values.
> > 
> > From ENCLS itself, but we do have scenarios where the return code can
> > hold a system error code (negative value), an SGX error code (positive
> > value) or fault inforation.  E.g. sgx_encl_init() sets ret to -ERESTARTSYS,
> > and __sgx_encl_ewb() returns system error codes if get_backing() fails
> > and also directly returns the result of __ewb().  We could dance around
> > those issues, but IMO it's too fragile as it essentially requires a full
> > audit of every possible error path when modifying code.
> 
> There should be zero of those now but if you find one we can fix that.
> These inline functions should only deal with what comes from ENCLS. I
> think it is cleanest to fix those sites. I think here the focus should
> be on clarity.

To me, using bit 30 to denote a fault provides the most clarity.  Values
in the form of 0x4000xxxx aren't very common, i.e. regardless of where I
see such a pattern in a register, message, etc... I know there's a good
chance I'm dealing with an ENCLS fault.

> In ioctl.c there are total 5 sites and all of them are good.
> 
> Also encl.c is all good.
> 
> From the sites that you mentioned sgx_encl_ewb() needs to be fixed. In
> reclaim.c that seems to be the only problematic site.

This is the crux of my argument, e.g. "seems to be" doesn't exactly instill
confidence.  I just don't see what we gain by switching to bit 31, and on
the flip side we're forced to constantly audit the code because nearly all
of the affected paths are only exercised when something goes haywire.

> 
> These are pretty easy to grep with 'encls_'.
> 
> For me it looks like that assuming the positive value is absolutely
> the right thing to do as long as sgx_encl_ewb() is fixed properly.

  reply index

Thread overview: 92+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-17 21:14 [PATCH v19 00/27] Intel SGX1 support Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 01/27] x86/cpufeatures: Add Intel-defined SGX feature bit Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 02/27] x86/cpufeatures: Add SGX sub-features (as Linux-defined bits) Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 03/27] x86/msr: Add IA32_FEATURE_CONTROL.SGX_ENABLE definition Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 04/27] x86/cpufeatures: Add Intel-defined SGX_LC feature bit Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 05/27] x86/msr: Add SGX Launch Control MSR definitions Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 06/27] x86/mm: x86/sgx: Add new 'PF_SGX' page fault error code bit Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 07/27] x86/mm: x86/sgx: Signal SIGSEGV for userspace #PFs w/ PF_SGX Jarkko Sakkinen
2019-03-18 17:15   ` Dave Hansen
2019-03-18 19:53     ` Sean Christopherson
2019-03-17 21:14 ` [PATCH v19 08/27] x86/cpu/intel: Detect SGX support and update caps appropriately Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 09/27] x86/sgx: Add ENCLS architectural error codes Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 10/27] x86/sgx: Add SGX1 and SGX2 architectural data structures Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 11/27] x86/sgx: Add definitions for SGX's CPUID leaf and variable sub-leafs Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 12/27] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2019-03-18 19:50   ` Sean Christopherson
2019-03-21 14:40     ` Jarkko Sakkinen
2019-03-21 15:28       ` Sean Christopherson
2019-03-22 10:19         ` Jarkko Sakkinen
2019-03-22 10:50           ` Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 13/27] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2019-03-19 19:59   ` Sean Christopherson
2019-03-21 14:51     ` Jarkko Sakkinen
2019-03-21 15:40       ` Sean Christopherson
2019-03-22 11:00         ` Jarkko Sakkinen
2019-03-22 16:43           ` Sean Christopherson [this message]
2019-03-17 21:14 ` [PATCH v19 16/27] x86/sgx: Add the Linux SGX Enclave Driver Jarkko Sakkinen
2019-03-19 21:19   ` Sean Christopherson
2019-03-21 15:51     ` Jarkko Sakkinen
2019-03-21 16:47       ` Sean Christopherson
2019-03-22 11:10         ` Jarkko Sakkinen
2019-03-26 13:26       ` Jarkko Sakkinen
2019-03-26 23:58         ` Sean Christopherson
2019-03-27  5:28           ` Jarkko Sakkinen
2019-03-27 17:57             ` Sean Christopherson
2019-03-27 18:38             ` Jethro Beekman
2019-03-27 20:06               ` Sean Christopherson
2019-03-28  1:21                 ` Jethro Beekman
2019-03-28 13:19                 ` Jarkko Sakkinen
2019-03-28 19:05                   ` Andy Lutomirski
2019-03-29  9:43                     ` Jarkko Sakkinen
2019-03-29 16:20                     ` Sean Christopherson
2019-04-01 10:01                       ` Jarkko Sakkinen
2019-04-01 17:25                         ` Jethro Beekman
2019-04-01 22:57                           ` Jarkko Sakkinen
2019-03-28 13:15               ` Jarkko Sakkinen
2019-03-19 23:00   ` Sean Christopherson
2019-03-21 16:18     ` Jarkko Sakkinen
2019-03-21 17:38       ` Sean Christopherson
2019-03-22 11:17         ` Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 17/27] x86/sgx: Add provisioning Jarkko Sakkinen
2019-03-19 20:09   ` Sean Christopherson
2019-03-21  2:08     ` Huang, Kai
2019-03-21 14:32       ` Jarkko Sakkinen
2019-03-21 21:41         ` Huang, Kai
2019-03-22 11:31           ` Jarkko Sakkinen
2019-03-21 14:30     ` Jarkko Sakkinen
2019-03-21 14:38   ` Nathaniel McCallum
2019-03-22 11:22     ` Jarkko Sakkinen
2019-03-21 16:50   ` Andy Lutomirski
2019-03-22 11:29     ` Jarkko Sakkinen
2019-03-22 11:43       ` Jarkko Sakkinen
2019-03-22 18:20         ` Andy Lutomirski
2019-03-25 14:55           ` Jarkko Sakkinen
2019-03-27  0:14             ` Sean Christopherson
2019-04-05 10:18             ` Jarkko Sakkinen
2019-04-05 13:53               ` Andy Lutomirski
2019-04-05 14:20                 ` Jarkko Sakkinen
2019-04-05 14:34                   ` Greg KH
2019-04-09 13:37                     ` Jarkko Sakkinen
2019-04-05 14:21                 ` Greg KH
2019-03-17 21:14 ` [PATCH v19 19/27] x86/sgx: ptrace() support for the SGX driver Jarkko Sakkinen
2019-03-19 22:22   ` Sean Christopherson
2019-03-21 15:02     ` Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 20/27] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 21/27] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 22/27] x86/fault: Attempt to fixup unhandled #PF in vDSO before signaling Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 23/27] x86/traps: Attempt to fixup exceptions " Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 25/27] x86/sgx: SGX documentation Jarkko Sakkinen
2019-03-20 17:14   ` Sean Christopherson
2019-03-21 16:24     ` Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 26/27] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2019-03-17 21:14 ` [PATCH v19 27/27] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2019-03-19 17:12   ` Sean Christopherson
2019-03-21 14:42     ` Jarkko Sakkinen
     [not found] ` <20190317211456.13927-19-jarkko.sakkinen@linux.intel.com>
2019-03-19 22:09   ` [PATCH v19 18/27] x86/sgx: Add swapping code to the core and SGX driver Sean Christopherson
2019-03-21 14:59     ` Jarkko Sakkinen
2019-03-19 23:41 ` [PATCH v19 00/27] Intel SGX1 support Sean Christopherson
2019-03-19 23:52   ` Jethro Beekman
2019-03-20  0:22     ` Sean Christopherson
2019-03-21 16:20     ` Jarkko Sakkinen
2019-03-21 16:00   ` 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=20190322164348.GB12666@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=josh@joshtriplett.org \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=rientjes@google.com \
    --cc=serge.ayoun@intel.com \
    --cc=shay.katz-zamir@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 linux-sgx@archiver.kernel.org
	public-inbox-index linux-sgx


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