Re: [intel-sgx-kernel-dev] [PATCH RFC v3 07/12] intel_sgx: driver for Intel Software Guard Extensions

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Tue, Nov 14, 2017 at 09:33:34PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 07, 2017 at 11:05:08AM -0800, Dave Hansen wrote:
> > On 11/07/2017 10:47 AM, Jarkko Sakkinen wrote:
> > > On Mon, Nov 06, 2017 at 07:54:00AM -0800, Dave Hansen wrote:
> > >> On 10/10/2017 07:32 AM, Jarkko Sakkinen wrote:
> > >>> +static LIST_HEAD(sgx_free_list);
> > >>> +static DEFINE_SPINLOCK(sgx_free_list_lock);
> > >>
> > >> Is this a global list?  Will this be a scalability problem on larger
> > >> systems?
> > > 
> > > It will be need to be refined for NUMA.
> > > 
> > > In addition, per-CPU caches would probably make sense.
> > > 
> > > For simplicity, I would keep it as it is up until the driver is in the
> > > mainline.
> > 
> > FWIW, I don't think we should merge things that aren't performant.
> > Global locks like this are just intolerable.  You can add this as a
> > later patch, but please don't merge stuff like this.
> 
> The back pointer to struct sgx_encl_page from struct sgx_epc_page is
> useless. I've had this in backlog already long time ago but had forgot
> it as I've been mostly working with the launch infrastructure lately.
> 
> Your comment worked as kind of a reminder of that. Thank you.
> 
> Once that field is removed the whole struct is useless and EPC bank
> converges to an array. With an array the driver should be able reserve
> pages without a global lock.
> 
> I've started writing a patch to make all this happen and it is
> progressing really well. I'm planning to include this change to v6.
> As it simplifies code I'm going to squash it as part of the initial
> driver patch.
> 
> How does this sound?

Every sgx_epc_bank will have a bitmap array for reserved EPC.

Every unswapped sgx_encl_page will have a pointer containing physical
address of the EPC page in the upper bits and bank number in the lower
bits (like sgx_epc_page has now in the 'pa' field).

This layout does not require a global lock.

/Jarkko