Re: [PATCH v13 09/13] x86/sgx: Enclave Page Cache (EPC) memory manager

[Sean Christopherson]

On Mon, 2018-08-27 at 21:53 +0300, Jarkko Sakkinen wrote:
> Add a Enclave Page Cache (EPC) memory manager that can be used to
> allocate and free EPC pages. The swapper thread ksgxswapd reclaims pages
> on the event when the number of free EPC pages goes below
> %SGX_NR_LOW_PAGES up until it reaches %SGX_NR_HIGH_PAGES.
> 
> Pages are reclaimed in LRU fashion from a global list. The consumers
> take care of calling EBLOCK (block page from new accesses), ETRACK
> (restart counting the entering hardware threads) and EWB (write page to
> the regular memory) because executing these operations usually (if not
> always) requires to do some subsystem-internal locking operations.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> Co-developed-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> Âarch/x86/include/asm/sgx.hÂÂÂÂÂÂ|ÂÂ56 ++++--
> Âarch/x86/kernel/cpu/intel_sgx.c | 322 ++++++++++++++++++++++++++++++++
> Â2 files changed, 362 insertions(+), 16 deletions(-)

...

> +/**
> + * sgx_reclaim_pages - reclaim EPC pages from the consumers
> + *
> + * Takes a fixed chunk of pages from the global list of consumed EPC pages and
> + * tries to swap them. Only the pages that are either being freed by the
> + * consumer or actively used are skipped.
> + */
> +static void sgx_reclaim_pages(void)
> +{
> +	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN + 1];

The array size should simply be SGX_NR_TO_SCAN. ÂThe +1 is a remnant
from the previous version that bounded the for-loops with "!chunk[i]"
check instead of "i < j". ÂNo functional issue, essentially just an
unused variable.

> +	struct sgx_epc_page *epc_page;
> +	struct sgx_epc_bank *bank;
> +	int i, j;
> +
> +	spin_lock(&sgx_active_page_list_lock);
> +	for (i = 0, j = 0; i < SGX_NR_TO_SCAN; i++) {
> +		if (list_empty(&sgx_active_page_list))
> +			break;
> +
> +		epc_page = list_first_entry(&sgx_active_page_list,
> +					ÂÂÂÂstruct sgx_epc_page, list);
> +		list_del_init(&epc_page->list);
> +
> +		if (epc_page->impl->ops->get(epc_page))
> +			chunk[j++] = epc_page;
> +		else
> +			epc_page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> +	}
> +	spin_unlock(&sgx_active_page_list_lock);
> +
> +	for (i = 0; i < j; i++) {
> +		epc_page = chunk[i];
> +		if (epc_page->impl->ops->reclaim(epc_page))
> +			continue;
> +
> +		spin_lock(&sgx_active_page_list_lock);
> +		list_add_tail(&epc_page->list, &sgx_active_page_list);
> +		spin_unlock(&sgx_active_page_list_lock);
> +
> +		epc_page->impl->ops->put(epc_page);
> +		chunk[i] = NULL;
> +	}
> +
> +	for (i = 0; i < j; i++) {
> +		epc_page = chunk[i];
> +		if (epc_page)
> +			epc_page->impl->ops->block(epc_page);
> +	}
> +
> +	for (i = 0; i < j; i++) {
> +		epc_page = chunk[i];
> +		if (epc_page) {
> +			epc_page->impl->ops->write(epc_page);
> +			epc_page->impl->ops->put(epc_page);
> +
> +			/*
> +			Â* Put the page back on the free list only after we
> +			Â* have put() our reference to the owner of the EPC
> +			Â* page, otherwise the page could be re-allocated and
> +			Â* we'd call put() on the wrong impl.
> +			Â*/
> +			epc_page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> +
> +			bank = sgx_epc_bank(epc_page);
> +			spin_lock(&bank->lock);
> +			bank->pages[bank->free_cnt++] = epc_page;
> +			spin_unlock(&bank->lock);
> +		}
> +	}
> +}