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

Kai Huang
2021-02-09T18:50:27Z

On Tue, 2021-02-09 at 08:18 -0800, Dave Hansen wrote:
> 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.

Oh sorry I missed this. I'll use yours in next version. Thanks.

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

Thanks. Agreed it's not good to mention specific caller name.

>
> > +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.

Yes. Adding comment should be another patch logically, and I don't want to introduce
it in this patch.