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.
> 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 :-)
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;
}
> }
>
> /*
> * 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. 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);
> 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.
/**
* __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.
*/
