[PATCH 0/2] v22 fixes

[PATCH 0/2] v22 fixes

[Jarkko Sakkinen]

Jarkko Sakkinen (2):
  x86/sgx: Remove duplicate check for entry->epc_page in
    sgx_encl_load_page()
  x86/sgx: Determine SECS at compile time in sgx_encl_eldu()

 arch/x86/kernel/cpu/sgx/driver/ioctl.c |  4 +--
 arch/x86/kernel/cpu/sgx/encl.c         | 50 ++++++++++++--------------
 arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
 arch/x86/kernel/cpu/sgx/reclaim.c      | 34 +++++++++++-------
 4 files changed, 48 insertions(+), 44 deletions(-)

-- 
2.20.1

[PATCH 1/2] x86/sgx: Remove duplicate check for entry->epc_page in sgx_encl_load_page()

[Jarkko Sakkinen]

The existence of the page is checked first hand for legit race
conditions (either two or more concurrent threads running the #PF
handler or the reclaimer has taken over the page):

/* Page is already resident in the EPC. */
if (entry->epc_page) {
	if (entry->desc & SGX_ENCL_PAGE_RECLAIMED)
		return ERR_PTR(-EBUSY);

	return entry;
}

After that the existence is a checked as a condition for ELDU.

This commit removes the redundant check.

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/encl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index a20d498e9dcd..d6397f7ef3b8 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -123,7 +123,7 @@ static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
 			return ERR_CAST(epc_page);
 	}
 
-	epc_page = entry->epc_page ? entry->epc_page : sgx_encl_eldu(entry);
+	epc_page = sgx_encl_eldu(entry);
 	if (IS_ERR(epc_page))
 		return ERR_CAST(epc_page);
 
-- 
2.20.1

[PATCH 2/2] x86/sgx: Determine SECS at compile time in sgx_encl_eldu()

[Jarkko Sakkinen]

Using address resolution of any kind is obviously an overkill for
anything that we know at compile time. Those sites should not hard
bind how we store SECS.

This commit adds @secs_child to sgx_encl_eldu() and sgx_encl_ewb()
and replaces sgx_encl_get_index() with a macro SGX_ENCL_PAGE_INDEX()

The new macro assumes that it is operated on SECS children and it is a
right call because it neither makes sense to bind large architectural
decisions inside the smallest helpers (e.g. where and how we store SECS).

Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c |  4 +--
 arch/x86/kernel/cpu/sgx/encl.c         | 50 ++++++++++++--------------
 arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
 arch/x86/kernel/cpu/sgx/reclaim.c      | 34 +++++++++++-------
 4 files changed, 48 insertions(+), 44 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 9b784a061a47..fbf2aa9da5fc 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -82,7 +82,7 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req,
 {
 	struct sgx_encl_page *encl_page = req->encl_page;
 	struct sgx_encl *encl = req->encl;
-	unsigned long page_index = sgx_encl_get_index(encl_page);
+	unsigned long page_index = SGX_ENCL_PAGE_INDEX(encl_page);
 	struct sgx_secinfo secinfo;
 	struct sgx_pageinfo pginfo;
 	struct page *backing;
@@ -475,7 +475,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
 			       struct sgx_secinfo *secinfo,
 			       unsigned int mrmask)
 {
-	unsigned long page_index = sgx_encl_get_index(encl_page);
+	unsigned long page_index = SGX_ENCL_PAGE_INDEX(encl_page);
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_add_page_req *req = NULL;
 	struct page *backing;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index d6397f7ef3b8..16d97198dafb 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -13,18 +13,26 @@
 #include "sgx.h"
 
 static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
-			   struct sgx_epc_page *epc_page)
+			   struct sgx_epc_page *epc_page, bool secs_child)
 {
 	unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page);
 	struct sgx_encl *encl = encl_page->encl;
-	pgoff_t page_index = sgx_encl_get_index(encl_page);
-	pgoff_t pcmd_index = sgx_pcmd_index(encl, page_index);
-	unsigned long pcmd_offset = sgx_pcmd_offset(page_index);
 	struct sgx_pageinfo pginfo;
+	unsigned long pcmd_offset;
 	struct page *backing;
+	pgoff_t page_index;
+	pgoff_t pcmd_index;
 	struct page *pcmd;
 	int ret;
 
+	if (secs_child)
+		page_index = SGX_ENCL_PAGE_INDEX(encl_page);
+	else
+		page_index = PFN_DOWN(encl->size);
+
+	pcmd_index = sgx_pcmd_index(encl, page_index);
+	pcmd_offset = sgx_pcmd_offset(page_index);
+
 	backing = sgx_encl_get_backing_page(encl, page_index);
 	if (IS_ERR(backing)) {
 		ret = PTR_ERR(backing);
@@ -40,8 +48,11 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
 	pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
 	pginfo.contents = (unsigned long)kmap_atomic(backing);
 	pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset;
-	pginfo.secs = SGX_ENCL_PAGE_IS_SECS(encl_page) ? 0 :
-			(unsigned long)sgx_epc_addr(encl->secs.epc_page);
+
+	if (secs_child)
+		pginfo.secs = (u64)sgx_epc_addr(encl->secs.epc_page);
+	else
+		pginfo.secs = 0;
 
 	ret = __eldu(&pginfo, sgx_epc_addr(epc_page),
 		     sgx_epc_addr(encl_page->va_page->epc_page) + va_offset);
@@ -64,7 +75,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
 	return ret;
 }
 
-static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page)
+static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page,
+					  bool secs_child)
 {
 	unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page);
 	struct sgx_encl *encl = encl_page->encl;
@@ -75,7 +87,7 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page)
 	if (IS_ERR(epc_page))
 		return epc_page;
 
-	ret = __sgx_encl_eldu(encl_page, epc_page);
+	ret = __sgx_encl_eldu(encl_page, epc_page, secs_child);
 	if (ret) {
 		sgx_free_page(epc_page);
 		return ERR_PTR(ret);
@@ -118,12 +130,12 @@ static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
 	}
 
 	if (!(encl->secs.epc_page)) {
-		epc_page = sgx_encl_eldu(&encl->secs);
+		epc_page = sgx_encl_eldu(&encl->secs, false);
 		if (IS_ERR(epc_page))
 			return ERR_CAST(epc_page);
 	}
 
-	epc_page = sgx_encl_eldu(entry);
+	epc_page = sgx_encl_eldu(entry, true);
 	if (IS_ERR(epc_page))
 		return ERR_CAST(epc_page);
 
@@ -534,24 +546,6 @@ void sgx_encl_release(struct kref *ref)
 	kfree(encl);
 }
 
-/**
- * sgx_encl_get_index() - Convert a page descriptor to a page index
- * @page:	an enclave page
- *
- * Given an enclave page descriptor, convert it to a page index used to access
- * backing storage. The backing page for SECS is located after the enclave
- * pages.
- */
-pgoff_t sgx_encl_get_index(struct sgx_encl_page *page)
-{
-	struct sgx_encl *encl = page->encl;
-
-	if (SGX_ENCL_PAGE_IS_SECS(page))
-		return PFN_DOWN(encl->size);
-
-	return PFN_DOWN(page->desc - encl->base);
-}
-
 /**
  * sgx_encl_encl_get_backing_page() - Pin the backing page
  * @encl:	an enclave
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index d3a1687ed84c..b7d4494d09ba 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -40,7 +40,8 @@ enum sgx_encl_page_desc {
 	((page)->desc & SGX_ENCL_PAGE_ADDR_MASK)
 #define SGX_ENCL_PAGE_VA_OFFSET(page) \
 	((page)->desc & SGX_ENCL_PAGE_VA_OFFSET_MASK)
-#define SGX_ENCL_PAGE_IS_SECS(page) ((page) == &(page)->encl->secs)
+#define SGX_ENCL_PAGE_INDEX(page) \
+	PFN_DOWN((page)->desc - (page)->encl->base)
 
 struct sgx_encl_page {
 	unsigned long desc;
@@ -120,7 +121,6 @@ int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
 		  struct vm_area_struct **vma);
 void sgx_encl_destroy(struct sgx_encl *encl);
 void sgx_encl_release(struct kref *ref);
-pgoff_t sgx_encl_get_index(struct sgx_encl_page *page);
 struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, pgoff_t index);
 int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm);
 int sgx_encl_test_and_clear_young(struct mm_struct *mm,
diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
index 7da9631ed434..6acc9d41aab3 100644
--- a/arch/x86/kernel/cpu/sgx/reclaim.c
+++ b/arch/x86/kernel/cpu/sgx/reclaim.c
@@ -220,17 +220,26 @@ static void sgx_reclaimer_block(struct sgx_epc_page *epc_page)
 }
 
 static int __sgx_encl_ewb(struct sgx_encl *encl, struct sgx_epc_page *epc_page,
-			  struct sgx_va_page *va_page, unsigned int va_offset)
+			  struct sgx_va_page *va_page, unsigned int va_offset,
+			  bool secs_child)
 {
 	struct sgx_encl_page *encl_page = epc_page->owner;
-	pgoff_t page_index = sgx_encl_get_index(encl_page);
-	pgoff_t pcmd_index = sgx_pcmd_index(encl, page_index);
-	unsigned long pcmd_offset = sgx_pcmd_offset(page_index);
 	struct sgx_pageinfo pginfo;
+	unsigned long pcmd_offset;
 	struct page *backing;
+	pgoff_t page_index;
+	pgoff_t pcmd_index;
 	struct page *pcmd;
 	int ret;
 
+	if (secs_child)
+		page_index = SGX_ENCL_PAGE_INDEX(encl_page);
+	else
+		page_index = PFN_DOWN(encl->size);
+
+	pcmd_index = sgx_pcmd_index(encl, page_index);
+	pcmd_offset = sgx_pcmd_offset(page_index);
+
 	backing = sgx_encl_get_backing_page(encl, page_index);
 	if (IS_ERR(backing)) {
 		ret = PTR_ERR(backing);
@@ -291,7 +300,7 @@ static const cpumask_t *sgx_encl_ewb_cpumask(struct sgx_encl *encl)
 	return cpumask;
 }
 
-static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
+static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool secs_child)
 {
 	struct sgx_encl_page *encl_page = epc_page->owner;
 	struct sgx_encl *encl = encl_page->encl;
@@ -308,7 +317,8 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
 		if (sgx_va_page_full(va_page))
 			list_move_tail(&va_page->list, &encl->va_pages);
 
-		ret = __sgx_encl_ewb(encl, epc_page, va_page, va_offset);
+		ret = __sgx_encl_ewb(encl, epc_page, va_page, va_offset,
+				     secs_child);
 		if (ret == SGX_NOT_TRACKED) {
 			ret = __etrack(sgx_epc_addr(encl->secs.epc_page));
 			if (ret) {
@@ -318,7 +328,7 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
 			}
 
 			ret = __sgx_encl_ewb(encl, epc_page, va_page,
-					     va_offset);
+					     va_offset, secs_child);
 			if (ret == SGX_NOT_TRACKED) {
 				/*
 				 * Slow path, send IPIs to kick cpus out of the
@@ -330,7 +340,7 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
 				on_each_cpu_mask(sgx_encl_ewb_cpumask(encl),
 						 sgx_ipi_cb, NULL, 1);
 				ret = __sgx_encl_ewb(encl, epc_page, va_page,
-						     va_offset);
+						     va_offset, secs_child);
 			}
 		}
 
@@ -340,12 +350,12 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
 
 		encl_page->desc |= va_offset;
 		encl_page->va_page = va_page;
-	} else if (!do_free) {
+	} else if (secs_child) {
 		ret = __eremove(sgx_epc_addr(epc_page));
 		WARN(ret, "EREMOVE returned %d\n", ret);
 	}
 
-	if (do_free)
+	if (!secs_child)
 		sgx_free_page(epc_page);
 
 	encl_page->epc_page = NULL;
@@ -358,11 +368,11 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page)
 
 	mutex_lock(&encl->lock);
 
-	sgx_encl_ewb(epc_page, false);
+	sgx_encl_ewb(epc_page, true);
 	encl->secs_child_cnt--;
 	if (!encl->secs_child_cnt &&
 	    (encl->flags & (SGX_ENCL_DEAD | SGX_ENCL_INITIALIZED))) {
-		sgx_encl_ewb(encl->secs.epc_page, true);
+		sgx_encl_ewb(encl->secs.epc_page, false);
 	}
 
 	mutex_unlock(&encl->lock);
-- 
2.20.1

Re: [PATCH 1/2] x86/sgx: Remove duplicate check for entry->epc_page in sgx_encl_load_page()

[Sean Christopherson]

On Thu, Aug 22, 2019 at 02:29:01AM +0300, Jarkko Sakkinen wrote:
> The existence of the page is checked first hand for legit race
> conditions (either two or more concurrent threads running the #PF
> handler or the reclaimer has taken over the page):
> 
> /* Page is already resident in the EPC. */
> if (entry->epc_page) {
> 	if (entry->desc & SGX_ENCL_PAGE_RECLAIMED)
> 		return ERR_PTR(-EBUSY);
> 
> 	return entry;
> }
> 
> After that the existence is a checked as a condition for ELDU.
> 
> This commit removes the redundant check.
> 
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---

Ha, I remember seeing this a while back and completely forgot about it.

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

>  arch/x86/kernel/cpu/sgx/encl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index a20d498e9dcd..d6397f7ef3b8 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -123,7 +123,7 @@ static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
>  			return ERR_CAST(epc_page);
>  	}
>  
> -	epc_page = entry->epc_page ? entry->epc_page : sgx_encl_eldu(entry);
> +	epc_page = sgx_encl_eldu(entry);
>  	if (IS_ERR(epc_page))
>  		return ERR_CAST(epc_page);
>  
> -- 
> 2.20.1
> 

Re: [PATCH 2/2] x86/sgx: Determine SECS at compile time in sgx_encl_eldu()

[Sean Christopherson]

On Thu, Aug 22, 2019 at 02:29:02AM +0300, Jarkko Sakkinen wrote:
> Using address resolution of any kind is obviously an overkill for
> anything that we know at compile time. Those sites should not hard
> bind how we store SECS.
> 
> This commit adds @secs_child to sgx_encl_eldu() and sgx_encl_ewb()
> and replaces sgx_encl_get_index() with a macro SGX_ENCL_PAGE_INDEX()

Why use a macro instead of an inline function?

> The new macro assumes that it is operated on SECS children and it is a
> right call because it neither makes sense to bind large architectural
> decisions inside the smallest helpers (e.g. where and how we store SECS).
> 
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c |  4 +--
>  arch/x86/kernel/cpu/sgx/encl.c         | 50 ++++++++++++--------------
>  arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
>  arch/x86/kernel/cpu/sgx/reclaim.c      | 34 +++++++++++-------
>  4 files changed, 48 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 9b784a061a47..fbf2aa9da5fc 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -82,7 +82,7 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req,
>  {
>  	struct sgx_encl_page *encl_page = req->encl_page;
>  	struct sgx_encl *encl = req->encl;
> -	unsigned long page_index = sgx_encl_get_index(encl_page);
> +	unsigned long page_index = SGX_ENCL_PAGE_INDEX(encl_page);
>  	struct sgx_secinfo secinfo;
>  	struct sgx_pageinfo pginfo;
>  	struct page *backing;
> @@ -475,7 +475,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
>  			       struct sgx_secinfo *secinfo,
>  			       unsigned int mrmask)
>  {
> -	unsigned long page_index = sgx_encl_get_index(encl_page);
> +	unsigned long page_index = SGX_ENCL_PAGE_INDEX(encl_page);
>  	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
>  	struct sgx_add_page_req *req = NULL;
>  	struct page *backing;
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index d6397f7ef3b8..16d97198dafb 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -13,18 +13,26 @@
>  #include "sgx.h"
>  
>  static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> -			   struct sgx_epc_page *epc_page)
> +			   struct sgx_epc_page *epc_page, bool secs_child)

secs_child isn't my first choice on names.  I see "secs" and think "this
is an SECS page".  I'd prefer is_secs, or maybe is_child?

>  {
>  	unsigned long va_offset = SGX_ENCL_PAGE_VA_OFFSET(encl_page);
>  	struct sgx_encl *encl = encl_page->encl;
> -	pgoff_t page_index = sgx_encl_get_index(encl_page);
> -	pgoff_t pcmd_index = sgx_pcmd_index(encl, page_index);
> -	unsigned long pcmd_offset = sgx_pcmd_offset(page_index);
>  	struct sgx_pageinfo pginfo;
> +	unsigned long pcmd_offset;
>  	struct page *backing;
> +	pgoff_t page_index;
> +	pgoff_t pcmd_index;
>  	struct page *pcmd;
>  	int ret;
>  
> +	if (secs_child)
> +		page_index = SGX_ENCL_PAGE_INDEX(encl_page);
> +	else
> +		page_index = PFN_DOWN(encl->size);
> +
> +	pcmd_index = sgx_pcmd_index(encl, page_index);
> +	pcmd_offset = sgx_pcmd_offset(page_index);
> +
>  	backing = sgx_encl_get_backing_page(encl, page_index);
>  	if (IS_ERR(backing)) {
>  		ret = PTR_ERR(backing);
> @@ -40,8 +48,11 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
>  	pginfo.addr = SGX_ENCL_PAGE_ADDR(encl_page);
>  	pginfo.contents = (unsigned long)kmap_atomic(backing);
>  	pginfo.metadata = (unsigned long)kmap_atomic(pcmd) + pcmd_offset;
> -	pginfo.secs = SGX_ENCL_PAGE_IS_SECS(encl_page) ? 0 :
> -			(unsigned long)sgx_epc_addr(encl->secs.epc_page);
> +
> +	if (secs_child)
> +		pginfo.secs = (u64)sgx_epc_addr(encl->secs.epc_page);
> +	else
> +		pginfo.secs = 0;

Rather than pass a bool, we can pass 'struct sgx_epc_page *parent_secs'.
That'd avoid having to name a bool, and would clean up this type of code.
In general, I dislike boolean parameters where the caller uses true/false
as I can never remember the context of true/false.

>  
>  	ret = __eldu(&pginfo, sgx_epc_addr(epc_page),
>  		     sgx_epc_addr(encl_page->va_page->epc_page) + va_offset);
> @@ -64,7 +75,8 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
>  	return ret;
>  }
>  

...

> --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> @@ -220,17 +220,26 @@ static void sgx_reclaimer_block(struct sgx_epc_page *epc_page)
>  }
>  
>  static int __sgx_encl_ewb(struct sgx_encl *encl, struct sgx_epc_page *epc_page,
> -			  struct sgx_va_page *va_page, unsigned int va_offset)
> +			  struct sgx_va_page *va_page, unsigned int va_offset,
> +			  bool secs_child)
>  {
>  	struct sgx_encl_page *encl_page = epc_page->owner;
> -	pgoff_t page_index = sgx_encl_get_index(encl_page);
> -	pgoff_t pcmd_index = sgx_pcmd_index(encl, page_index);
> -	unsigned long pcmd_offset = sgx_pcmd_offset(page_index);
>  	struct sgx_pageinfo pginfo;
> +	unsigned long pcmd_offset;
>  	struct page *backing;
> +	pgoff_t page_index;
> +	pgoff_t pcmd_index;
>  	struct page *pcmd;
>  	int ret;
>  
> +	if (secs_child)
> +		page_index = SGX_ENCL_PAGE_INDEX(encl_page);
> +	else
> +		page_index = PFN_DOWN(encl->size);
> +
> +	pcmd_index = sgx_pcmd_index(encl, page_index);
> +	pcmd_offset = sgx_pcmd_offset(page_index);
> +
>  	backing = sgx_encl_get_backing_page(encl, page_index);
>  	if (IS_ERR(backing)) {
>  		ret = PTR_ERR(backing);
> @@ -291,7 +300,7 @@ static const cpumask_t *sgx_encl_ewb_cpumask(struct sgx_encl *encl)
>  	return cpumask;
>  }
>  
> -static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
> +static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool secs_child)

Same comments on the param as ELDU.  The SECS pointer is needed for ETRACK,
so it's not completely extraneous.

>  {
>  	struct sgx_encl_page *encl_page = epc_page->owner;
>  	struct sgx_encl *encl = encl_page->encl;
> @@ -308,7 +317,8 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
>  		if (sgx_va_page_full(va_page))
>  			list_move_tail(&va_page->list, &encl->va_pages);
>  
> -		ret = __sgx_encl_ewb(encl, epc_page, va_page, va_offset);
> +		ret = __sgx_encl_ewb(encl, epc_page, va_page, va_offset,
> +				     secs_child);
>  		if (ret == SGX_NOT_TRACKED) {
>  			ret = __etrack(sgx_epc_addr(encl->secs.epc_page));
>  			if (ret) {
> @@ -318,7 +328,7 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
>  			}
>  
>  			ret = __sgx_encl_ewb(encl, epc_page, va_page,
> -					     va_offset);
> +					     va_offset, secs_child);
>  			if (ret == SGX_NOT_TRACKED) {
>  				/*
>  				 * Slow path, send IPIs to kick cpus out of the
> @@ -330,7 +340,7 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
>  				on_each_cpu_mask(sgx_encl_ewb_cpumask(encl),
>  						 sgx_ipi_cb, NULL, 1);
>  				ret = __sgx_encl_ewb(encl, epc_page, va_page,
> -						     va_offset);
> +						     va_offset, secs_child);
>  			}
>  		}
>  
> @@ -340,12 +350,12 @@ static void sgx_encl_ewb(struct sgx_epc_page *epc_page, bool do_free)
>  
>  		encl_page->desc |= va_offset;
>  		encl_page->va_page = va_page;
> -	} else if (!do_free) {
> +	} else if (secs_child) {
>  		ret = __eremove(sgx_epc_addr(epc_page));
>  		WARN(ret, "EREMOVE returned %d\n", ret);
>  	}
>  
> -	if (do_free)
> +	if (!secs_child)
>  		sgx_free_page(epc_page);
>  
>  	encl_page->epc_page = NULL;
> @@ -358,11 +368,11 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page)
>  
>  	mutex_lock(&encl->lock);
>  
> -	sgx_encl_ewb(epc_page, false);
> +	sgx_encl_ewb(epc_page, true);
>  	encl->secs_child_cnt--;
>  	if (!encl->secs_child_cnt &&
>  	    (encl->flags & (SGX_ENCL_DEAD | SGX_ENCL_INITIALIZED))) {
> -		sgx_encl_ewb(encl->secs.epc_page, true);
> +		sgx_encl_ewb(encl->secs.epc_page, false);
>  	}
>  
>  	mutex_unlock(&encl->lock);
> -- 
> 2.20.1
> 

Re: [PATCH 1/2] x86/sgx: Remove duplicate check for entry->epc_page in sgx_encl_load_page()

[Jarkko Sakkinen]

On Wed, 2019-08-21 at 17:33 -0700, Sean Christopherson wrote:
> On Thu, Aug 22, 2019 at 02:29:01AM +0300, Jarkko Sakkinen wrote:
> > The existence of the page is checked first hand for legit race
> > conditions (either two or more concurrent threads running the #PF
> > handler or the reclaimer has taken over the page):
> > 
> > /* Page is already resident in the EPC. */
> > if (entry->epc_page) {
> > 	if (entry->desc & SGX_ENCL_PAGE_RECLAIMED)
> > 		return ERR_PTR(-EBUSY);
> > 
> > 	return entry;
> > }
> > 
> > After that the existence is a checked as a condition for ELDU.
> > 
> > This commit removes the redundant check.
> > 
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> 
> Ha, I remember seeing this a while back and completely forgot about it.
> 
> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

[I guess for this ack-by would be appropriate as they are not mainline
patches]

Thanks Sean. These kind of regression are almost unavoidable because of
heavy rebasing...

/Jarkko

Re: [PATCH 2/2] x86/sgx: Determine SECS at compile time in sgx_encl_eldu()

[Jarkko Sakkinen]

On Wed, 2019-08-21 at 17:55 -0700, Sean Christopherson wrote:
> On Thu, Aug 22, 2019 at 02:29:02AM +0300, Jarkko Sakkinen wrote:
> > Using address resolution of any kind is obviously an overkill for
> > anything that we know at compile time. Those sites should not hard
> > bind how we store SECS.
> > 
> > This commit adds @secs_child to sgx_encl_eldu() and sgx_encl_ewb()
> > and replaces sgx_encl_get_index() with a macro SGX_ENCL_PAGE_INDEX()
> 
> Why use a macro instead of an inline function?

Just for consistency and nothing else.

It falls into same category as the two other macros in encl.h:
unconditionally give attributes of the page with some calculation
appied (the earlier function had a real flow).

We could consider separately converting all of the three as inline
functions though (not in the context of this patch).

> Rather than pass a bool, we can pass 'struct sgx_epc_page *parent_secs'.
> That'd avoid having to name a bool, and would clean up this type of code.
> In general, I dislike boolean parameters where the caller uses true/false
> as I can never remember the context of true/false.

Sure, but why not just @secs? It is always a parent.

/Jarkko