On Mon, 2021-03-01 at 09:29 -0800, Sean Christopherson wrote: > On Mon, Mar 01, 2021, Kai Huang wrote: > > 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! > > > + /* > > + * 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