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

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

On Fri, 2021-02-12 at 14:17 +0200, Jarkko Sakkinen wrote:
> 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

Yes reasonable. I added some description to the commit message. I have already sent
out the v5. Please take a look and see whether it is OK for you. Thanks!