Re: [RFC PATCH v4 05/26] x86/sgx: Introduce virtual EPC for use by KVM guests

From: Jarkko Sakkinen <jarkko@kernel.org>
To: Kai Huang <kai.huang@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>,
	linux-sgx@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org,
	seanjc@google.com, luto@kernel.org, rick.p.edgecombe@intel.com,
	haitao.huang@intel.com, pbonzini@redhat.com, bp@alien8.de,
	tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com
Subject: Re: [RFC PATCH v4 05/26] x86/sgx: Introduce virtual EPC for use by KVM guests
Date: Fri, 12 Feb 2021 14:15:59 +0200	[thread overview]
Message-ID: <YCZxf+B2jtPmS5uk@kernel.org> (raw)
In-Reply-To: <ec9604199072e185de4b6b74209e84f30423c5e3.camel@intel.com>

On Wed, Feb 10, 2021 at 01:20:32PM +1300, Kai Huang wrote:
> On Tue, 2021-02-09 at 13:36 -0800, Dave Hansen wrote:
> > On 2/9/21 1:19 PM, Jarkko Sakkinen wrote:
> > > > Without that clearly documented, it would be unwise to merge this.
> > > E.g.
> > > 
> > > - Have ioctl() to turn opened fd as vEPC.
> > > - If FLC is disabled, you could only use the fd for creating vEPC.
> > > 
> > > Quite easy stuff to implement.
> > 
> > The most important question to me is not how close vEPC is today, but
> > how close it will be in the future.  It's basically the age old question
> > of: do we make one syscall that does two things or two syscalls?
> > 
> > Is there a _compelling_ reason to change direction?  How much code would
> > we save?
> 
> Basically we need to defer 'sgx_encl' related code from open to after mmap(). For
> instance, We need to defer 'sgx_encl' allocation from open to SGX_IOC_ENCLAVE_CREATE.
> And due to this change, we also need to move some members out of 'sgx_encl', and use
> them as common for enclave and vEPC. The 'struct xarray page_array' would be a good
> example.
> 
> The code we can save, from my first glance, is just misc_register("/dev/sgx_vepc")
> related, maybe plus some mmap() related code. The logic to handle both host enclave
> and vEPC still needs to be there.
> 
> To me the major concern is /dev/sgx_enclave, by its name, implies it is associated
> with host enclave. Adding IOCTL to *convert* it to vEPC is just mixing things up, and
> is ugly. If we really want to do this, IMHO we need at least change /dev/sgx_enclave
> to /dev/sgx_epc, for instance, to imply the fd we opened and mmap()'d just represents
> some raw EPC. However this is changing to existing userspace ABI.
> 
> Sean,
> 
> What's your opinion? Did I miss anything?

I think this mostly makes sense to me.

And also, it gives an opportunity to have add some granularity to access
control (worth of mentioning in the commit message).

Thanks.

/Jarkko