On Thu, Jun 25, 2020 at 11:34:48AM -0700, Sean Christopherson wrote:

On Thu, Jun 25, 2020 at 07:23:19PM +0200, Borislav Petkov wrote:

quoted

Also, you had all patches until now split nice and logically doing one
thing only.

But this one is huge. Why?

Why can't you split out the facilities which the driver uses: encl.[ch]
into a patch, then ioctl.c into a separate one and then the driver into
a third one? Or do they all belong together inseparably?

I guess I'll find out eventually but it would've been nice if they were
split out...

Hmm, I think the most reasonable way to break up this beast would be to
incrementally introduce functionality.  E.g. four or so patches, one for
each ioctl() of ENCLAVE_CREATE, ENCLAVE_ADD_PAGES, ENCLAVE_INIT and
ENCLAVE_SET_ATTRIBUTE, in that order.

Splitting up by file probably wouldn't work very well.  The split is
pretty arbitrary, e.g. encl.[ch] isn't simply a pure representation of an
enclave, there is a lot of the driver details/dependencies in there, i.e.
the functionality between encl/ioctl/driver is all pretty intertwined.

But I think serially introducing each ioctl() would be fairly clean, and
would help readers/reviewers better understand SGX as the patches would
naturally document the process of building an enclave, e.g. CREATE the
enclave, then ADD_PAGES, then INIT the enclave.  SET_ATTRIBUTE is a bit
of an outlier in that it would be chronologically out of order with
respect to building the enclave, but I think that's ok. 

Jarkko, thoughts?

I proposed the same before I go this email so I guess we have a
consensus here.

/Jarkko