[intel-sgx-kernel-dev,6/?] intel_sgx: move sgx_ewb call into sgx_evict_page
diff mbox

Message ID 1486487114-2785-2-git-send-email-sean.j.christopherson@intel.com
State New
Headers show

Commit Message

Sean Christopherson Feb. 7, 2017, 5:05 p.m. UTC
Move the call to sgx_ewb() into sgx_evict_page(), and always perform
EBLOCK->ETRACK-EWB for pages that are being evicted even if the VMA
for the page cannot be found.  Killing an enclave due to VMA closure
is a lazy operation, i.e. an enclave may contain active threads even
after a VMA is closed.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 drivers/platform/x86/intel_sgx_page_cache.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

Comments

Jarkko Sakkinen Feb. 17, 2017, 11:52 a.m. UTC | #1
On Tue, Feb 07, 2017 at 09:05:14AM -0800, Sean Christopherson wrote:
> Move the call to sgx_ewb() into sgx_evict_page(), and always perform
> EBLOCK->ETRACK-EWB for pages that are being evicted even if the VMA
> for the page cannot be found.  Killing an enclave due to VMA closure
> is a lazy operation, i.e. an enclave may contain active threads even
> after a VMA is closed.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx_page_cache.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> index 72f8ef1..8224729 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -310,6 +310,7 @@ static void sgx_ewb(struct sgx_encl *encl,
>  static void sgx_evict_page(struct sgx_encl_page *entry,
>  			   struct sgx_encl *encl)
>  {
> +	sgx_ewb(encl, entry);
>  	sgx_free_page(entry->epc_page, encl);
>  	entry->epc_page = NULL;
>  	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
> @@ -319,7 +320,7 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  {
>  	struct sgx_encl_page *entry;
>  	struct sgx_encl_page *tmp;
> -	struct vm_area_struct *evma;
> +	struct vm_area_struct *vma;
>  
>  	if (list_empty(src))
>  		return;
> @@ -330,14 +331,11 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  
>  	/* EBLOCK */
>  	list_for_each_entry_safe(entry, tmp, src, load_list) {
> -		evma = sgx_find_vma(encl, entry->addr);
> -		if (!evma) {
> -			list_del(&entry->load_list);
> -			sgx_evict_page(entry, encl);
> -			continue;
> +		vma = sgx_find_vma(encl, entry->addr);
> +		if (vma) {
> +			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
>  		}
>  
> -		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);
>  		sgx_eblock(encl, entry->epc_page);
>  	}
>  
> @@ -350,20 +348,14 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  					 load_list);
>  		list_del(&entry->load_list);
>  
> -
> -		evma = sgx_find_vma(encl, entry->addr);
> -		if (evma) {
> -			sgx_ewb(encl, entry);
> -			encl->secs_child_cnt--;
> -		}
> -
>  		sgx_evict_page(entry, encl);
> +
> +		encl->secs_child_cnt--;
>  	}
>  
>  	if (!encl->secs_child_cnt && (encl->flags & SGX_ENCL_INITIALIZED)) {
> -		sgx_ewb(encl, &encl->secs_page);
> -		encl->flags |= SGX_ENCL_SECS_EVICTED;
>  		sgx_evict_page(&encl->secs_page, encl);
> +		encl->flags |= SGX_ENCL_SECS_EVICTED;
>  	}
>  
>  	mutex_unlock(&encl->lock);
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Jarkko Sakkinen Feb. 20, 2017, 12:42 p.m. UTC | #2
On Tue, Feb 07, 2017 at 09:05:14AM -0800, Sean Christopherson wrote:
> Move the call to sgx_ewb() into sgx_evict_page(), and always perform
> EBLOCK->ETRACK-EWB for pages that are being evicted even if the VMA
> for the page cannot be found.  Killing an enclave due to VMA closure
> is a lazy operation, i.e. an enclave may contain active threads even
> after a VMA is closed.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Doesn't apply.

/Jarkko

> ---
>  drivers/platform/x86/intel_sgx_page_cache.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> index 72f8ef1..8224729 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -310,6 +310,7 @@ static void sgx_ewb(struct sgx_encl *encl,
>  static void sgx_evict_page(struct sgx_encl_page *entry,
>  			   struct sgx_encl *encl)
>  {
> +	sgx_ewb(encl, entry);
>  	sgx_free_page(entry->epc_page, encl);
>  	entry->epc_page = NULL;
>  	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
> @@ -319,7 +320,7 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  {
>  	struct sgx_encl_page *entry;
>  	struct sgx_encl_page *tmp;
> -	struct vm_area_struct *evma;
> +	struct vm_area_struct *vma;
>  
>  	if (list_empty(src))
>  		return;
> @@ -330,14 +331,11 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  
>  	/* EBLOCK */
>  	list_for_each_entry_safe(entry, tmp, src, load_list) {
> -		evma = sgx_find_vma(encl, entry->addr);
> -		if (!evma) {
> -			list_del(&entry->load_list);
> -			sgx_evict_page(entry, encl);
> -			continue;
> +		vma = sgx_find_vma(encl, entry->addr);
> +		if (vma) {
> +			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
>  		}
>  
> -		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);
>  		sgx_eblock(encl, entry->epc_page);
>  	}
>  
> @@ -350,20 +348,14 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  					 load_list);
>  		list_del(&entry->load_list);
>  
> -
> -		evma = sgx_find_vma(encl, entry->addr);
> -		if (evma) {
> -			sgx_ewb(encl, entry);
> -			encl->secs_child_cnt--;
> -		}
> -
>  		sgx_evict_page(entry, encl);
> +
> +		encl->secs_child_cnt--;
>  	}
>  
>  	if (!encl->secs_child_cnt && (encl->flags & SGX_ENCL_INITIALIZED)) {
> -		sgx_ewb(encl, &encl->secs_page);
> -		encl->flags |= SGX_ENCL_SECS_EVICTED;
>  		sgx_evict_page(&encl->secs_page, encl);
> +		encl->flags |= SGX_ENCL_SECS_EVICTED;
>  	}
>  
>  	mutex_unlock(&encl->lock);
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Jarkko Sakkinen March 6, 2017, 3:06 p.m. UTC | #3
On Tue, Feb 07, 2017 at 09:05:14AM -0800, Sean Christopherson wrote:
> Move the call to sgx_ewb() into sgx_evict_page(), and always perform
> EBLOCK->ETRACK-EWB for pages that are being evicted even if the VMA
> for the page cannot be found.  Killing an enclave due to VMA closure
> is a lazy operation, i.e. an enclave may contain active threads even
> after a VMA is closed.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I found an issue when I started to merge this one.

> ---
>  drivers/platform/x86/intel_sgx_page_cache.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> index 72f8ef1..8224729 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -310,6 +310,7 @@ static void sgx_ewb(struct sgx_encl *encl,
>  static void sgx_evict_page(struct sgx_encl_page *entry,
>  			   struct sgx_encl *encl)
>  {
> +	sgx_ewb(encl, entry);
>  	sgx_free_page(entry->epc_page, encl);
>  	entry->epc_page = NULL;
>  	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
> @@ -319,7 +320,7 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  {
>  	struct sgx_encl_page *entry;
>  	struct sgx_encl_page *tmp;
> -	struct vm_area_struct *evma;
> +	struct vm_area_struct *vma;
>  
>  	if (list_empty(src))
>  		return;
> @@ -330,14 +331,11 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  
>  	/* EBLOCK */
>  	list_for_each_entry_safe(entry, tmp, src, load_list) {
> -		evma = sgx_find_vma(encl, entry->addr);
> -		if (!evma) {
> -			list_del(&entry->load_list);
> -			sgx_evict_page(entry, encl);
> -			continue;
> +		vma = sgx_find_vma(encl, entry->addr);
> +		if (vma) {
> +			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
>  		}
>  
> -		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);

We want the pages to be freed even if the VMA does not exist.

/Jarkko
Sean Christopherson March 13, 2017, 6:07 p.m. UTC | #4
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> On Tue, Feb 07, 2017 at 09:05:14AM -0800, Sean Christopherson wrote:
> > Move the call to sgx_ewb() into sgx_evict_page(), and always perform
> > EBLOCK->ETRACK-EWB for pages that are being evicted even if the VMA
> > for the page cannot be found.  Killing an enclave due to VMA closure
> > is a lazy operation, i.e. an enclave may contain active threads even
> > after a VMA is closed.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I found an issue when I started to merge this one.
> 
> > ---
> >  drivers/platform/x86/intel_sgx_page_cache.c | 24 ++++++++----------------
> >  1 file changed, 8 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c
> > b/drivers/platform/x86/intel_sgx_page_cache.c index 72f8ef1..8224729 100644
> > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > @@ -310,6 +310,7 @@ static void sgx_ewb(struct sgx_encl *encl,
> >  static void sgx_evict_page(struct sgx_encl_page *entry,
> >  			   struct sgx_encl *encl)
> >  {
> > +	sgx_ewb(encl, entry);
> >  	sgx_free_page(entry->epc_page, encl);
> >  	entry->epc_page = NULL;
> >  	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
> > @@ -319,7 +320,7 @@ static void sgx_write_pages(struct sgx_encl *encl,
> > struct list_head *src) {
> >  	struct sgx_encl_page *entry;
> >  	struct sgx_encl_page *tmp;
> > -	struct vm_area_struct *evma;
> > +	struct vm_area_struct *vma;
> >  
> >  	if (list_empty(src))
> >  		return;
> > @@ -330,14 +331,11 @@ static void sgx_write_pages(struct sgx_encl *encl,
> > struct list_head *src) 
> >  	/* EBLOCK */
> >  	list_for_each_entry_safe(entry, tmp, src, load_list) {
> > -		evma = sgx_find_vma(encl, entry->addr);
> > -		if (!evma) {
> > -			list_del(&entry->load_list);
> > -			sgx_evict_page(entry, encl);
> > -			continue;
> > +		vma = sgx_find_vma(encl, entry->addr);
> > +		if (vma) {
> > +			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
> >  		}
> >  
> > -		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);
> 
> We want the pages to be freed even if the VMA does not exist.
> 
> /Jarkko

Pages without a VMA are still freed, the only functional change in this patch
is that pages without a VMA now go through EBLOCK and ETRACK, i.e. the entries
are left in the list and later processed in the common EWB loop.
Jarkko Sakkinen March 15, 2017, 8:25 a.m. UTC | #5
On Mon, Mar 13, 2017 at 06:07:20PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > On Tue, Feb 07, 2017 at 09:05:14AM -0800, Sean Christopherson wrote:
> > > Move the call to sgx_ewb() into sgx_evict_page(), and always perform
> > > EBLOCK->ETRACK-EWB for pages that are being evicted even if the VMA
> > > for the page cannot be found.  Killing an enclave due to VMA closure
> > > is a lazy operation, i.e. an enclave may contain active threads even
> > > after a VMA is closed.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > I found an issue when I started to merge this one.
> > 
> > > ---
> > >  drivers/platform/x86/intel_sgx_page_cache.c | 24 ++++++++----------------
> > >  1 file changed, 8 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c
> > > b/drivers/platform/x86/intel_sgx_page_cache.c index 72f8ef1..8224729 100644
> > > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > > @@ -310,6 +310,7 @@ static void sgx_ewb(struct sgx_encl *encl,
> > >  static void sgx_evict_page(struct sgx_encl_page *entry,
> > >  			   struct sgx_encl *encl)
> > >  {
> > > +	sgx_ewb(encl, entry);
> > >  	sgx_free_page(entry->epc_page, encl);
> > >  	entry->epc_page = NULL;
> > >  	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
> > > @@ -319,7 +320,7 @@ static void sgx_write_pages(struct sgx_encl *encl,
> > > struct list_head *src) {
> > >  	struct sgx_encl_page *entry;
> > >  	struct sgx_encl_page *tmp;
> > > -	struct vm_area_struct *evma;
> > > +	struct vm_area_struct *vma;
> > >  
> > >  	if (list_empty(src))
> > >  		return;
> > > @@ -330,14 +331,11 @@ static void sgx_write_pages(struct sgx_encl *encl,
> > > struct list_head *src) 
> > >  	/* EBLOCK */
> > >  	list_for_each_entry_safe(entry, tmp, src, load_list) {
> > > -		evma = sgx_find_vma(encl, entry->addr);
> > > -		if (!evma) {
> > > -			list_del(&entry->load_list);
> > > -			sgx_evict_page(entry, encl);
> > > -			continue;
> > > +		vma = sgx_find_vma(encl, entry->addr);
> > > +		if (vma) {
> > > +			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
> > >  		}
> > >  
> > > -		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);
> > 
> > We want the pages to be freed even if the VMA does not exist.
> > 
> > /Jarkko
> 
> Pages without a VMA are still freed, the only functional change in this patch
> is that pages without a VMA now go through EBLOCK and ETRACK, i.e. the entries
> are left in the list and later processed in the common EWB loop.

Right, I see.

The commit message was just very misleading (emphasizing clean up
instead of this).

I'll eventually merge this commit. Thank you.

/Jarkko
Jarkko Sakkinen March 15, 2017, 7:50 p.m. UTC | #6
On Wed, Mar 15, 2017 at 10:25:52AM +0200, Jarkko Sakkinen wrote:
> On Mon, Mar 13, 2017 at 06:07:20PM +0000, Christopherson, Sean J wrote:
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > On Tue, Feb 07, 2017 at 09:05:14AM -0800, Sean Christopherson wrote:
> > > > Move the call to sgx_ewb() into sgx_evict_page(), and always perform
> > > > EBLOCK->ETRACK-EWB for pages that are being evicted even if the VMA
> > > > for the page cannot be found.  Killing an enclave due to VMA closure
> > > > is a lazy operation, i.e. an enclave may contain active threads even
> > > > after a VMA is closed.
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > 
> > > I found an issue when I started to merge this one.
> > > 
> > > > ---
> > > >  drivers/platform/x86/intel_sgx_page_cache.c | 24 ++++++++----------------
> > > >  1 file changed, 8 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c
> > > > b/drivers/platform/x86/intel_sgx_page_cache.c index 72f8ef1..8224729 100644
> > > > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > > > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > > > @@ -310,6 +310,7 @@ static void sgx_ewb(struct sgx_encl *encl,
> > > >  static void sgx_evict_page(struct sgx_encl_page *entry,
> > > >  			   struct sgx_encl *encl)
> > > >  {
> > > > +	sgx_ewb(encl, entry);
> > > >  	sgx_free_page(entry->epc_page, encl);
> > > >  	entry->epc_page = NULL;
> > > >  	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
> > > > @@ -319,7 +320,7 @@ static void sgx_write_pages(struct sgx_encl *encl,
> > > > struct list_head *src) {
> > > >  	struct sgx_encl_page *entry;
> > > >  	struct sgx_encl_page *tmp;
> > > > -	struct vm_area_struct *evma;
> > > > +	struct vm_area_struct *vma;
> > > >  
> > > >  	if (list_empty(src))
> > > >  		return;
> > > > @@ -330,14 +331,11 @@ static void sgx_write_pages(struct sgx_encl *encl,
> > > > struct list_head *src) 
> > > >  	/* EBLOCK */
> > > >  	list_for_each_entry_safe(entry, tmp, src, load_list) {
> > > > -		evma = sgx_find_vma(encl, entry->addr);
> > > > -		if (!evma) {
> > > > -			list_del(&entry->load_list);
> > > > -			sgx_evict_page(entry, encl);
> > > > -			continue;
> > > > +		vma = sgx_find_vma(encl, entry->addr);
> > > > +		if (vma) {
> > > > +			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
> > > >  		}
> > > >  
> > > > -		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);
> > > 
> > > We want the pages to be freed even if the VMA does not exist.
> > > 
> > > /Jarkko
> > 
> > Pages without a VMA are still freed, the only functional change in this patch
> > is that pages without a VMA now go through EBLOCK and ETRACK, i.e. the entries
> > are left in the list and later processed in the common EWB loop.
> 
> Right, I see.
> 
> The commit message was just very misleading (emphasizing clean up
> instead of this).
> 
> I'll eventually merge this commit. Thank you.
> 
> /Jarkko

It's now merged. Thank you.

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
index 72f8ef1..8224729 100644
--- a/drivers/platform/x86/intel_sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx_page_cache.c
@@ -310,6 +310,7 @@  static void sgx_ewb(struct sgx_encl *encl,
 static void sgx_evict_page(struct sgx_encl_page *entry,
 			   struct sgx_encl *encl)
 {
+	sgx_ewb(encl, entry);
 	sgx_free_page(entry->epc_page, encl);
 	entry->epc_page = NULL;
 	entry->flags &= ~SGX_ENCL_PAGE_RESERVED;
@@ -319,7 +320,7 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 {
 	struct sgx_encl_page *entry;
 	struct sgx_encl_page *tmp;
-	struct vm_area_struct *evma;
+	struct vm_area_struct *vma;
 
 	if (list_empty(src))
 		return;
@@ -330,14 +331,11 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 
 	/* EBLOCK */
 	list_for_each_entry_safe(entry, tmp, src, load_list) {
-		evma = sgx_find_vma(encl, entry->addr);
-		if (!evma) {
-			list_del(&entry->load_list);
-			sgx_evict_page(entry, encl);
-			continue;
+		vma = sgx_find_vma(encl, entry->addr);
+		if (vma) {
+			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
 		}
 
-		zap_vma_ptes(evma, entry->addr, PAGE_SIZE);
 		sgx_eblock(encl, entry->epc_page);
 	}
 
@@ -350,20 +348,14 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 					 load_list);
 		list_del(&entry->load_list);
 
-
-		evma = sgx_find_vma(encl, entry->addr);
-		if (evma) {
-			sgx_ewb(encl, entry);
-			encl->secs_child_cnt--;
-		}
-
 		sgx_evict_page(entry, encl);
+
+		encl->secs_child_cnt--;
 	}
 
 	if (!encl->secs_child_cnt && (encl->flags & SGX_ENCL_INITIALIZED)) {
-		sgx_ewb(encl, &encl->secs_page);
-		encl->flags |= SGX_ENCL_SECS_EVICTED;
 		sgx_evict_page(&encl->secs_page, encl);
+		encl->flags |= SGX_ENCL_SECS_EVICTED;
 	}
 
 	mutex_unlock(&encl->lock);