Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-sgx@vger.kernel.org,
	Nathaniel McCallum <npmccallum@redhat.com>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Cedric Xing <cedric.xing@intel.com>
Subject: Re: [PATCH] x86/vdso: Flatten and clean up struct sgx_enclave_run
Date: Thu, 1 Oct 2020 15:50:29 -0700	[thread overview]
Message-ID: <20201001225029.GJ7474@linux.intel.com> (raw)
In-Reply-To: <20201001204027.GA18610@linux.intel.com>

On Fri, Oct 02, 2020 at 12:46:11AM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 01, 2020 at 09:51:51AM -0700, Sean Christopherson wrote:
> > > Remove 'exit_reason' as it is really a redundant field to save some space.
> > > Instead, have just a field called 'leaf' that tells the last seen ENCLU.
> > 
> > This justification contradicts the removal of flags.  If you want to shave
> > bytes, keep flags and enforce that flags is zero so that you don't need to
> > have reserved bytes at the end of the struct.
> > 
> > IMO, having a separate exit reason provides saner code for userspace, e.g. 
> > 
> >   if (run->exit_reason == SGX_EXCEPTION_EXIT)
> > 	<do exception stuff>
> > 
> > versus
> > 
> >   if (run->leaf == EENTER || run->leaf == ERESUME)
> >         <do exception stuff>
> > 
> > I would prefer to be explicit about why the vDSO is transferring control to
> > the caller or its handler.  It also gives us line of sight to supporting
> > exit types other than EEXIT and exceptions.  Maybe we never end up with
> > another type, but IMO shaving 4 bytes and a MOV isn't worth the risk of
> > ending up with a mess of an API if another exit reason comes along.
> 
> 'exit_reason' is just waste of expensive memory space that we have
> limited fixed amount of.

Huh?  4 bytes on the user stack can't possibly be a real concern, the default
stack size on x86-64 is 8MB.  Golang shaves that down to 2kb but dynamically
adjusts the stack as needed.  The "allocation" is completely free, the caller
will already be adjusting RSP to create space for the struct, and it doesn't
need to be initialized.

> Using the same field in a *guaranteed*
> non-conflicting manner saves space. There is absolutely no question
> that what I did should not be done.

But you can't guarantee that it doesn't conflict with future changes to the
SGX architecture.  Hell, you can't even guarantee that it doesn't conflict
with the current SGX architecture, e.g. see Jethro's request to invoke the
handler on AEX's due to interrupts.