Re: [PATCH v25 07/21] x86/sgx: Enumerate and track EPC sections

[Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>]

On Thu, Feb 06, 2020 at 01:11:47AM +0200, Jarkko Sakkinen wrote:
> On Wed, Feb 05, 2020 at 11:57:00AM -0800, Sean Christopherson wrote:
> > > +		ret = __eremove(sgx_epc_addr(page));
> > > +		if (!WARN_ON_ONCE(ret)) {
> > 
> > Sadly, this WARN can fire after kexec() on systems with multiple EPC
> > sections if the SECS has child pages in another section.
> 
> What causes this?

Ah obviously this can happen given that the final loop is done per
section before other sections are processed.

Lets fix the code first rather than change the approach based on code
that has an underlying regression. Performance can be fine tuned even
after upstreaming. Especially if the performance increases complexity it
is better to work that after there is a mainline code base.

Given that the loop is done in separate thread anyway, I'm not sure how
bad performance issue there is anyway. Performance based changes should
be always done based on a workloads and statistics.

I think you'd fix this issue by first changing the functions as:

static void sgx_sanitize_section(struct sgx_epc_section *section)
{
	struct sgx_epc_page *page, *tmp;
	LIST_HEAD(secs_list);
	int ret;

	while (!list_empty(&section->unsanitized_page_list)) {
		if (kthread_should_stop())
			return;

		spin_lock(&section->lock);

		page = list_first_entry(&section->unsanitized_page_list,
					struct sgx_epc_page, list);

		ret = __eremove(sgx_epc_addr(page));
		if (!ret)
			list_move(&page->list, &section->page_list);
		else
			list_move_tail(&page->list, &secs_list);

		spin_unlock(&section->lock);

		cond_resched();
	}

	list_move_tail(&secs_list, &section->unsanitized_list);
}

Then in ksgxswapd() you'd

for (i = 0; i < sgx_nr_epc_sections; i++)
	sgx_sanitize_section(&sgx_epc_sections[i]);

/* 2nd round for SECS */
for (i = 0; i < sgx_nr_epc_sections; i++)
	sgx_sanitize_section(&sgx_epc_sections[i]);

Finally you'd:

for (i = 0; i < sgx_nr_epc_sections; i++)
	WARN_ONCE(!list_empty(&sgx_epc_sections[i]->unsanitized_list));

/Jarkko