Re: [PATCH v5 4/5] x86/sgx: Implement ENCLS[EUPDATESVN]

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, May 20, 2025 at 06:31:46AM +0000, Reshetova, Elena wrote:

BTW, please keep the line which tells who responded.

>  > +/**
> > > + * sgx_updatesvn() - Attempt to call ENCLS[EUPDATESVN]
> > > + * If EPC is empty, this instruction attempts to update CPUSVN to the
> > > + * currently loaded microcode update SVN and generate new
> > > + * cryptographic assets.sgx_updatesvn() Most of the time, there will
> > 
> > Is there something wrong here in the text? It looks malformed.
> 
> Yes, sorry, looks like copy-paste error I missed in the comment. 
> Will fix. 
> 
> > 
> > > + * be no update and that's OK.
> > > + *
> > > + * Return:
> > > + * 0: Success, not supported or run out of entropy
> > > + */
> > > +static int sgx_update_svn(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * If EUPDATESVN is not available, it is ok to
> > > +	 * silently skip it to comply with legacy behavior.
> > > +	 */
> > > +	if (!X86_FEATURE_SGX_EUPDATESVN)
> > > +		return 0;
> > > +
> > > +	for (int i = 0; i < RDRAND_RETRY_LOOPS; i++) {
> > > +		ret = __eupdatesvn();
> > > +
> > > +		/* Stop on success or unexpected errors: */
> > > +		if (ret != SGX_INSUFFICIENT_ENTROPY)
> > > +			break;
> > > +	}
> > > +
> > > +	/*
> > > +	 * SVN either was up-to-date or SVN update failed due
> > > +	 * to lack of entropy. In both cases, we want to return
> > > +	 * 0 in order not to break sgx_(vepc_)open. We dont expect
> > > +	 * SGX_INSUFFICIENT_ENTROPY error unless underlying RDSEED
> > > +	 * is under heavy pressure.
> > > +	 */
> > > +	if ((ret == SGX_NO_UPDATE) || (ret == SGX_INSUFFICIENT_ENTROPY))
> > 
> > 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)
> 
> Ok, but I will have to change this anyhow since we seems to trend that we want
> to return -EBUSY when SGX_INSUFFICIENT_ENTROPY and do not
> proceed with open() call. 
> 
> > 
> > > +		return 0;
> > > +
> > > +	if (!ret) {
> > > +		/*
> > > +		 * SVN successfully updated.
> > > +		 * Let users know when the update was successful.
> > > +		 */
> > 
> > This comment is like as useless as an inline comment can ever possibly
> > be. Please, remove it.
> 
> It is actually not quite so useless because this is the rare case we know
> the EUPDATESVN actually executed and hence the pr_info also below.
> Without this, there will be no way for sysadmin to trace whenever CPU
> SVN was upgraded or not (Sean mentioned that this is already pretty
> opaque to users). 
> 
> > 
> > > +		pr_info("SVN updated successfully\n");
> > 
> > Let's not add this either in the scope of this patch set.
> 
> See above. 
> 
> > 
> > > +		return 0;
> > > +	}
> > 
> > Since you parse error codes already, I don't understand why deal with
> > the success case in the middle of doing that.
> > 
> > More consistent would be (not also the use of unlikely()):
> > 
> > 	if (ret == SGX_NO_UPDATE || ret == SGX_INSUFFICIENT_ENTROPY)
> > 		return 0;
> > 
> > 	/*
> > 	 * EUPDATESVN was called when EPC is empty, all other error
> > 	 * codes are unexpected.
> > 	 */
> > 	if (unlikely(ret)) {
> > 		ENCLS_WARN(ret, "EUPDATESVN");
> > 		return ret;
> > 	}
> > 
> > 	return 0;
> > }
> > 
> > This is how I would rewrite the tail of this function.
> 
> I think everyone already re-wrote this function at least once and no one is
> happy with the version from previous person )) 
> Let me try another version again, taking into account changes in return codes
> discussed in this thread also. 

unlikely() is both (minor) optimization and documents that it is not expected
branch so it obviously makes sense here.

> 
> Best Regards,
> Elena.

BR, Jarkko