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@xxxxxxxxx>
> >
> > 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?
