[PATCH for_v23 0/3] x86/sgx: More cleanup for v23

[PATCH for_v23 0/3] x86/sgx: More cleanup for v23

[Sean Christopherson]

Two small enchancements, and a rework of the sgx_free_page() split that
was sent in a previous series.

Sean Christopherson (3):
  x86/sgx: Update the free page count in a single operation
  x86/sgx: Do not add in-use EPC page to the free page list
  x86/sgx: Move reclaim logic out of sgx_free_page()

 arch/x86/kernel/cpu/sgx/encl.c    |  3 ++-
 arch/x86/kernel/cpu/sgx/main.c    | 36 +++++++++----------------------
 arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h     |  3 ++-
 4 files changed, 46 insertions(+), 28 deletions(-)

-- 
2.22.0

[PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation

[Sean Christopherson]

Use atomic_add() instead of running atomic_inc() in a loop to manually
do the equivalent addition.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 499a9b0740c8..d45bf6fca0c8 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -195,8 +195,7 @@ static bool __init sgx_alloc_epc_section(u64 addr, u64 size,
 		list_add_tail(&page->list, &section->unsanitized_page_list);
 	}
 
-	for (i = 0; i < nr_pages; i++)
-		atomic_inc(&sgx_nr_free_pages);
+	atomic_add(nr_pages, &sgx_nr_free_pages);
 
 	return true;
 
-- 
2.22.0

[PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list

[Sean Christopherson]

Don't add an EPC page to the free page list of EREMOVE fails, as doing
so will cause any future attempt to use the EPC page to fail, and likely
WARN as well.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index d45bf6fca0c8..8e7557d3ff03 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page)
 	spin_unlock(&sgx_active_page_list_lock);
 
 	ret = __eremove(sgx_epc_addr(page));
-	WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret);
+	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
+		return -EIO;
 
 	spin_lock(&section->lock);
 	list_add_tail(&page->list, &section->page_list);
-- 
2.22.0

[PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page()

[Sean Christopherson]

Move the reclaim logic out of sgx_free_page() and into a standalone
helper to avoid taking sgx_active_page_list_lock when the page is known
to be unreclaimable, which is the vast majority of flows that free EPC
pages.

Movig reclaim logic to a separate function also eliminates any
possibility of silently leaking a page because it is unexpectedly
reclaimable (and being actively reclaimed).

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

I really don't like the sgx_unmark_...() name, but couldn't come up with
anything better.  Suggestions welcome...

 arch/x86/kernel/cpu/sgx/encl.c    |  3 ++-
 arch/x86/kernel/cpu/sgx/main.c    | 32 ++++++++-----------------------
 arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h     |  3 ++-
 4 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 8045f1ddfd62..22186d89042a 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -474,9 +474,10 @@ void sgx_encl_destroy(struct sgx_encl *encl)
 			 * The page and its radix tree entry cannot be freed
 			 * if the page is being held by the reclaimer.
 			 */
-			if (sgx_free_page(entry->epc_page))
+			if (sgx_unmark_page_reclaimable(entry->epc_page))
 				continue;
 
+			sgx_free_page(entry->epc_page);
 			encl->secs_child_cnt--;
 			entry->epc_page = NULL;
 		}
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 8e7557d3ff03..cfd8480ef563 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -108,45 +108,29 @@ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim)
  * sgx_free_page() - Free an EPC page
  * @page:	pointer a previously allocated EPC page
  *
- * EREMOVE an EPC page and insert it back to the list of free pages. If the
- * page is reclaimable, delete it from the active page list.
- *
- * Return:
- *   0 on success,
- *   -EBUSY if a reclaim is in progress
+ * EREMOVE an EPC page and insert it back to the list of free pages. The page
+ * must not be reclaimable.
  */
-int sgx_free_page(struct sgx_epc_page *page)
+void sgx_free_page(struct sgx_epc_page *page)
 {
 	struct sgx_epc_section *section = sgx_epc_section(page);
 	int ret;
 
 	/*
-	 * Remove the page from the active list if necessary.  If the page
-	 * is actively being reclaimed, i.e. RECLAIMABLE is set but the
-	 * page isn't on the active list, return -EBUSY as we can't free
-	 * the page at this time since it is "owned" by the reclaimer.
+	 * Don't take sgx_active_page_list_lock when asserting the page isn't
+	 * reclaimable, missing a WARN in the very rare case is preferable to
+	 * unnecessarily taking a global lock in the common case.
 	 */
-	spin_lock(&sgx_active_page_list_lock);
-	if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) {
-		if (list_empty(&page->list)) {
-			spin_unlock(&sgx_active_page_list_lock);
-			return -EBUSY;
-		}
-		list_del(&page->list);
-		page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
-	}
-	spin_unlock(&sgx_active_page_list_lock);
+	WARN_ON_ONCE(page->desc & SGX_EPC_PAGE_RECLAIMABLE);
 
 	ret = __eremove(sgx_epc_addr(page));
 	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
-		return -EIO;
+		return;
 
 	spin_lock(&section->lock);
 	list_add_tail(&page->list, &section->page_list);
 	atomic_inc(&sgx_nr_free_pages);
 	spin_unlock(&section->lock);
-
-	return 0;
 }
 
 static void __init sgx_free_epc_section(struct sgx_epc_section *section)
diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
index 8067ce1915a4..e64c810883ec 100644
--- a/arch/x86/kernel/cpu/sgx/reclaim.c
+++ b/arch/x86/kernel/cpu/sgx/reclaim.c
@@ -125,6 +125,38 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
 	spin_unlock(&sgx_active_page_list_lock);
 }
 
+/**
+ * sgx_unmark_page_reclaimable() - Remove a page from the reclaim list
+ * @page:	EPC page
+ *
+ * Clear the reclaimable flag and remove the page from the active page list.
+ *
+ * Return:
+ *   0 on success,
+ *   -EBUSY if the page is in the process of being reclaimed
+ */
+int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
+{
+	/*
+	 * Remove the page from the active list if necessary.  If the page
+	 * is actively being reclaimed, i.e. RECLAIMABLE is set but the
+	 * page isn't on the active list, return -EBUSY as we can't free
+	 * the page at this time since it is "owned" by the reclaimer.
+	 */
+	spin_lock(&sgx_active_page_list_lock);
+	if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) {
+		if (list_empty(&page->list)) {
+			spin_unlock(&sgx_active_page_list_lock);
+			return -EBUSY;
+		}
+		list_del(&page->list);
+		page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
+	}
+	spin_unlock(&sgx_active_page_list_lock);
+
+	return 0;
+}
+
 static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page)
 {
 	struct sgx_encl_page *page = epc_page->owner;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 45753f236a83..f6d23ef7c74a 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -81,10 +81,11 @@ extern spinlock_t sgx_active_page_list_lock;
 
 bool __init sgx_page_reclaimer_init(void);
 void sgx_mark_page_reclaimable(struct sgx_epc_page *page);
+int sgx_unmark_page_reclaimable(struct sgx_epc_page *page);
 void sgx_reclaim_pages(void);
 
 struct sgx_epc_page *sgx_try_alloc_page(void);
 struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim);
-int sgx_free_page(struct sgx_epc_page *page);
+void sgx_free_page(struct sgx_epc_page *page);
 
 #endif /* _X86_SGX_H */
-- 
2.22.0

Re: [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page()

[Jarkko Sakkinen]

On Tue, Oct 22, 2019 at 03:49:22PM -0700, Sean Christopherson wrote:
> Move the reclaim logic out of sgx_free_page() and into a standalone
> helper to avoid taking sgx_active_page_list_lock when the page is known
> to be unreclaimable, which is the vast majority of flows that free EPC
> pages.
> 
> Movig reclaim logic to a separate function also eliminates any
> possibility of silently leaking a page because it is unexpectedly
> reclaimable (and being actively reclaimed).
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> 
> I really don't like the sgx_unmark_...() name, but couldn't come up with
> anything better.  Suggestions welcome...
> 
>  arch/x86/kernel/cpu/sgx/encl.c    |  3 ++-
>  arch/x86/kernel/cpu/sgx/main.c    | 32 ++++++++-----------------------
>  arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h     |  3 ++-
>  4 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 8045f1ddfd62..22186d89042a 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -474,9 +474,10 @@ void sgx_encl_destroy(struct sgx_encl *encl)
>  			 * The page and its radix tree entry cannot be freed
>  			 * if the page is being held by the reclaimer.
>  			 */
> -			if (sgx_free_page(entry->epc_page))
> +			if (sgx_unmark_page_reclaimable(entry->epc_page))
>  				continue;
>  
> +			sgx_free_page(entry->epc_page);
>  			encl->secs_child_cnt--;
>  			entry->epc_page = NULL;
>  		}
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 8e7557d3ff03..cfd8480ef563 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -108,45 +108,29 @@ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim)
>   * sgx_free_page() - Free an EPC page
>   * @page:	pointer a previously allocated EPC page
>   *
> - * EREMOVE an EPC page and insert it back to the list of free pages. If the
> - * page is reclaimable, delete it from the active page list.
> - *
> - * Return:
> - *   0 on success,
> - *   -EBUSY if a reclaim is in progress
> + * EREMOVE an EPC page and insert it back to the list of free pages. The page
> + * must not be reclaimable.
>   */
> -int sgx_free_page(struct sgx_epc_page *page)
> +void sgx_free_page(struct sgx_epc_page *page)
>  {
>  	struct sgx_epc_section *section = sgx_epc_section(page);
>  	int ret;
>  
>  	/*
> -	 * Remove the page from the active list if necessary.  If the page
> -	 * is actively being reclaimed, i.e. RECLAIMABLE is set but the
> -	 * page isn't on the active list, return -EBUSY as we can't free
> -	 * the page at this time since it is "owned" by the reclaimer.
> +	 * Don't take sgx_active_page_list_lock when asserting the page isn't
> +	 * reclaimable, missing a WARN in the very rare case is preferable to
> +	 * unnecessarily taking a global lock in the common case.
>  	 */
> -	spin_lock(&sgx_active_page_list_lock);
> -	if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) {
> -		if (list_empty(&page->list)) {
> -			spin_unlock(&sgx_active_page_list_lock);
> -			return -EBUSY;
> -		}
> -		list_del(&page->list);
> -		page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> -	}
> -	spin_unlock(&sgx_active_page_list_lock);
> +	WARN_ON_ONCE(page->desc & SGX_EPC_PAGE_RECLAIMABLE);
>  
>  	ret = __eremove(sgx_epc_addr(page));
>  	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> -		return -EIO;
> +		return;
>  
>  	spin_lock(&section->lock);
>  	list_add_tail(&page->list, &section->page_list);
>  	atomic_inc(&sgx_nr_free_pages);
>  	spin_unlock(&section->lock);
> -
> -	return 0;
>  }
>  
>  static void __init sgx_free_epc_section(struct sgx_epc_section *section)
> diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
> index 8067ce1915a4..e64c810883ec 100644
> --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> @@ -125,6 +125,38 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
>  	spin_unlock(&sgx_active_page_list_lock);
>  }
>  
> +/**
> + * sgx_unmark_page_reclaimable() - Remove a page from the reclaim list
> + * @page:	EPC page
> + *
> + * Clear the reclaimable flag and remove the page from the active page list.
> + *
> + * Return:
> + *   0 on success,
> + *   -EBUSY if the page is in the process of being reclaimed
> + */
> +int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
> +{
> +	/*
> +	 * Remove the page from the active list if necessary.  If the page
> +	 * is actively being reclaimed, i.e. RECLAIMABLE is set but the
> +	 * page isn't on the active list, return -EBUSY as we can't free
> +	 * the page at this time since it is "owned" by the reclaimer.
> +	 */
> +	spin_lock(&sgx_active_page_list_lock);
> +	if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) {
> +		if (list_empty(&page->list)) {
> +			spin_unlock(&sgx_active_page_list_lock);
> +			return -EBUSY;
> +		}
> +		list_del(&page->list);
> +		page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> +	}
> +	spin_unlock(&sgx_active_page_list_lock);

Would a WARN_ONCE() make sense when SGX_EPC_PAGE_RECLAIMABLE is not set,
or do we have a legit flow where this can happen?

/Jarkko

Re: [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list

[Jarkko Sakkinen]

On Tue, Oct 22, 2019 at 03:49:21PM -0700, Sean Christopherson wrote:
> Don't add an EPC page to the free page list of EREMOVE fails, as doing
> so will cause any future attempt to use the EPC page to fail, and likely
> WARN as well.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index d45bf6fca0c8..8e7557d3ff03 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page)
>  	spin_unlock(&sgx_active_page_list_lock);
>  
>  	ret = __eremove(sgx_epc_addr(page));
> -	WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret);
> +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> +		return -EIO;

How did you end up choosing -EIO? Don't immediately have any better
suggestion but neither sure if that is the best choice. That is why I'm
asking.

/Jarkko

Re: [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation

[Jarkko Sakkinen]

On Tue, Oct 22, 2019 at 03:49:20PM -0700, Sean Christopherson wrote:
> Use atomic_add() instead of running atomic_inc() in a loop to manually
> do the equivalent addition.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 499a9b0740c8..d45bf6fca0c8 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -195,8 +195,7 @@ static bool __init sgx_alloc_epc_section(u64 addr, u64 size,
>  		list_add_tail(&page->list, &section->unsanitized_page_list);
>  	}
>  
> -	for (i = 0; i < nr_pages; i++)
> -		atomic_inc(&sgx_nr_free_pages);
> +	atomic_add(nr_pages, &sgx_nr_free_pages);
>  
>  	return true;
>  
> -- 
> 2.22.0
> 

There reason I used atomic_inc() was that atomic_add() takes int that
could potentially overflow.

I'll ignore this as I'll do the revert that I promised to do.

/Jarkko

Re: [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page()

[Sean Christopherson]

On Wed, Oct 23, 2019 at 03:42:20PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 22, 2019 at 03:49:22PM -0700, Sean Christopherson wrote:
> > Move the reclaim logic out of sgx_free_page() and into a standalone
> > helper to avoid taking sgx_active_page_list_lock when the page is known
> > to be unreclaimable, which is the vast majority of flows that free EPC
> > pages.
> > 
> > Movig reclaim logic to a separate function also eliminates any
> > possibility of silently leaking a page because it is unexpectedly
> > reclaimable (and being actively reclaimed).
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> > 
> > I really don't like the sgx_unmark_...() name, but couldn't come up with
> > anything better.  Suggestions welcome...
> > 
> >  arch/x86/kernel/cpu/sgx/encl.c    |  3 ++-
> >  arch/x86/kernel/cpu/sgx/main.c    | 32 ++++++++-----------------------
> >  arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++++++
> >  arch/x86/kernel/cpu/sgx/sgx.h     |  3 ++-
> >  4 files changed, 44 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 8045f1ddfd62..22186d89042a 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -474,9 +474,10 @@ void sgx_encl_destroy(struct sgx_encl *encl)
> >  			 * The page and its radix tree entry cannot be freed
> >  			 * if the page is being held by the reclaimer.
> >  			 */
> > -			if (sgx_free_page(entry->epc_page))
> > +			if (sgx_unmark_page_reclaimable(entry->epc_page))
> >  				continue;
> >  
> > +			sgx_free_page(entry->epc_page);
> >  			encl->secs_child_cnt--;
> >  			entry->epc_page = NULL;
> >  		}
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 8e7557d3ff03..cfd8480ef563 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -108,45 +108,29 @@ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim)
> >   * sgx_free_page() - Free an EPC page
> >   * @page:	pointer a previously allocated EPC page
> >   *
> > - * EREMOVE an EPC page and insert it back to the list of free pages. If the
> > - * page is reclaimable, delete it from the active page list.
> > - *
> > - * Return:
> > - *   0 on success,
> > - *   -EBUSY if a reclaim is in progress
> > + * EREMOVE an EPC page and insert it back to the list of free pages. The page
> > + * must not be reclaimable.
> >   */
> > -int sgx_free_page(struct sgx_epc_page *page)
> > +void sgx_free_page(struct sgx_epc_page *page)
> >  {
> >  	struct sgx_epc_section *section = sgx_epc_section(page);
> >  	int ret;
> >  
> >  	/*
> > -	 * Remove the page from the active list if necessary.  If the page
> > -	 * is actively being reclaimed, i.e. RECLAIMABLE is set but the
> > -	 * page isn't on the active list, return -EBUSY as we can't free
> > -	 * the page at this time since it is "owned" by the reclaimer.
> > +	 * Don't take sgx_active_page_list_lock when asserting the page isn't
> > +	 * reclaimable, missing a WARN in the very rare case is preferable to
> > +	 * unnecessarily taking a global lock in the common case.
> >  	 */
> > -	spin_lock(&sgx_active_page_list_lock);
> > -	if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) {
> > -		if (list_empty(&page->list)) {
> > -			spin_unlock(&sgx_active_page_list_lock);
> > -			return -EBUSY;
> > -		}
> > -		list_del(&page->list);
> > -		page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> > -	}
> > -	spin_unlock(&sgx_active_page_list_lock);
> > +	WARN_ON_ONCE(page->desc & SGX_EPC_PAGE_RECLAIMABLE);
> >  
> >  	ret = __eremove(sgx_epc_addr(page));
> >  	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> > -		return -EIO;
> > +		return;
> >  
> >  	spin_lock(&section->lock);
> >  	list_add_tail(&page->list, &section->page_list);
> >  	atomic_inc(&sgx_nr_free_pages);
> >  	spin_unlock(&section->lock);
> > -
> > -	return 0;
> >  }
> >  
> >  static void __init sgx_free_epc_section(struct sgx_epc_section *section)
> > diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
> > index 8067ce1915a4..e64c810883ec 100644
> > --- a/arch/x86/kernel/cpu/sgx/reclaim.c
> > +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> > @@ -125,6 +125,38 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page)
> >  	spin_unlock(&sgx_active_page_list_lock);
> >  }
> >  
> > +/**
> > + * sgx_unmark_page_reclaimable() - Remove a page from the reclaim list
> > + * @page:	EPC page
> > + *
> > + * Clear the reclaimable flag and remove the page from the active page list.
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   -EBUSY if the page is in the process of being reclaimed
> > + */
> > +int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
> > +{
> > +	/*
> > +	 * Remove the page from the active list if necessary.  If the page
> > +	 * is actively being reclaimed, i.e. RECLAIMABLE is set but the
> > +	 * page isn't on the active list, return -EBUSY as we can't free
> > +	 * the page at this time since it is "owned" by the reclaimer.
> > +	 */
> > +	spin_lock(&sgx_active_page_list_lock);
> > +	if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) {
> > +		if (list_empty(&page->list)) {
> > +			spin_unlock(&sgx_active_page_list_lock);
> > +			return -EBUSY;
> > +		}
> > +		list_del(&page->list);
> > +		page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE;
> > +	}
> > +	spin_unlock(&sgx_active_page_list_lock);
> 
> Would a WARN_ONCE() make sense when SGX_EPC_PAGE_RECLAIMABLE is not set,
> or do we have a legit flow where this can happen?

No, there's a legit case.   sgx_reclaim_pages() clears RECLAIMABLE when
it can't get a reference to the enclave, i.e. the enclave is being
released.  That would incorrectly trigger the WARN when sgx_encl_destroy()
frees all pages as part of sgx_encl_release().

Re: [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list

[Sean Christopherson]

On Wed, Oct 23, 2019 at 03:43:40PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 22, 2019 at 03:49:21PM -0700, Sean Christopherson wrote:
> > Don't add an EPC page to the free page list of EREMOVE fails, as doing
> > so will cause any future attempt to use the EPC page to fail, and likely
> > WARN as well.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index d45bf6fca0c8..8e7557d3ff03 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page)
> >  	spin_unlock(&sgx_active_page_list_lock);
> >  
> >  	ret = __eremove(sgx_epc_addr(page));
> > -	WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret);
> > +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> > +		return -EIO;
> 
> How did you end up choosing -EIO? Don't immediately have any better
> suggestion but neither sure if that is the best choice. That is why I'm
> asking.

sgx_edbgrd() and sgx_edbgwr() return -EIO on failure, and they're the only
other case I can think of where an ENCLS instruction failure is reported
to userspace *and* may or may not be due to a fault.

I honestly didn't spend much time thinking about it as the code is dropped
in the next path.

Re: [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation

[Jarkko Sakkinen]

On Wed, Oct 23, 2019 at 03:44:49PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 22, 2019 at 03:49:20PM -0700, Sean Christopherson wrote:
> > Use atomic_add() instead of running atomic_inc() in a loop to manually
> > do the equivalent addition.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/main.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> > index 499a9b0740c8..d45bf6fca0c8 100644
> > --- a/arch/x86/kernel/cpu/sgx/main.c
> > +++ b/arch/x86/kernel/cpu/sgx/main.c
> > @@ -195,8 +195,7 @@ static bool __init sgx_alloc_epc_section(u64 addr, u64 size,
> >  		list_add_tail(&page->list, &section->unsanitized_page_list);
> >  	}
> >  
> > -	for (i = 0; i < nr_pages; i++)
> > -		atomic_inc(&sgx_nr_free_pages);
> > +	atomic_add(nr_pages, &sgx_nr_free_pages);
> >  
> >  	return true;
> >  
> > -- 
> > 2.22.0
> > 
> 
> There reason I used atomic_inc() was that atomic_add() takes int that
> could potentially overflow.
> 
> I'll ignore this as I'll do the revert that I promised to do.

static inline unsigned long sgx_nr_free_pages(void)
{
	unsigned long cnt = 0;
	int i;

	for (i = 0; i < sgx_nr_epc_sections; i++)
		cnt += sgx_epc_sections[i].free_cnt;

	return cnt;
}

static inline bool sgx_should_reclaim(unsigned long watermark)
{
	return sgx_nr_free_pages() < watermark &&
	       !list_empty(&sgx_active_page_list);
}

I use the latter in all call sites.

/Jarkko

Re: [PATCH for_v23 0/3] x86/sgx: More cleanup for v23

[Jarkko Sakkinen]

On Tue, Oct 22, 2019 at 03:49:19PM -0700, Sean Christopherson wrote:
> Two small enchancements, and a rework of the sgx_free_page() split that
> was sent in a previous series.
> 
> Sean Christopherson (3):
>   x86/sgx: Update the free page count in a single operation
>   x86/sgx: Do not add in-use EPC page to the free page list
>   x86/sgx: Move reclaim logic out of sgx_free_page()
> 
>  arch/x86/kernel/cpu/sgx/encl.c    |  3 ++-
>  arch/x86/kernel/cpu/sgx/main.c    | 36 +++++++++----------------------
>  arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h     |  3 ++-
>  4 files changed, 46 insertions(+), 28 deletions(-)
> 
> -- 
> 2.22.0
> 

I think these should be now merged. Always have to do a lot of tweaking
to keep diff's straight with these three commits:

1. x86/sgx: Add functions to allocate and free EPC pages
2. x86/sgx: Linux Enclave Driver
3. x86/sgx: Add a page reclaimer

If something is different the only reason is most likely that the
difference is just side-product of the patch adjustment. I haven't
made my own mods on purpose.

/Jarkko

Re: [PATCH for_v23 0/3] x86/sgx: More cleanup for v23

[Sean Christopherson]

On Thu, Oct 24, 2019 at 09:35:32PM +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 22, 2019 at 03:49:19PM -0700, Sean Christopherson wrote:
> > Two small enchancements, and a rework of the sgx_free_page() split that
> > was sent in a previous series.
> > 
> > Sean Christopherson (3):
> >   x86/sgx: Update the free page count in a single operation
> >   x86/sgx: Do not add in-use EPC page to the free page list
> >   x86/sgx: Move reclaim logic out of sgx_free_page()
> > 
> >  arch/x86/kernel/cpu/sgx/encl.c    |  3 ++-
> >  arch/x86/kernel/cpu/sgx/main.c    | 36 +++++++++----------------------
> >  arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++
> >  arch/x86/kernel/cpu/sgx/sgx.h     |  3 ++-
> >  4 files changed, 46 insertions(+), 28 deletions(-)
> > 
> > -- 
> > 2.22.0
> > 
> 
> I think these should be now merged. Always have to do a lot of tweaking
> to keep diff's straight with these three commits:
> 
> 1. x86/sgx: Add functions to allocate and free EPC pages
> 2. x86/sgx: Linux Enclave Driver
> 3. x86/sgx: Add a page reclaimer
> 
> If something is different the only reason is most likely that the
> difference is just side-product of the patch adjustment. I haven't
> made my own mods on purpose.

Diff looks good and smoke test ran clean.

Re: [PATCH for_v23 0/3] x86/sgx: More cleanup for v23

[Jarkko Sakkinen]

On Thu, Oct 24, 2019 at 01:22:22PM -0700, Sean Christopherson wrote:
> On Thu, Oct 24, 2019 at 09:35:32PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 22, 2019 at 03:49:19PM -0700, Sean Christopherson wrote:
> > > Two small enchancements, and a rework of the sgx_free_page() split that
> > > was sent in a previous series.
> > > 
> > > Sean Christopherson (3):
> > >   x86/sgx: Update the free page count in a single operation
> > >   x86/sgx: Do not add in-use EPC page to the free page list
> > >   x86/sgx: Move reclaim logic out of sgx_free_page()
> > > 
> > >  arch/x86/kernel/cpu/sgx/encl.c    |  3 ++-
> > >  arch/x86/kernel/cpu/sgx/main.c    | 36 +++++++++----------------------
> > >  arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++
> > >  arch/x86/kernel/cpu/sgx/sgx.h     |  3 ++-
> > >  4 files changed, 46 insertions(+), 28 deletions(-)
> > > 
> > > -- 
> > > 2.22.0
> > > 
> > 
> > I think these should be now merged. Always have to do a lot of tweaking
> > to keep diff's straight with these three commits:
> > 
> > 1. x86/sgx: Add functions to allocate and free EPC pages
> > 2. x86/sgx: Linux Enclave Driver
> > 3. x86/sgx: Add a page reclaimer
> > 
> > If something is different the only reason is most likely that the
> > difference is just side-product of the patch adjustment. I haven't
> > made my own mods on purpose.
> 
> Diff looks good and smoke test ran clean.

Great!

/Jarkko