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@xxxxxxxxxx> > > > > 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@xxxxxxxxx/ > > 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.
