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
