Date:   Mon, 15 Mar 2021 09:03:21 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>, linux-sgx@...r.kernel.org
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] x86/sgx: Replace section local dirty page lists
 with a global list

On 3/13/21 8:01 AM, Jarkko Sakkinen wrote:
> Reset initialized EPC pages in sgx_dirty_page_list to uninitialized state,
> and free them using sgx_free_epc_page(). Do two passes, as for SECS pages
> the first round can fail, if all child pages have not yet been removed.
> The driver puts all pages on startup first to sgx_dirty_page_list, as the
> initialization could be triggered by kexec(), meaning that pages have been
> reserved for active enclaves before the operation.
> 
> The section local lists are redundant, as sgx_free_epc_page() figures
> out the correction by using epc_page->section.

During normal runtime, the "ksgxd" daemon behaves like a  version of
kswapd just for SGX.  But, its first job is to initialize enclave
memory.  This is done in a a separate thread because this initialization
can be quite slow.

Currently, the SGX boot code places each enclave page on a
sgx_section-local list (init_laundry_list).  Once it starts up, the
ksgxd code walks over that list and populates the actual SGX page allocator.

However, the per-section structures are going away to make way for the
SGX NUMA allocator.  There's also little need to have a per-section
structure; the enclave pages are all treated identically, and they can
be placed on the correct allocator list from metadata stoered in the
enclave page itself.

> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 65004fb8a91f..cb4561444b96 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -27,39 +27,10 @@ static LIST_HEAD(sgx_active_page_list);
>  static DEFINE_SPINLOCK(sgx_reclaimer_lock);
>  
>  /*
> - * Reset dirty EPC pages to uninitialized state. Laundry can be left with SECS
> - * pages whose child pages blocked EREMOVE.
> + * When the driver initialized, EPC pages go first here, as they could be
> + * initialized to an active enclave, on kexec entry.
>   */
> -static void sgx_sanitize_section(struct sgx_epc_section *section)
> -{
> -	struct sgx_epc_page *page;
> -	LIST_HEAD(dirty);
> -	int ret;
> -
> -	/* init_laundry_list is thread-local, no need for a lock: */
> -	while (!list_empty(&section->init_laundry_list)) {
> -		if (kthread_should_stop())
> -			return;
> -
> -		/* needed for access to ->page_list: */
> -		spin_lock(&section->lock);
> -
> -		page = list_first_entry(&section->init_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->init_laundry_list);
> -}
> +static LIST_HEAD(sgx_dirty_page_list);
>  
>  static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)
>  {
> @@ -400,25 +371,48 @@ static bool sgx_should_reclaim(unsigned long watermark)
>  
>  static int ksgxd(void *p)
>  {
> -	int i;
> +	struct sgx_epc_page *page;
> +	LIST_HEAD(dirty);
> +	int i, ret;
>  
>  	set_freezable();
>  
>  	/*
> -	 * Sanitize pages in order to recover from kexec(). The 2nd pass is
> -	 * required for SECS pages, whose child pages blocked EREMOVE.
> +	 * Reset initialized EPC pages in sgx_dirty_page_list to uninitialized state,
> +	 * and free them using sgx_free_epc_page(). 

I'm not a fan of comments that tell us verbatim what the code does.

>							Do two passes, as for SECS pages the
> +	 * first round can fail, if all child pages have not yet been removed.  The
> +	 * driver puts all pages on startup first to sgx_dirty_page_list, as the
> +	 * initialization could be triggered by kexec(), meaning that pages have been
> +	 * reserved for active enclaves before the operation.
>  	 */



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

FWIW, I don't like the removal of the helper here.  I really like kernel
threads' top-level function to be very understandable and clean.  This
makes it quite a bit harder to figure out what is going on.

For instance, we could just have a sgx_sanitize_pages() which has a
local dirty list and just calls:

void sgx_santitize_pages(void)
{
	LIST_HEAD(dirty);

	/*
	 * Comment about two passes
	 */
	__sgx_sanitize_pages(&dirty)
	__sgx_sanitize_pages(&dirty)
}

> +	/* sgx_dirty_page_list is thread-local to ksgxd, no need for a lock: */
> +	for (i = 0; i < 2 && !list_empty(&sgx_dirty_page_list); i++) {
> +		while (!list_empty(&sgx_dirty_page_list)) {
> +			if (kthread_should_stop())
> +				return 0;
> +
> +			page = list_first_entry(&sgx_dirty_page_list, struct sgx_epc_page, list);
> +
> +			ret = __eremove(sgx_get_epc_virt_addr(page));
> +			if (!ret) {
> +				/* The page is clean - move to the free list. */

I'd even say:
				/*
				 * page is now sanitized.  Make it
				 * available via the SGX page allocator:
				 */

See what that does?  It actually links the "cleaning" to the freeing.

> +				list_del(&page->list);
> +				sgx_free_epc_page(page);
> +			} else {
> +				/* The page is not yet clean - move to the dirty list. */
> +				list_move_tail(&page->list, &dirty);
> +			}
> +
> +			cond_resched();
> +		}
>  
> -		/* Should never happen. */
> -		if (!list_empty(&sgx_epc_sections[i].init_laundry_list))
> -			WARN(1, "EPC section %d has unsanitized pages.\n", i);
> +		list_splice(&dirty, &sgx_dirty_page_list);
>  	}
>  
> +	if (!list_empty(&sgx_dirty_page_list))
> +		WARN(1, "EPC section %d has unsanitized pages.\n", i);
> +
>  	while (!kthread_should_stop()) {
>  		if (try_to_freeze())
>  			continue;
> @@ -632,13 +626,12 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
>  	section->phys_addr = phys_addr;
>  	spin_lock_init(&section->lock);
>  	INIT_LIST_HEAD(&section->page_list);
> -	INIT_LIST_HEAD(&section->init_laundry_list);
>  
>  	for (i = 0; i < nr_pages; i++) {
>  		section->pages[i].section = index;
>  		section->pages[i].flags = 0;
>  		section->pages[i].owner = NULL;
> -		list_add_tail(&section->pages[i].list, &section->init_laundry_list);
> +		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>  	}
...

Powered by blists - more mailing lists