Re: [PATCH v19 00/27] Intel SGX1 support

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jethro Beekman <jethro@fortanix.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"dave.hansen@intel.com" <dave.hansen@intel.com>,
	"nhorman@redhat.com" <nhorman@redhat.com>,
	"npmccallum@redhat.com" <npmccallum@redhat.com>,
	"serge.ayoun@intel.com" <serge.ayoun@intel.com>,
	"shay.katz-zamir@intel.com" <shay.katz-zamir@intel.com>,
	"haitao.huang@intel.com" <haitao.huang@intel.com>,
	"andriy.shevchenko@linux.intel.com" 
	<andriy.shevchenko@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"kai.svahn@intel.com" <kai.svahn@intel.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"josh@joshtriplett.org" <josh@joshtriplett.org>,
	"luto@kernel.org" <luto@kernel.org>,
	"kai.huang@intel.com" <kai.huang@intel.com>,
	"rientjes@google.com" <rientjes@google.com>
Subject: Re: [PATCH v19 00/27] Intel SGX1 support
Date: Tue, 19 Mar 2019 17:22:46 -0700	[thread overview]
Message-ID: <20190320002246.GO25575@linux.intel.com> (raw)
In-Reply-To: <d04bd50e-4fc1-70ee-780d-2c2acd939bdb@fortanix.com>

On Tue, Mar 19, 2019 at 11:52:32PM +0000, Jethro Beekman wrote:
> On 2019-03-19 16:41, Sean Christopherson wrote:
> >On Sun, Mar 17, 2019 at 11:14:29PM +0200, Jarkko Sakkinen wrote:
> >>* Allow the driver to be compiled as a module now that it no code is using
> >>   its routines and it only uses exported symbols. Now the driver is
> >>   essentially just a thin ioctl layer.
> >
> >I'm not convinced it's worth the effort to allow the driver to be compiled
> >as a module, especially if we drop the ACPI-based probing.  Making the
> >driver loadable means the kernel can easily end up in situations where it's
> >tracking EPC and running its reclaimer kthread, but the driver can't be
> >loaded and can *never* be loaded, e.g. because the platform doesn't support
> >Launch Control.
> 
> Tracking EPC etc. is necessary for KVM anyway.

This part isn't related to KVM.  As it's written, a kernel with SGX support
running on a non-LC system will allocate memory and spin up a kthread, and
then do absolutely nothing with it.

When KVM support is added, then yes, it's a slightly different story.  But
we end up in the same spot if the kernel isn't built with EPC virtualization
support.

> 
> >And IMO encl.{c,h} belongs in the "driver" code, but to let the driver be
> >loadable it got shoved into the core subsystem.  All of that code is
> >specific to running enclaves in the host, i.e. it shouldn't exist if I
> >compile out the driver entirely (in a future world where I want the core
> >SGX subsystem for virtualization purposes).
> 
> Your argument here is "something that belongs in the driver isn't, therefore
> we shouldn't have a loadable driver". This seems backwards to me. Instead,
> we should see what interface would be needed so that this stuff *can* be in
> the driver.

Speaking of rehashing arguments, that approach got nixed in a previous
revision because it requires implementing the EPC eviction flows via
callbacks.  Specifically, Dave Hansen argued that we shouldn't be adding
infrastructure (the callback framework, layer of indirection, etc...)
without any true need or user for it.

> >And yes, I realize this is a full 180 from my stance a year ago :)
> 
> I don't really want to rehash this argument but I think it should be a
> module.

I agree that ideally the userspace facing driver would be a module, but
practically speaking making the driver a module complicates the kernel
a bit and leads to some undesirable behavior.  A loadable module is
"nice", but I haven't seen a true use case that requires the driver to
be a module.