Re: [PATCH v41 05/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections

From: Dave Hansen <dave.hansen@intel.com>
To: Hillf Danton <hdanton@sina.com>, Jarkko Sakkinen <jarkko@kernel.org>
Cc: x86@kernel.org, linux-sgx@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Jethro Beekman <jethro@fortanix.com>,
	Serge Ayoun <serge.ayoun@intel.com>,
	akpm@linux-foundation.org
Subject: Re: [PATCH v41 05/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections
Date: Mon, 16 Nov 2020 10:33:24 -0800	[thread overview]
Message-ID: <0a96ecb7-50cc-634d-2f94-09e547be7b44@intel.com> (raw)
In-Reply-To: <20201114084211.5284-1-hdanton@sina.com>

On 11/14/20 12:42 AM, Hillf Danton wrote:
> On Fri, 13 Nov 2020 00:01:16 +0200 Jarkko Sakkinen wrote:
>> + */
>> +static void sgx_sanitize_section(struct sgx_epc_section *section)
>> +{
>> +	struct sgx_epc_page *page;
>> +	LIST_HEAD(dirty);
>> +	int ret;
>> +
>> +	while (!list_empty(&section->laundry_list)) {
>> +		if (kthread_should_stop())
>> +			return;
>> +
>> +		spin_lock(&section->lock);
>> +
>> +		page = list_first_entry(&section->laundry_list,
>> +					struct sgx_epc_page, list);
>> +
>> +		ret = __eremove(sgx_get_epc_virt_addr(page));
>> +		if (!ret)
>> +			list_move(&page->list, &section->page_list);
>> +		else
>> +			list_move_tail(&page->list, &dirty);
>> +
>> +		spin_unlock(&section->lock);
>> +
>> +		cond_resched();
>> +	}
>> +
>> +	list_splice(&dirty, &section->laundry_list);
> 
> Move list splice into the section under section->lock.

The naming, commenting and changelogs could be better here.  But, I
think the code is correct.

section->lock actually only protects ->page_list.

->laundry_list is initialized in core code, but after that is only used
by ksgxd and is effectively a thread-local structure.

I can think of a few other ways of doing this so that, for instance,
laundry_list was a thread-local structure in ksgxd that is freed after
initialization and sanitizing.  But, this is pretty simple, although
under-documented, and wastes a list_head worth of space per section at
runtime.

>> +}
> [...]
> 
>> +struct sgx_epc_page {
>> +	unsigned int section;
> 
> To make the sgx page naturally packed, add a small pad to match both
> sizeof(struct list_head) and X86_64. Feel free to turn back on the
> pad OTOH to save memory.

You make a good point: it's pretty obvious that the current code is
space-optimized.  That doesn't seem like a bad thing to me, though.