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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Feb 10, 2021 at 08:52:25AM -0800, Sean Christopherson wrote:
> On Wed, Feb 10, 2021, 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.
> 
> ...
> 
> > What's your opinion? Did I miss anything?
> 
> Frankly, I think trying to smush them together would be a complete trainwreck.
> 
> The vast majority of flows would need to go down completely different paths, so
> you'd end up with code like this:
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index f2eac41bb4ff..5128043c7871 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -46,6 +46,9 @@ static int sgx_release(struct inode *inode, struct file *file)
>         struct sgx_encl *encl = file->private_data;
>         struct sgx_encl_mm *encl_mm;
>  
> +       if (encl->not_an_enclave)
> +               return sgx_virt_epc_release(encl);
> +
>         /*
>          * Drain the remaining mm_list entries. At this point the list contains
>          * entries for processes, which have closed the enclave file but have
> @@ -83,6 +86,9 @@ static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
>         struct sgx_encl *encl = file->private_data;
>         int ret;
>  
> +       if (encl->not_an_enclave)
> +               return sgx_virt_epc_mmap(encl, vma);
> +
>         ret = sgx_encl_may_map(encl, vma->vm_start, vma->vm_end, vma->vm_flags);
>         if (ret)
>                 return ret;
> @@ -104,6 +110,11 @@ static unsigned long sgx_get_unmapped_area(struct file *file,
>                                            unsigned long pgoff,
>                                            unsigned long flags)
>  {
> +       struct sgx_encl *encl = file->private_data;
> +
> +       if (encl->not_an_enclave)
> +               return sgx_virt_epc_mmap(encl, addr, len, pgoff, flags);
> +
>         if ((flags & MAP_TYPE) == MAP_PRIVATE)
>                 return -EINVAL;
> 
> I suspect it would also be tricky to avoid introducing races, since anything that
> is different for virtual EPC would have a dependency on the ioctl() being called.
> 
> This would also prevent making /dev/sgx_enclave root-only while allowing users
> access to /dev/sgx_vepc.  Forcing admins to use LSMs to do the same is silly.
> 
> For the few flows that can share code, just split out the common bits to helpers.

I'm cool with keeping the device. This is just my opinion that even
"obvious" should be documented when it comes to uapi. I.e. no matter
how stupid and simple reasons are to add a new device file, please
just write it down to commit message.

/Jarkko