RE: [PATCH v7 5/5] x86/sgx: Enable automatic SVN updates for SGX enclaves

[Reshetova, Elena]

> On 7/11/25 09:50, Elena Reshetova wrote:
> > == Background ==
> >
> > ENCLS[EUPDATESVN] is a new SGX instruction [1] which allows enclave
> > attestation to include information about updated microcode SVN without a
> > reboot. Before an EUPDATESVN operation can be successful, all SGX memory
> > (aka. EPC) must be marked as “unused” in the SGX hardware metadata
> > (aka.EPCM). This requirement ensures that no compromised enclave can
> > survive the EUPDATESVN procedure and provides an opportunity to generate
> > new cryptographic assets.
> >
> > == Patch Contents ==
> >
> > Attempt to execute ENCLS[EUPDATESVN] every time the first file descriptor
> > is obtained via sgx_(vepc_)open(). In the most common case the microcode
> > SVN is already up-to-date, and the operation succeeds without updating SVN.
> > If it fails with any other error code than SGX_INSUFFICIENT_ENTROPY, this
> > is considered unexpected and the *open() returns an error. This should not
> > happen in practice. On contrary, SGX_INSUFFICIENT_ENTROPY might happen
> due
> > to a pressure on the system's DRNG (RDSEED) and therefore the *open() can
> > be safely retried to allow normal enclave operation.
> 
> The effort to add paragraphs would be appreciated.

You mean break this paragrah into two starting from "On contrary" ? 

> 
> >  int sgx_inc_usage_count(void)
> >  {
> > -	atomic64_inc(&sgx_usage_count);
> > -	return 0;
> > +	int ret;
> ...
> 
> For what it does sgx_inc_usage_count() is seriously hard to parse and
> complicated. Three logical atomic ops *and* a spinlock? Wouldn't this
> suffice?
> 
> Just make 'sgx_usage_count' a normal integer and always guard it with a
> the existing lock:
> 
> int sgx_inc_usage_count(void)
> {
> 	int ret;
> 
> 	guard(mutex)(&sgx_svn_lock);
> 
> 	if (sgx_usage_count++ == 0)
> 		return sgx_update_svn();
> 
> 	return 0;
> }
> 
> Yeah, it makes the fast path a spinlock instead of an atomic_inc, but
> it's still the same number of atomic ops in the end.
> 
> Isn't that a billion times more readable? It barely even needs commenting.

Yes, agree, billion times more readable. 

> 
> What am I missing?

I think you put it: this would require a spinlock in the fast path and
*in theory* if we are running many many concurrent enclaves can create 
contention. Whenever this is a realistic *practical deployment problem*
I don't know. On the other hand, since we switched per Sean's suggestion
to taking a lock per sgx_open() vs. per each EPC page manipulation, the
contention is only on enclave creation, so this would require many
concurrent enclaves being constantly created, which even further limits
a problem to a particular deployment. Do we have such deployments
where people constantly create and destroy big number of enclaves?
Maybe instead of trying to find this out we go with your suggestion,
and only if there is a practical problem reported, go back to atomic version? 

Best Regards,
Elena.