RE: [PATCH v11 1/5] x86/sgx: Introduce functions to count the sgx_(vepc_)open()

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@xxxxxxxxx>
> Sent: Thursday, August 7, 2025 2:39 AM
> To: Reshetova, Elena <elena.reshetova@xxxxxxxxx>; Hansen, Dave
> <dave.hansen@xxxxxxxxx>
> Cc: seanjc@xxxxxxxxxx; mingo@xxxxxxxxxx; Scarlata, Vincent R
> <vincent.r.scarlata@xxxxxxxxx>; x86@xxxxxxxxxx; jarkko@xxxxxxxxxx;
> Annapurve, Vishal <vannapurve@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> Mallick, Asit K <asit.k.mallick@xxxxxxxxx>; Aktas, Erdem
> <erdemaktas@xxxxxxxxxx>; Cai, Chong <chongc@xxxxxxxxxx>; Bondarevska,
> Nataliia <bondarn@xxxxxxxxxx>; linux-sgx@xxxxxxxxxxxxxxx; Raynor, Scott
> <scott.raynor@xxxxxxxxx>
> Subject: Re: [PATCH v11 1/5] x86/sgx: Introduce functions to count the
> sgx_(vepc_)open()
> 
> 
> (sorry for back-and-forth and not saying those before, but since I found
> some issues in this version so I feel I should still point out.)

Thank you Kai for your review! Later is better than never ))

> 
> On Wed, 2025-08-06 at 11:11 +0300, Elena Reshetova wrote:
> > Currently SGX does not have a global counter to count the
> > active users from userspace or hypervisor.
> 
> First, the text wrap of this is not consistent with other lines.  It
> breaks too aggressively AFAICT.  I personally set textwidth=72, but I've
> seen other people suggesting to wrap at 75 characters.  But this looks too
> aggressive and not consistent with  other lines.
> 
> I also observed this problem in other patches too.  Could you check all
> changelogs and re-wrap the text if needed?

Ok, I can do the 75 wrap, I think this is considered standard. 

> 
> Back to technical:
> 
> "virtual EPC" is also opened from the userspace, so using "hypervisor"
> doesn't look quite right to me.
> 
> Also, it would be better to mention the "why" first, so people don't need
> to find out the reason after reading these "how"s.
> 
> How about:
> 
> Currently, when SGX is compromised and the microcode update fix is
> applied, the machine needs to be rebooted to invalidate old SGX crypto-
> assets and make SGX be in an updated safe state.  It's not friendly for
> the cloud.
> 
> To avoid having to reboot, a new ENCLS[EUPDATESVN] is introduced to update
> SGX environment at runtime.  This process needs to be done when there's no
> SGX user to make sure no compromised enclaves can survive from the update
> and allow the system to regenerate crypto-assets etc.
> 
> For now there's no counter to track the active SGX users of host enclave
> and virtual EPC.  Introduce such counter mechanism so that the EUPDATESVN
> can be done only when there's no SGX users.


Thank you! I am fine with this description, will use it. 

> 
> > Define placeholder
> > functions sgx_inc/dec_usage_count() that are used to increment
> > and decrement such a counter. Also, wire the call sites for
> > these functions.
> >
> 
> [...]
> 
> > For the latter, in order to introduce the
> > counting of active sgx users on top of clean functions that
> > allocate vepc structures
> >
> 
> It's not just "vepc structures" only, right?
> 
> Encapsulate the current sgx_(vepc_)open() to __sgx_(vepc_)open() to make
> the new sgx_(vepc_)open() easy to read.

Sure, will change to this wording. 

> 
> > , covert existing sgx_(vepc_)open() to __sgx_(vepc_)open().
>     ^
>     convert ?
> 
> >
> > The definition of the counter itself and the actual implementation
> > of these two functions comes next.
> 			 ^
> 			 come next ?

Yes, will fix.

> 
> > The counter will be used by
> > the driver that would be attempting to call EUPDATESVN SGX instruction
> > only when incrementing from zero.
> 
> This can be removed if my paragraphs are used (which already mentioned
> this).

Agree. 

> 
> >
> > Note: the sgx_inc_usage_count() prototype is defined to return
> > int for the cleanliness of the follow-up patches despite always
> > returning zero in this patch. When the EUPDATESVN SGX instruction
> > will be enabled in the follow-up patch, the sgx_inc_usage_count()
> 			 ^
> 			 follow-up patches?
> 
> > will start to return the actual return code.
> 
> Could this paragraph be shorter, like below?
> 
> The EUPDATESVN, which may fail, will be done in sgx_inc_usage_count().
> Make it return 'int' to make subsequent patches which implement
> EUPDATESVN
> easier to review.  For now it always returns success.

Sure, will change. 

> 
> 
> [...]
> 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 308dbbae6c6e..cf149b9f4916 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -765,6 +765,7 @@ void sgx_encl_release(struct kref *ref)
> >  	WARN_ON_ONCE(encl->secs.epc_page);
> >
> >  	kfree(encl);
> > +	sgx_dec_usage_count();
> >  }
> >
> >
> 
> [...]
> 
> > --- a/arch/x86/kernel/cpu/sgx/virt.c
> > +++ b/arch/x86/kernel/cpu/sgx/virt.c
> > @@ -255,10 +255,11 @@ static int sgx_vepc_release(struct inode *inode,
> struct file *file)
> >  	xa_destroy(&vepc->page_array);
> >  	kfree(vepc);
> >
> > +	sgx_dec_usage_count();
> >  	return 0;
> >  }
> 
> Given we have __sgx_(vepc_)open(), I think it makes more sense to have
> __sgx_(encl_|vepc_)release() counterpart?

Is it worth it? In case of *_open() variants there are quite error handling
under different cases, but for release as you can see it is just a one-line
addition. Not sure it is worth adding the wrappers just for that. 
But I can change it if people think it would look better this way. 

Best Regards,
Elena.