On Fri, 2025-04-18 at 07:18 -0700, Sean Christopherson wrote:
> On Thu, Apr 17, 2025, Kai Huang wrote:
> > I think the sgx_updatesvn() should just return true when EUPDATESVN returns
> > 0 or
> > SGX_NO_UPDATE, and return false for all other error codes. And it should
> > ENCLS_WARN() for all other error codes, except SGX_INSUFFICIENT_ENTROPY
> > because
> > it can still legally happen.
> >
> > Something like:
> >
> > do {
> > ret = __eupdatesvn();
> > if (ret != SGX_INSUFFICIENT_ENTROPY)
> > break;
> > } while (--retry);
>
> This can be:
>
> do {
> ret = __eupdatesvn();
> } while (ret == SGX_INSUFFICIENT_ENTROPY && --retry)
>
> To make it super obvious that retry is only relevant to lack of entropy.
Yep looks better.
>
> > if (!ret || ret == SGX_NO_UPDATE) {
> > /*
> > * SVN successfully updated, or it was already up-to-date.
> > * Let users know when the update was successful.
> > */
> > if (!ret)
> > pr_info("SVN updated successfully\n");
> > return true;
>
> Returning true/false is confusing since the vast majority of the SGX code uses
> '0' for success. A lot of cleverness went into splicing SGX's error codes
> into
> the kernel's -ernno; it would be a shame to ignore that :-)
Agreed :-)
>
> E.g. this looks wrong at first (and second glance)
>
> ret = sgx_updatesvn();
> if (!ret) {
> /*
> * sgx_updatesvn() returned unknown error, smth
> * must be broken, do not allocate a page from
> EPC
> */
> spin_unlock(&sgx_epc_eupdatesvn_lock);
> spin_unlock(&node->lock);
> return NULL;
> }
Yep.
>
> > }
> >
> > /*
> > * EUPDATESVN was called when EPC is empty, all other error
> > * codes are unexcepted except running out of entropy.
> > */
> > if (ret != SGX_INSUFFICIENT_ENTROPY)
> > ENCLS_WARN(ret, "EUPDATESVN");
> >
> > return false;
> >
> >
> > In __sgx_alloc_epc_page_from_node(), it should fail to allocate EPC page and
> > return -ENOMEM when sgx_updatesvn() returns false. We should only allow
> > EPC to
>
> No, it should return a meaningful error code, not -ENOMEM.
>
Oh I was actually thinking to keep __sgx_alloc_epc_page_from_node() returning
NULL if sgx_updatesvn() fails (given it returns true/false) and make
sgx_alloc_epc_page() return -ENOMEM.
Sorry for didn't say fully. :-(
> And if that's the
> behavior you want, then __sgx_alloc_epc_page() should be updated to bail
> immediately.
> The current code assuming -ENOMEM is the only failure scenario:
>
> do {
> page = __sgx_alloc_epc_page_from_node(nid);
> if (page)
> return page;
>
> nid = next_node_in(nid, sgx_numa_mask);
> } while (nid != nid_start);
>
> That should be something like:
>
> do {
> page = __sgx_alloc_epc_page_from_node(nid);
> if (!IS_ERR(page) || PTR_ERR(page) != -ENOMEM)
> return page;
>
> nid = next_node_in(nid, sgx_numa_mask);
> } while (nid != nid_start);
If we want to bail out immediately, then perhaps -EIO is a better option instead
of -ENOMEM. And in this case sgx_alloc_epc_page() can also bail out early:
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -569,6 +569,9 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool
reclaim)
break;
}
+ if (PTR_ERR(page) == -EIO)
+ return page;
+
if (list_empty(&sgx_active_page_list))
return ERR_PTR(-ENOMEM);
>
> > be allocated when we know the SVN is already up-to-date.
> >
> > Any further call of EPC allocation will trigger sgx_updatesvn() again. If
> > it
> > was failed due to unexpected error, then it should continue to fail,
> > guaranteeing "sgx_alloc_epc_page() return consistently -ENOMEM, if the
> > unexpected happens". If it was failed due to running out of entropy, it
> > then
> > may fail again, or it will just succeed and then SGX can continue to work.
>
>
> Side topic, the function comment for __sgx_alloc_epc_page() is stale/wrong.
> It
> returns ERR_PTR(-ENOMEM), not NULL, on failure.
Right :-)
Thanks for spending time on review!
>
> /**
> * __sgx_alloc_epc_page() - Allocate an EPC page
> *
> * Iterate through NUMA nodes and reserve ia free EPC page to the caller.
> Start
> * from the NUMA node, where the caller is executing.
> *
> * Return:
> * - an EPC page: A borrowed EPC pages were available.
> * - NULL: Out of EPC pages.
> */
