Re: [RFC PATCH 03/23] x86/sgx: Introduce virtual EPC for use by KVM guests

[Kai Huang <kai.huang@intel.com>]

On Wed, 6 Jan 2021 11:35:41 -0800 Dave Hansen wrote:
> On 1/5/21 5:55 PM, Kai Huang wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Add a misc device /dev/sgx_virt_epc to allow userspace to allocate "raw"
> > EPC without an associated enclave.  The intended and only known use case
> > for raw EPC allocation is to expose EPC to a KVM guest, hence the
> > virt_epc moniker, virt.{c,h} files and X86_SGX_VIRTUALIZATION Kconfig.
> > 
> > Modify sgx_init() to always try to initialize virtual EPC driver, even
> > when SGX driver is disabled due to SGX Launch Control is in locked mode,
> > or not present at all, since SGX virtualization allows to expose SGX to
> > guests that support non-LC configurations.
> 
> The grammar here is a bit off.  Here's a rewrite:
> 
> Modify sgx_init() to always try to initialize the virtual EPC driver,
> even if the bare-metal SGX driver is disabled.  The bare-metal driver
> might be disabled if SGX Launch Control is in locked mode, or not
> supported in the hardware at all.  This allows (non-Linux) guests that
> support non-LC configurations to use SGX.

Thanks. I'll use yours, except I want to change "bare-metal driver might be
disabled.." to "bare-metal driver will be disabled..".

I'll also use all your comments mentioned in your reply to this patch.

[...]

> > +
> > +static int sgx_virt_epc_release(struct inode *inode, struct file *file)
> > +{
> > +	struct sgx_virt_epc *epc = file->private_data;
> 
> FWIW, I hate the "struct sgx_virt_epc *epc" name.  "epc" here is really
> an instance
> 

How about "struct sgx_virt_epc *vepc" ?

[...]

> > +static int sgx_virt_epc_open(struct inode *inode, struct file *file)
> > +{
> > +	struct sgx_virt_epc *epc;
> > +
> > +	epc = kzalloc(sizeof(struct sgx_virt_epc), GFP_KERNEL);
> > +	if (!epc)
> > +		return -ENOMEM;
> > +	/*
> > +	 * Keep the current->mm to virtual EPC. It will be checked in
> > +	 * sgx_virt_epc_mmap() to prevent, in case of fork, child being
> > +	 * able to mmap() to the same virtual EPC pages.
> > +	 */
> > +	mmgrab(current->mm);
> > +	epc->mm = current->mm;
> > +	mutex_init(&epc->lock);
> > +	xa_init(&epc->page_array);
> > +
> > +	file->private_data = epc;
> > +
> > +	return 0;
> > +}
> 
> I understand why this made sense for regular enclaves, but I'm having a
> harder time here.  If you mmap(fd, MAP_SHARED), fork(), and then pass
> that mapping through to two different guests, you get to hold the
> pieces, just like if you did the same with normal memory.
> 
> Why does the kernel need to enforce this policy?

Does Sean's reply in another email satisfy you?