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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171114210505.wu7kf5wef5l7ujhj@linux.intel.com>
Date:   Tue, 14 Nov 2017 23:05:05 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     linux-kernel@...r.kernel.org, intel-sgx-kernel-dev@...ts.01.org,
        platform-driver-x86@...r.kernel.org
Subject: Re: [intel-sgx-kernel-dev] [PATCH RFC v3 07/12] intel_sgx: driver
 for Intel Software Guard Extensions

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