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

From: Kai Huang <kai.huang@intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
	linux-sgx@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org
Cc: seanjc@google.com, jarkko@kernel.org, luto@kernel.org,
	rick.p.edgecombe@intel.com, haitao.huang@intel.com,
	pbonzini@redhat.com, bp@alien8.de, tglx@linutronix.de,
	mingo@redhat.com, hpa@zytor.com
Subject: Re: [RFC PATCH v4 03/26] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()
Date: Wed, 10 Feb 2021 07:26:23 +1300	[thread overview]
Message-ID: <c8a87b4980edb5e21a7d14497e84a12827e57648.camel@intel.com> (raw)
In-Reply-To: <38eb4166-8687-2f0d-0ca3-66c1afdf0a5d@intel.com>

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.