Re: [PATCH v29 00/20] Intel SGX foundations

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Fri, May 08, 2020 at 02:02:26PM -0500, Dr. Greg wrote:
> On Thu, May 07, 2020 at 02:41:30AM +0200, Thomas Gleixner wrote:
> > The diffstat of your patch is irrelevant. What's relevant is the
> > fact that it is completely unreviewed and that it creates yet
> > another user space visible ABI with a noticable lack of
> > documentation.

...

> As to lack of review, we would certainly welcome technical and API
> comments but we cannot force them.

Thomas' point isn't that your code needs to be reviewed, it's that trying
to squeeze it into the initial merge will, not might, _will_ push out the
acceptance of SGX.  The same is true for other features we have lined up,
e.g. KVM and cgroup support.  It's not a slight on your code, it's just
reality at this point.

> In fact, anyone who reviews the patch will see that the current driver
> creates a pointer in the ioctl call to pass downward into the hardware
> initialization routines.  Our code simply replaces that pointer with
> one supplied by userspace.

Personally, I'm in favor of adding a reserved placeholder for a token so
as to avoid adding a second ioctl() in the event that something gets
upstreamed that wants the token.  Ditto for a file descriptor for the
backing store in sgx_enclave_create.

But, I have contributed exactly zero ioctls to the kernel, so take that
with a big block of salt :-)

> At the very least a modular form of the driver should be considered
> that would allow alternate implementations.  Sean indicated that there
> was a 'kludgy' approach that would allow an alternate modular
> implementation alongside the in-kernel driver.  I believe that Andy
> has already indicated that may not be an advisable approach.

Modularizing the the driver does nothing for your use case.  The "driver"
is a relatively lightweight wrapper, removing that is akin to making
/dev/sgx/enclave inaccessible, i.e. it doesn't make the EPC tracking and
core code go away.  Modularizing the whole thing is flat out not an option,
as doing so is not compatible with an EPC cgroup and adds unnecessary
complexity to KVM enabling, both of which are highly desired features by
pretty much everyone that plans on using SGX.

Andy is right to caution against playing games to squish in-kernel things,
but the virtualization snafu is a completely different beast.  E.g. SGX
doesn't require fiddling with CR4, won't corrupt random memory across
kexec(), doesn't block INIT, etc...  And I'm not advocating long-term
shenanigans, I just wanted to point out that there are options for running
out-of-tree drivers in the short term, e.g. until proper policy controls
land upstream.