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

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

[Jarkko Sakkinen]

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?

/Jarkko

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

[Jarkko Sakkinen]

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

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

[Dave Hansen]

On 11/14/2017 01:05 PM, Jarkko Sakkinen wrote:
> 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?

Sounds sane to me.