Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 13 Jul 2021 16:56:24 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Tony Luck <tony.luck@intel.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        Dave Hansen <dave.hansen@intel.com>,
        Jarkko Sakkinen <jarkko.sakkinen@intel.com>
Subject: Re: [RFC PATCH 1/4] x86/sgx: Track phase and type of SGX EPC pages
Message-ID: <YO3FuBupQTKYaKBf@google.com>
References: <20210608214038.1026259-1-tony.luck@intel.com>
 <20210608214038.1026259-2-tony.luck@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210608214038.1026259-2-tony.luck@intel.com>
Precedence: bulk

On Tue, Jun 08, 2021, Tony Luck wrote:
> Add a type field to struct epc_page for how an in-use page has been
> allocated. Re-use "enum sgx_page_type" for this type, with a couple
> of additions for s/w types.

IMO, taking different actions based on the page type is flawed.  Ditto for adding
a flag to track so called "dirty" pages.

> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 3be203297988..abf6e1a704c0 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -72,7 +72,7 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page,
>  	struct sgx_epc_page *epc_page;
>  	int ret;
>  
> -	epc_page = sgx_alloc_epc_page(encl_page, false);
> +	epc_page = sgx_alloc_epc_page(encl_page, SGX_PAGE_TYPE_REG, false);

This is wrong when reloading an SECS page.

>  	if (IS_ERR(epc_page))
>  		return epc_page;
>  
> @@ -651,7 +667,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size,
>  
>  	for (i = 0; i < nr_pages; i++) {
>  		section->pages[i].section = index;
> -		section->pages[i].flags = 0;
> +		section->pages[i].flags = SGX_EPC_PAGE_DIRTY;

This flag is confusing, "dirty" in paging context almost always refers to the
page having been written.  The existing sgx_dirty_page_list name is not good,
fixing that instead of propagating the bad name would preferable.

But why is the flag needed?  If the flag is set iff the page is on the dirty
list, what's the point of the flag?

Looking forward to patch 3, why does sgx_memory_failure() need to eagerly remove
pages from the list?  Just have the sanitization skip poisoned pages.

> +		section->pages[i].type = SGX_PAGE_TYPE_FREE;
>  		section->pages[i].owner = NULL;
>  		list_add_tail(&section->pages[i].list, &sgx_dirty_page_list);
>  	}