On Mon, 2021-03-01 at 09:29 -0800, Sean Christopherson wrote:

On Mon, Mar 01, 2021, Kai Huang wrote:

quoted

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 7449ef33f081..a7dc86e87a09 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c

@@ -381,6 +381,26 @@ const struct vm_operations_struct sgx_vm_ops = {

 	.access = sgx_vma_access,
 };
 



+static void sgx_encl_free_epc_page(struct sgx_epc_page *epc_page)
+{
+	int ret;
+
+	WARN_ON_ONCE(epc_page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
+
+	ret = __eremove(sgx_get_epc_virt_addr(epc_page));
+	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) {

This can be ENCLS_WARN, especially if you're printing a separate error message
about leaking the page.  That being said, I'm not sure a seperate error message
is a good idea.  If other stuff gets dumped to the kernel log between the WARN
and the pr_err_once(), it may not be clear to admins that the two events are
directly connected.  It's even possible the prints could come from two different
CPUs.

Good point. Thanks for educating me :)

Why not dump a short blurb in the WARN itself?  The error message can be thrown
in a define if the line length is too obnoxious (it's ~109 chars if embedded
directly).

#define EREMOVE_ERROR_MESSAGE \
	"EREMOVE returned %d (0x%x).  EPC page leaked, reboot recommended."

	if (WARN_ONCE(ret, EREMOVE_ERROR_MESSAGE, ret, ret))

Will do in your way. Thanks!

quoted

+		/*
+		 * Give a message to remind EPC page is leaked, and requires
+		 * machine reboot to get leaked pages back. This can be improved
+		 * in the future by adding stats of leaked pages, etc.
+		 */
+		pr_err_once("EPC page is leaked. Require machine reboot to get leaked pages back.\n");
+		return;
+	}
+
+	sgx_free_epc_page(epc_page);
+}
+
 /**
  * sgx_encl_release - Destroy an enclave instance
  * @kref:	address of a kref inside &sgx_encl