Re: [PATCH v13 09/13] x86/sgx: Enclave Page Cache (EPC) memory manager

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

On Tue, Aug 28, 2018 at 02:26:36PM -0700, Dave Hansen wrote:
> On 08/28/2018 02:22 PM, Sean Christopherson wrote:
> > On Tue, Aug 28, 2018 at 07:07:33AM -0700, Dave Hansen wrote:
> >> On 08/28/2018 01:35 AM, Jarkko Sakkinen wrote:
> >>> On Mon, Aug 27, 2018 at 02:15:34PM -0700, Dave Hansen wrote:
> >>>> On 08/27/2018 11:53 AM, Jarkko Sakkinen wrote:
> >>>>> +struct sgx_epc_page_ops {
> >>>>> +	bool (*get)(struct sgx_epc_page *epc_page);
> >>>>> +	void (*put)(struct sgx_epc_page *epc_page);
> >>>>> +	bool (*reclaim)(struct sgx_epc_page *epc_page);
> >>>>> +	void (*block)(struct sgx_epc_page *epc_page);
> >>>>> +	void (*write)(struct sgx_epc_page *epc_page);
> >>>>> +};
> >>>> Why do we need a fancy, slow (retpoline'd) set of function pointers when
> >>>> we only have one user of these (the SGX driver)?
> >>> KVM has its own implementation for these operations.
> >>
> >> That belongs in the changelog.
> >>
> >> Also, where is the implementation?  How can we assess this code that was
> >> built to create an abstraction without both of the users?
> > 
> > I can provide an early preview of the KVM reclaim code, but honestly
> > I think that would do more harm than good.  The VMX architecture for
> > EPC reclaim is complex, even for SGX standards.  Opening that can of
> > worms would likely derail this discussion.  That being said, this
> > abstraction isn't exactly what KVM will need, but it's pretty close
> > and gives us something to build on.
> 
> Please remove the abstraction code.  We don't introduce infrastructure
> which no one will use.

The infrastructure is used in the sense that it allows us to split the
userspace-facing code, i.e. the driver, into a separate module.  This
in turn allows virtualization of SGX without having to load the driver
or building it in the first place, e.g. to virtualize SGX on a system
that doesn't meet the driver's requirements.

We could eliminate the abstraction by moving the EPC management code
into the driver, but that would directly conflict with past feedback
and would need to be completely undone to enable KVM.  The abstraction
could be dumbed down to a single function, but as mentioned earlier,
that comes with its own costs.  I can dive into exactly what we lose
with a single function approach if this is a sticking point.