Re: [PATCH v4 1/3] x86/sgx: Use sgx_free_epc_page() in sgx_reclaim_pages()

[Dave Hansen <dave.hansen@xxxxxxxxx>]

On 3/13/21 8:01 AM, Jarkko Sakkinen wrote:
> Replace the ad-hoc code with a sgx_free_epc_page(), in order to make sure
> that all the relevant checks and book keeping is done, while freeing a
> borrowed EPC page, and remove redundant code. EREMOVE inside
> sgx_free_epc_page() does not change the semantics, as EREMOVE to an
> uninitialize pages is a nop.

  ^ uninitialized

I know this is a short patch, but this changelog still falls a bit short
for me.

Why is this patch a part of _this_ series?  What *problem* does it
solve, related to this series?

It would also be nice to remind me why the EREMOVE is redundant.  Why
didn't we need one before?  What put the page in the uninitialized
state?  Is EREMOVE guaranteed to do nothing?  How expensive is it?



> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 8df81a3ed945..65004fb8a91f 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -305,7 +305,6 @@ static void sgx_reclaim_pages(void)
>  {
>  	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
>  	struct sgx_backing backing[SGX_NR_TO_SCAN];
> -	struct sgx_epc_section *section;
>  	struct sgx_encl_page *encl_page;
>  	struct sgx_epc_page *epc_page;
>  	pgoff_t page_index;
> @@ -378,11 +377,7 @@ static void sgx_reclaim_pages(void)
>  		kref_put(&encl_page->encl->refcount, sgx_encl_release);
>  		epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>  
> -		section = &sgx_epc_sections[epc_page->section];
> -		spin_lock(&section->lock);
> -		list_add_tail(&epc_page->list, &section->page_list);
> -		section->free_cnt++;
> -		spin_unlock(&section->lock);
> +		sgx_free_epc_page(epc_page);
>  	}
>  }
>  
>