Re: [RFC PATCH v4 03/26] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()

Dave Hansen
2021-02-09T16:19:52Z

On 2/8/21 2:54 AM, Kai Huang wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
>
> Encapsulate the snippet in sgx_free_epc_page() concerning EREMOVE to
> sgx_reset_epc_page(), which is a static helper function for
> sgx_encl_release(). It's the only function existing, which deals with
> initialized pages.

I didn't really like the changelog the last time around, so I wrote you
a new one:

> https://lore.kernel.org/kvm/8250aedb-a623-646d-071a-75ece2c41c09@intel.com/

The "v4" changelog is pretty hard for me to read. It doesn't tell me
why we can "wipe out EREMOVE" or how it is going to get used going forward.

> +
> +/*
> + * Place the page in uninitialized state. Called by in sgx_encl_release()
> + * before sgx_free_epc_page(), which requires EPC page is already in clean
> + * slate.
> + */

I really don't like comments like that that refer to callers. They're
basically guaranteed to become obsolete.

/*
* Place the page in uninitialized state. Only usable by callers that
* know the page is in a clean state in which EREMOVE will succeed.
*/

> +static void sgx_reset_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))
> + return;
> +}

The uncommented warnings aren't very nice, but I guess they're in the
existing code.