Re: [PATCH v3 2/2] x86/sgx: Implement EUPDATESVN and opportunistically call it during first EPC page alloc

["Huang, Kai" <kai.huang@intel.com>]

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.
>  */