[intel-sgx-kernel-dev,RFC,01/12] intel_sgx: reuse sgx_epc_page list for loaded pages
diff mbox

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

Commit Message

Sean Christopherson June 14, 2017, 5:37 p.m. UTC
Use struct sgx_epc_page's list_head member to track an EPC page in
both its "free" and "loaded" states.  Add a back-pointer to the EPC
page to associate a loaded EPC page with its enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/sgx.h                      |  3 +-
 drivers/platform/x86/intel_sgx/sgx.h            |  1 -
 drivers/platform/x86/intel_sgx/sgx_ioctl.c      |  3 +-
 drivers/platform/x86/intel_sgx/sgx_page_cache.c | 53 ++++++++++++-------------
 drivers/platform/x86/intel_sgx/sgx_util.c       |  9 +++--
 5 files changed, 36 insertions(+), 33 deletions(-)

Comments

Jarkko Sakkinen June 16, 2017, 8:11 a.m. UTC | #1
Sean,

On Wed, Jun 14, 2017 at 10:37:27AM -0700, Sean Christopherson wrote:
> Use struct sgx_epc_page's list_head member to track an EPC page in
> both its "free" and "loaded" states.  Add a back-pointer to the EPC
> page to associate a loaded EPC page with its enclave.

And you are making this change so that we can backtrack EPC pages from
the global list, am I correct (the commit message explains only action
not the rationale for doing the change)?

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

I might squash this to my first upstream patch set if you don't mind.

> ---
>  arch/x86/include/asm/sgx.h                      |  3 +-
>  drivers/platform/x86/intel_sgx/sgx.h            |  1 -
>  drivers/platform/x86/intel_sgx/sgx_ioctl.c      |  3 +-
>  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 53 ++++++++++++-------------
>  drivers/platform/x86/intel_sgx/sgx_util.c       |  9 +++--
>  5 files changed, 36 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 31bce3f..e1f27fd 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -343,7 +343,8 @@ struct sgx_encl;
>  
>  struct sgx_epc_page {
>  	resource_size_t	pa;
> -	struct list_head free_list;
> +	struct list_head list;
> +	struct sgx_encl_page *encl_page;

It would have been good mention also in the commit message that memory
is saved as a whole even if a new field added.

There's enough rationale to squash this even without the cgroup context
but the commit message is lacking *all of it* :-)

>  };
>  
>  enum sgx_alloc_flags {
> diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
> index d5cdf96..78d048e 100644
> --- a/drivers/platform/x86/intel_sgx/sgx.h
> +++ b/drivers/platform/x86/intel_sgx/sgx.h
> @@ -107,7 +107,6 @@ struct sgx_encl_page {
>  	unsigned long addr;
>  	unsigned int flags;
>  	struct sgx_epc_page *epc_page;
> -	struct list_head load_list;

This is where the memory savings comes from.

>  	struct sgx_va_page *va_page;
>  	unsigned int va_offset;
>  };
> diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> index 70d7417..0741e6c 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> @@ -267,9 +267,10 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
>  		goto out;
>  	}
>  
> +	epc_page->encl_page = encl_page;
>  	encl_page->epc_page = epc_page;
>  	sgx_test_and_clear_young(encl_page, encl);
> -	list_add_tail(&encl_page->load_list, &encl->load_list);
> +	list_add_tail(&epc_page->list, &encl->load_list);
>  
>  	mutex_unlock(&encl->lock);
>  	up_read(&encl->mm->mmap_sem);
> diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> index 8e01427..0829ee0 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> @@ -193,7 +193,7 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
>  			      struct list_head *dst,
>  			      unsigned long nr_to_scan)
>  {
> -	struct sgx_encl_page *entry;
> +	struct sgx_epc_page *entry;
>  	int i;
>  
>  	mutex_lock(&encl->lock);
> @@ -206,15 +206,15 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
>  			break;
>  
>  		entry = list_first_entry(&encl->load_list,
> -					 struct sgx_encl_page,
> -					 load_list);
> +					 struct sgx_epc_page,
> +					 list);
>  
> -		if (!sgx_test_and_clear_young(entry, encl) &&
> -		    !(entry->flags & SGX_ENCL_PAGE_RESERVED)) {
> -			entry->flags |= SGX_ENCL_PAGE_RESERVED;
> -			list_move_tail(&entry->load_list, dst);
> +		if (!sgx_test_and_clear_young(entry->encl_page, encl) &&
> +		    !(entry->encl_page->flags & SGX_ENCL_PAGE_RESERVED)) {
> +			entry->encl_page->flags |= SGX_ENCL_PAGE_RESERVED;
> +			list_move_tail(&entry->list, dst);
>  		} else {
> -			list_move_tail(&entry->load_list, &encl->load_list);
> +			list_move_tail(&entry->list, &encl->load_list);
>  		}
>  	}
>  out:
> @@ -304,25 +304,25 @@ static void sgx_evict_page(struct sgx_encl_page *entry,
>  
>  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  {
> -	struct sgx_encl_page *entry;
> -	struct sgx_encl_page *tmp;
> +	struct sgx_epc_page *entry;
> +	struct sgx_epc_page *tmp;
>  	struct vm_area_struct *vma;
>  
>  	if (list_empty(src))
>  		return;
>  
> -	entry = list_first_entry(src, struct sgx_encl_page, load_list);
> +	entry = list_first_entry(src, struct sgx_epc_page, list);
>  
>  	mutex_lock(&encl->lock);
>  
>  	/* EBLOCK */
> -	list_for_each_entry_safe(entry, tmp, src, load_list) {
> -		vma = sgx_find_vma(encl, entry->addr);
> +	list_for_each_entry_safe(entry, tmp, src, list) {
> +		vma = sgx_find_vma(encl, entry->encl_page->addr);
>  		if (vma) {
> -			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
> +			zap_vma_ptes(vma, entry->encl_page->addr, PAGE_SIZE);
>  		}
>  
> -		sgx_eblock(encl, entry->epc_page);
> +		sgx_eblock(encl, entry);
>  	}
>  
>  	/* ETRACK */
> @@ -330,10 +330,9 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
>  
>  	/* EWB */
>  	while (!list_empty(src)) {
> -		entry = list_first_entry(src, struct sgx_encl_page,
> -					 load_list);
> -		list_del(&entry->load_list);
> -		sgx_evict_page(entry, encl);
> +		entry = list_first_entry(src, struct sgx_epc_page, list);
> +		list_del(&entry->list);
> +		sgx_evict_page(entry->encl_page, encl);
>  		encl->secs_child_cnt--;
>  	}
>  
> @@ -397,7 +396,7 @@ int sgx_add_epc_bank(resource_size_t start, unsigned long size)
>  		new_epc_page->pa = start + i;
>  
>  		spin_lock(&sgx_free_list_lock);
> -		list_add_tail(&new_epc_page->free_list, &sgx_free_list);
> +		list_add_tail(&new_epc_page->list, &sgx_free_list);
>  		sgx_nr_total_epc_pages++;
>  		sgx_nr_free_pages++;
>  		spin_unlock(&sgx_free_list_lock);
> @@ -407,8 +406,8 @@ int sgx_add_epc_bank(resource_size_t start, unsigned long size)
>  err_freelist:
>  	list_for_each_safe(parser, temp, &sgx_free_list) {
>  		spin_lock(&sgx_free_list_lock);
> -		entry = list_entry(parser, struct sgx_epc_page, free_list);
> -		list_del(&entry->free_list);
> +		entry = list_entry(parser, struct sgx_epc_page, list);
> +		list_del(&entry->list);
>  		spin_unlock(&sgx_free_list_lock);
>  		kfree(entry);
>  	}
> @@ -432,8 +431,8 @@ void sgx_page_cache_teardown(void)
>  
>  	spin_lock(&sgx_free_list_lock);
>  	list_for_each_safe(parser, temp, &sgx_free_list) {
> -		entry = list_entry(parser, struct sgx_epc_page, free_list);
> -		list_del(&entry->free_list);
> +		entry = list_entry(parser, struct sgx_epc_page, list);
> +		list_del(&entry->list);
>  		kfree(entry);
>  	}
>  	spin_unlock(&sgx_free_list_lock);
> @@ -447,8 +446,8 @@ static struct sgx_epc_page *sgx_alloc_page_fast(void)
>  
>  	if (!list_empty(&sgx_free_list)) {
>  		entry = list_first_entry(&sgx_free_list, struct sgx_epc_page,
> -					 free_list);
> -		list_del(&entry->free_list);
> +					 list);
> +		list_del(&entry->list);
>  		sgx_nr_free_pages--;
>  	}
>  
> @@ -533,7 +532,7 @@ int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl)
>  	}
>  
>  	spin_lock(&sgx_free_list_lock);
> -	list_add(&entry->free_list, &sgx_free_list);
> +	list_add(&entry->list, &sgx_free_list);
>  	sgx_nr_free_pages++;
>  	spin_unlock(&sgx_free_list_lock);
>  
> diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
> index a8a954a..021e789 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_util.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_util.c
> @@ -110,9 +110,11 @@ struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr)
>  
>  void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma)
>  {
> +	struct sgx_epc_page *tmp;
>  	struct sgx_encl_page *entry;
>  
> -	list_for_each_entry(entry, &encl->load_list, load_list) {
> +	list_for_each_entry(tmp, &encl->load_list, list) {
> +		entry = tmp->encl_page;
>  		if ((entry->flags & SGX_ENCL_PAGE_TCS) &&
>  		    entry->addr >= vma->vm_start &&
>  		    entry->addr < vma->vm_end)
> @@ -335,6 +337,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  	 */
>  	encl->secs_child_cnt++;
>  
> +	epc_page->encl_page = entry;
>  	entry->epc_page = epc_page;
>  
>  	if (reserve)
> @@ -342,7 +345,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
>  
>  	/* Do not free */
>  	epc_page = NULL;
> -	list_add_tail(&entry->load_list, &encl->load_list);
> +	list_add_tail(&entry->epc_page->list, &encl->load_list);
>  
>  	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa));
>  	if (rc) {
> @@ -399,7 +402,7 @@ void sgx_encl_release(struct kref *ref)
>  	radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
>  		entry = *slot;
>  		if (entry->epc_page) {
> -			list_del(&entry->load_list);
> +			list_del(&entry->epc_page->list);
>  			sgx_free_page(entry->epc_page, encl);
>  		}
>  		radix_tree_delete(&encl->page_tree, entry->addr >> PAGE_SHIFT);
> -- 
> 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

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

... but please try to explain rationale especially in the patches like
this that do not radically change things but still give a lot of value.
If you do that, I can more quickly apply them to my tree :-)

/Jarkko
Jarkko Sakkinen June 16, 2017, 8:30 a.m. UTC | #2
On Fri, Jun 16, 2017 at 10:11:44AM +0200, Jarkko Sakkinen wrote:
> Sean,
> 
> On Wed, Jun 14, 2017 at 10:37:27AM -0700, Sean Christopherson wrote:
> > Use struct sgx_epc_page's list_head member to track an EPC page in
> > both its "free" and "loaded" states.  Add a back-pointer to the EPC
> > page to associate a loaded EPC page with its enclave.
> 
> And you are making this change so that we can backtrack EPC pages from
> the global list, am I correct (the commit message explains only action
> not the rationale for doing the change)?
> 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> I might squash this to my first upstream patch set if you don't mind.
> 
> > ---
> >  arch/x86/include/asm/sgx.h                      |  3 +-
> >  drivers/platform/x86/intel_sgx/sgx.h            |  1 -
> >  drivers/platform/x86/intel_sgx/sgx_ioctl.c      |  3 +-
> >  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 53 ++++++++++++-------------
> >  drivers/platform/x86/intel_sgx/sgx_util.c       |  9 +++--
> >  5 files changed, 36 insertions(+), 33 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > index 31bce3f..e1f27fd 100644
> > --- a/arch/x86/include/asm/sgx.h
> > +++ b/arch/x86/include/asm/sgx.h
> > @@ -343,7 +343,8 @@ struct sgx_encl;
> >  
> >  struct sgx_epc_page {
> >  	resource_size_t	pa;
> > -	struct list_head free_list;
> > +	struct list_head list;
> > +	struct sgx_encl_page *encl_page;
> 
> It would have been good mention also in the commit message that memory
> is saved as a whole even if a new field added.
> 
> There's enough rationale to squash this even without the cgroup context
> but the commit message is lacking *all of it* :-)
> 
> >  };
> >  
> >  enum sgx_alloc_flags {
> > diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
> > index d5cdf96..78d048e 100644
> > --- a/drivers/platform/x86/intel_sgx/sgx.h
> > +++ b/drivers/platform/x86/intel_sgx/sgx.h
> > @@ -107,7 +107,6 @@ struct sgx_encl_page {
> >  	unsigned long addr;
> >  	unsigned int flags;
> >  	struct sgx_epc_page *epc_page;
> > -	struct list_head load_list;
> 
> This is where the memory savings comes from.
> 
> >  	struct sgx_va_page *va_page;
> >  	unsigned int va_offset;
> >  };
> > diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> > index 70d7417..0741e6c 100644
> > --- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> > +++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> > @@ -267,9 +267,10 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
> >  		goto out;
> >  	}
> >  
> > +	epc_page->encl_page = encl_page;
> >  	encl_page->epc_page = epc_page;
> >  	sgx_test_and_clear_young(encl_page, encl);
> > -	list_add_tail(&encl_page->load_list, &encl->load_list);
> > +	list_add_tail(&epc_page->list, &encl->load_list);
> >  
> >  	mutex_unlock(&encl->lock);
> >  	up_read(&encl->mm->mmap_sem);
> > diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> > index 8e01427..0829ee0 100644
> > --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> > +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> > @@ -193,7 +193,7 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
> >  			      struct list_head *dst,
> >  			      unsigned long nr_to_scan)
> >  {
> > -	struct sgx_encl_page *entry;
> > +	struct sgx_epc_page *entry;
> >  	int i;
> >  
> >  	mutex_lock(&encl->lock);
> > @@ -206,15 +206,15 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
> >  			break;
> >  
> >  		entry = list_first_entry(&encl->load_list,
> > -					 struct sgx_encl_page,
> > -					 load_list);
> > +					 struct sgx_epc_page,
> > +					 list);
> >  
> > -		if (!sgx_test_and_clear_young(entry, encl) &&
> > -		    !(entry->flags & SGX_ENCL_PAGE_RESERVED)) {
> > -			entry->flags |= SGX_ENCL_PAGE_RESERVED;
> > -			list_move_tail(&entry->load_list, dst);
> > +		if (!sgx_test_and_clear_young(entry->encl_page, encl) &&
> > +		    !(entry->encl_page->flags & SGX_ENCL_PAGE_RESERVED)) {
> > +			entry->encl_page->flags |= SGX_ENCL_PAGE_RESERVED;
> > +			list_move_tail(&entry->list, dst);
> >  		} else {
> > -			list_move_tail(&entry->load_list, &encl->load_list);
> > +			list_move_tail(&entry->list, &encl->load_list);
> >  		}
> >  	}
> >  out:
> > @@ -304,25 +304,25 @@ static void sgx_evict_page(struct sgx_encl_page *entry,
> >  
> >  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
> >  {
> > -	struct sgx_encl_page *entry;
> > -	struct sgx_encl_page *tmp;
> > +	struct sgx_epc_page *entry;
> > +	struct sgx_epc_page *tmp;
> >  	struct vm_area_struct *vma;
> >  
> >  	if (list_empty(src))
> >  		return;
> >  
> > -	entry = list_first_entry(src, struct sgx_encl_page, load_list);
> > +	entry = list_first_entry(src, struct sgx_epc_page, list);
> >  
> >  	mutex_lock(&encl->lock);
> >  
> >  	/* EBLOCK */
> > -	list_for_each_entry_safe(entry, tmp, src, load_list) {
> > -		vma = sgx_find_vma(encl, entry->addr);
> > +	list_for_each_entry_safe(entry, tmp, src, list) {
> > +		vma = sgx_find_vma(encl, entry->encl_page->addr);
> >  		if (vma) {
> > -			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
> > +			zap_vma_ptes(vma, entry->encl_page->addr, PAGE_SIZE);
> >  		}
> >  
> > -		sgx_eblock(encl, entry->epc_page);
> > +		sgx_eblock(encl, entry);
> >  	}
> >  
> >  	/* ETRACK */
> > @@ -330,10 +330,9 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
> >  
> >  	/* EWB */
> >  	while (!list_empty(src)) {
> > -		entry = list_first_entry(src, struct sgx_encl_page,
> > -					 load_list);
> > -		list_del(&entry->load_list);
> > -		sgx_evict_page(entry, encl);
> > +		entry = list_first_entry(src, struct sgx_epc_page, list);
> > +		list_del(&entry->list);
> > +		sgx_evict_page(entry->encl_page, encl);
> >  		encl->secs_child_cnt--;
> >  	}
> >  
> > @@ -397,7 +396,7 @@ int sgx_add_epc_bank(resource_size_t start, unsigned long size)
> >  		new_epc_page->pa = start + i;
> >  
> >  		spin_lock(&sgx_free_list_lock);
> > -		list_add_tail(&new_epc_page->free_list, &sgx_free_list);
> > +		list_add_tail(&new_epc_page->list, &sgx_free_list);
> >  		sgx_nr_total_epc_pages++;
> >  		sgx_nr_free_pages++;
> >  		spin_unlock(&sgx_free_list_lock);
> > @@ -407,8 +406,8 @@ int sgx_add_epc_bank(resource_size_t start, unsigned long size)
> >  err_freelist:
> >  	list_for_each_safe(parser, temp, &sgx_free_list) {
> >  		spin_lock(&sgx_free_list_lock);
> > -		entry = list_entry(parser, struct sgx_epc_page, free_list);
> > -		list_del(&entry->free_list);
> > +		entry = list_entry(parser, struct sgx_epc_page, list);
> > +		list_del(&entry->list);
> >  		spin_unlock(&sgx_free_list_lock);
> >  		kfree(entry);
> >  	}
> > @@ -432,8 +431,8 @@ void sgx_page_cache_teardown(void)
> >  
> >  	spin_lock(&sgx_free_list_lock);
> >  	list_for_each_safe(parser, temp, &sgx_free_list) {
> > -		entry = list_entry(parser, struct sgx_epc_page, free_list);
> > -		list_del(&entry->free_list);
> > +		entry = list_entry(parser, struct sgx_epc_page, list);
> > +		list_del(&entry->list);
> >  		kfree(entry);
> >  	}
> >  	spin_unlock(&sgx_free_list_lock);
> > @@ -447,8 +446,8 @@ static struct sgx_epc_page *sgx_alloc_page_fast(void)
> >  
> >  	if (!list_empty(&sgx_free_list)) {
> >  		entry = list_first_entry(&sgx_free_list, struct sgx_epc_page,
> > -					 free_list);
> > -		list_del(&entry->free_list);
> > +					 list);
> > +		list_del(&entry->list);
> >  		sgx_nr_free_pages--;
> >  	}
> >  
> > @@ -533,7 +532,7 @@ int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl)
> >  	}
> >  
> >  	spin_lock(&sgx_free_list_lock);
> > -	list_add(&entry->free_list, &sgx_free_list);
> > +	list_add(&entry->list, &sgx_free_list);
> >  	sgx_nr_free_pages++;
> >  	spin_unlock(&sgx_free_list_lock);
> >  
> > diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
> > index a8a954a..021e789 100644
> > --- a/drivers/platform/x86/intel_sgx/sgx_util.c
> > +++ b/drivers/platform/x86/intel_sgx/sgx_util.c
> > @@ -110,9 +110,11 @@ struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr)
> >  
> >  void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma)
> >  {
> > +	struct sgx_epc_page *tmp;
> >  	struct sgx_encl_page *entry;
> >  
> > -	list_for_each_entry(entry, &encl->load_list, load_list) {
> > +	list_for_each_entry(tmp, &encl->load_list, list) {
> > +		entry = tmp->encl_page;
> >  		if ((entry->flags & SGX_ENCL_PAGE_TCS) &&
> >  		    entry->addr >= vma->vm_start &&
> >  		    entry->addr < vma->vm_end)
> > @@ -335,6 +337,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
> >  	 */
> >  	encl->secs_child_cnt++;
> >  
> > +	epc_page->encl_page = entry;
> >  	entry->epc_page = epc_page;
> >  
> >  	if (reserve)
> > @@ -342,7 +345,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
> >  
> >  	/* Do not free */
> >  	epc_page = NULL;
> > -	list_add_tail(&entry->load_list, &encl->load_list);
> > +	list_add_tail(&entry->epc_page->list, &encl->load_list);
> >  
> >  	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa));
> >  	if (rc) {
> > @@ -399,7 +402,7 @@ void sgx_encl_release(struct kref *ref)
> >  	radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
> >  		entry = *slot;
> >  		if (entry->epc_page) {
> > -			list_del(&entry->load_list);
> > +			list_del(&entry->epc_page->list);
> >  			sgx_free_page(entry->epc_page, encl);
> >  		}
> >  		radix_tree_delete(&encl->page_tree, entry->addr >> PAGE_SHIFT);
> > -- 
> > 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
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> ... but please try to explain rationale especially in the patches like
> this that do not radically change things but still give a lot of value.
> If you do that, I can more quickly apply them to my tree :-)
> 
> /Jarkko

Applied to my master branch. I'll squash it after some testing (and if
you don't oppose squashing it!).

/Jarkko
Jarkko Sakkinen June 16, 2017, 11:09 a.m. UTC | #3
On Fri, Jun 16, 2017 at 10:30:16AM +0200, Jarkko Sakkinen wrote:
> On Fri, Jun 16, 2017 at 10:11:44AM +0200, Jarkko Sakkinen wrote:
> > Sean,
> > 
> > On Wed, Jun 14, 2017 at 10:37:27AM -0700, Sean Christopherson wrote:
> > > Use struct sgx_epc_page's list_head member to track an EPC page in
> > > both its "free" and "loaded" states.  Add a back-pointer to the EPC
> > > page to associate a loaded EPC page with its enclave.
> > 
> > And you are making this change so that we can backtrack EPC pages from
> > the global list, am I correct (the commit message explains only action
> > not the rationale for doing the change)?
> > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > I might squash this to my first upstream patch set if you don't mind.
> > 
> > > ---
> > >  arch/x86/include/asm/sgx.h                      |  3 +-
> > >  drivers/platform/x86/intel_sgx/sgx.h            |  1 -
> > >  drivers/platform/x86/intel_sgx/sgx_ioctl.c      |  3 +-
> > >  drivers/platform/x86/intel_sgx/sgx_page_cache.c | 53 ++++++++++++-------------
> > >  drivers/platform/x86/intel_sgx/sgx_util.c       |  9 +++--
> > >  5 files changed, 36 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> > > index 31bce3f..e1f27fd 100644
> > > --- a/arch/x86/include/asm/sgx.h
> > > +++ b/arch/x86/include/asm/sgx.h
> > > @@ -343,7 +343,8 @@ struct sgx_encl;
> > >  
> > >  struct sgx_epc_page {
> > >  	resource_size_t	pa;
> > > -	struct list_head free_list;
> > > +	struct list_head list;
> > > +	struct sgx_encl_page *encl_page;
> > 
> > It would have been good mention also in the commit message that memory
> > is saved as a whole even if a new field added.
> > 
> > There's enough rationale to squash this even without the cgroup context
> > but the commit message is lacking *all of it* :-)
> > 
> > >  };
> > >  
> > >  enum sgx_alloc_flags {
> > > diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
> > > index d5cdf96..78d048e 100644
> > > --- a/drivers/platform/x86/intel_sgx/sgx.h
> > > +++ b/drivers/platform/x86/intel_sgx/sgx.h
> > > @@ -107,7 +107,6 @@ struct sgx_encl_page {
> > >  	unsigned long addr;
> > >  	unsigned int flags;
> > >  	struct sgx_epc_page *epc_page;
> > > -	struct list_head load_list;
> > 
> > This is where the memory savings comes from.
> > 
> > >  	struct sgx_va_page *va_page;
> > >  	unsigned int va_offset;
> > >  };
> > > diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> > > index 70d7417..0741e6c 100644
> > > --- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> > > +++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
> > > @@ -267,9 +267,10 @@ static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
> > >  		goto out;
> > >  	}
> > >  
> > > +	epc_page->encl_page = encl_page;
> > >  	encl_page->epc_page = epc_page;
> > >  	sgx_test_and_clear_young(encl_page, encl);
> > > -	list_add_tail(&encl_page->load_list, &encl->load_list);
> > > +	list_add_tail(&epc_page->list, &encl->load_list);
> > >  
> > >  	mutex_unlock(&encl->lock);
> > >  	up_read(&encl->mm->mmap_sem);
> > > diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> > > index 8e01427..0829ee0 100644
> > > --- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> > > +++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
> > > @@ -193,7 +193,7 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
> > >  			      struct list_head *dst,
> > >  			      unsigned long nr_to_scan)
> > >  {
> > > -	struct sgx_encl_page *entry;
> > > +	struct sgx_epc_page *entry;
> > >  	int i;
> > >  
> > >  	mutex_lock(&encl->lock);
> > > @@ -206,15 +206,15 @@ static void sgx_isolate_pages(struct sgx_encl *encl,
> > >  			break;
> > >  
> > >  		entry = list_first_entry(&encl->load_list,
> > > -					 struct sgx_encl_page,
> > > -					 load_list);
> > > +					 struct sgx_epc_page,
> > > +					 list);
> > >  
> > > -		if (!sgx_test_and_clear_young(entry, encl) &&
> > > -		    !(entry->flags & SGX_ENCL_PAGE_RESERVED)) {
> > > -			entry->flags |= SGX_ENCL_PAGE_RESERVED;
> > > -			list_move_tail(&entry->load_list, dst);
> > > +		if (!sgx_test_and_clear_young(entry->encl_page, encl) &&
> > > +		    !(entry->encl_page->flags & SGX_ENCL_PAGE_RESERVED)) {
> > > +			entry->encl_page->flags |= SGX_ENCL_PAGE_RESERVED;
> > > +			list_move_tail(&entry->list, dst);
> > >  		} else {
> > > -			list_move_tail(&entry->load_list, &encl->load_list);
> > > +			list_move_tail(&entry->list, &encl->load_list);
> > >  		}
> > >  	}
> > >  out:
> > > @@ -304,25 +304,25 @@ static void sgx_evict_page(struct sgx_encl_page *entry,
> > >  
> > >  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
> > >  {
> > > -	struct sgx_encl_page *entry;
> > > -	struct sgx_encl_page *tmp;
> > > +	struct sgx_epc_page *entry;
> > > +	struct sgx_epc_page *tmp;
> > >  	struct vm_area_struct *vma;
> > >  
> > >  	if (list_empty(src))
> > >  		return;
> > >  
> > > -	entry = list_first_entry(src, struct sgx_encl_page, load_list);
> > > +	entry = list_first_entry(src, struct sgx_epc_page, list);
> > >  
> > >  	mutex_lock(&encl->lock);
> > >  
> > >  	/* EBLOCK */
> > > -	list_for_each_entry_safe(entry, tmp, src, load_list) {
> > > -		vma = sgx_find_vma(encl, entry->addr);
> > > +	list_for_each_entry_safe(entry, tmp, src, list) {
> > > +		vma = sgx_find_vma(encl, entry->encl_page->addr);
> > >  		if (vma) {
> > > -			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
> > > +			zap_vma_ptes(vma, entry->encl_page->addr, PAGE_SIZE);
> > >  		}
> > >  
> > > -		sgx_eblock(encl, entry->epc_page);
> > > +		sgx_eblock(encl, entry);
> > >  	}
> > >  
> > >  	/* ETRACK */
> > > @@ -330,10 +330,9 @@ static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
> > >  
> > >  	/* EWB */
> > >  	while (!list_empty(src)) {
> > > -		entry = list_first_entry(src, struct sgx_encl_page,
> > > -					 load_list);
> > > -		list_del(&entry->load_list);
> > > -		sgx_evict_page(entry, encl);
> > > +		entry = list_first_entry(src, struct sgx_epc_page, list);
> > > +		list_del(&entry->list);
> > > +		sgx_evict_page(entry->encl_page, encl);
> > >  		encl->secs_child_cnt--;
> > >  	}
> > >  
> > > @@ -397,7 +396,7 @@ int sgx_add_epc_bank(resource_size_t start, unsigned long size)
> > >  		new_epc_page->pa = start + i;
> > >  
> > >  		spin_lock(&sgx_free_list_lock);
> > > -		list_add_tail(&new_epc_page->free_list, &sgx_free_list);
> > > +		list_add_tail(&new_epc_page->list, &sgx_free_list);
> > >  		sgx_nr_total_epc_pages++;
> > >  		sgx_nr_free_pages++;
> > >  		spin_unlock(&sgx_free_list_lock);
> > > @@ -407,8 +406,8 @@ int sgx_add_epc_bank(resource_size_t start, unsigned long size)
> > >  err_freelist:
> > >  	list_for_each_safe(parser, temp, &sgx_free_list) {
> > >  		spin_lock(&sgx_free_list_lock);
> > > -		entry = list_entry(parser, struct sgx_epc_page, free_list);
> > > -		list_del(&entry->free_list);
> > > +		entry = list_entry(parser, struct sgx_epc_page, list);
> > > +		list_del(&entry->list);
> > >  		spin_unlock(&sgx_free_list_lock);
> > >  		kfree(entry);
> > >  	}
> > > @@ -432,8 +431,8 @@ void sgx_page_cache_teardown(void)
> > >  
> > >  	spin_lock(&sgx_free_list_lock);
> > >  	list_for_each_safe(parser, temp, &sgx_free_list) {
> > > -		entry = list_entry(parser, struct sgx_epc_page, free_list);
> > > -		list_del(&entry->free_list);
> > > +		entry = list_entry(parser, struct sgx_epc_page, list);
> > > +		list_del(&entry->list);
> > >  		kfree(entry);
> > >  	}
> > >  	spin_unlock(&sgx_free_list_lock);
> > > @@ -447,8 +446,8 @@ static struct sgx_epc_page *sgx_alloc_page_fast(void)
> > >  
> > >  	if (!list_empty(&sgx_free_list)) {
> > >  		entry = list_first_entry(&sgx_free_list, struct sgx_epc_page,
> > > -					 free_list);
> > > -		list_del(&entry->free_list);
> > > +					 list);
> > > +		list_del(&entry->list);
> > >  		sgx_nr_free_pages--;
> > >  	}
> > >  
> > > @@ -533,7 +532,7 @@ int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl)
> > >  	}
> > >  
> > >  	spin_lock(&sgx_free_list_lock);
> > > -	list_add(&entry->free_list, &sgx_free_list);
> > > +	list_add(&entry->list, &sgx_free_list);
> > >  	sgx_nr_free_pages++;
> > >  	spin_unlock(&sgx_free_list_lock);
> > >  
> > > diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
> > > index a8a954a..021e789 100644
> > > --- a/drivers/platform/x86/intel_sgx/sgx_util.c
> > > +++ b/drivers/platform/x86/intel_sgx/sgx_util.c
> > > @@ -110,9 +110,11 @@ struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr)
> > >  
> > >  void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma)
> > >  {
> > > +	struct sgx_epc_page *tmp;
> > >  	struct sgx_encl_page *entry;
> > >  
> > > -	list_for_each_entry(entry, &encl->load_list, load_list) {
> > > +	list_for_each_entry(tmp, &encl->load_list, list) {
> > > +		entry = tmp->encl_page;
> > >  		if ((entry->flags & SGX_ENCL_PAGE_TCS) &&
> > >  		    entry->addr >= vma->vm_start &&
> > >  		    entry->addr < vma->vm_end)
> > > @@ -335,6 +337,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
> > >  	 */
> > >  	encl->secs_child_cnt++;
> > >  
> > > +	epc_page->encl_page = entry;
> > >  	entry->epc_page = epc_page;
> > >  
> > >  	if (reserve)
> > > @@ -342,7 +345,7 @@ static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
> > >  
> > >  	/* Do not free */
> > >  	epc_page = NULL;
> > > -	list_add_tail(&entry->load_list, &encl->load_list);
> > > +	list_add_tail(&entry->epc_page->list, &encl->load_list);
> > >  
> > >  	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa));
> > >  	if (rc) {
> > > @@ -399,7 +402,7 @@ void sgx_encl_release(struct kref *ref)
> > >  	radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
> > >  		entry = *slot;
> > >  		if (entry->epc_page) {
> > > -			list_del(&entry->load_list);
> > > +			list_del(&entry->epc_page->list);
> > >  			sgx_free_page(entry->epc_page, encl);
> > >  		}
> > >  		radix_tree_delete(&encl->page_tree, entry->addr >> PAGE_SHIFT);
> > > -- 
> > > 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
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > ... but please try to explain rationale especially in the patches like
> > this that do not radically change things but still give a lot of value.
> > If you do that, I can more quickly apply them to my tree :-)
> > 
> > /Jarkko
> 
> Applied to my master branch. I'll squash it after some testing (and if
> you don't oppose squashing it!).
> 
> /Jarkko

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

I've also squashed this to get these changes for the first upstream
version.

In addition I've recently squahed these patches:

* https://patchwork.kernel.org/patch/9786895/
* https://patchwork.kernel.org/patch/9790319/

They are also in the next branch.

Sean, you are sending now a subsequent patch set. Are you planning to
send updated patch set for combining epc and va? I would rather go
through one at a time.

Are you planning to setup a Github tree for this cgroup stuff?

/Jarkko
Sean Christopherson June 16, 2017, 3:12 p.m. UTC | #4
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> ... but please try to explain rationale especially in the patches like
> this that do not radically change things but still give a lot of value.
> If you do that, I can more quickly apply them to my tree :-)
> 
> /Jarkko

My apologies, I simply forgot to revisit this patch to update its commit
message once the implementation had stabilized.  Let me know if any other
patches need clarification.
Jarkko Sakkinen June 16, 2017, 10:52 p.m. UTC | #5
On Fri, Jun 16, 2017 at 03:12:28PM +0000, Christopherson, Sean J wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > ... but please try to explain rationale especially in the patches like
> > this that do not radically change things but still give a lot of value.
> > If you do that, I can more quickly apply them to my tree :-)
> > 
> > /Jarkko
> 
> My apologies, I simply forgot to revisit this patch to update its commit
> message once the implementation had stabilized.  Let me know if any other
> patches need clarification.

I squashed this one to the driver commit (I hope you didn't mind, you
anyway have the proper credit in the authors field), thanks for the good
work!

/JArkko

Patch
diff mbox

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 31bce3f..e1f27fd 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -343,7 +343,8 @@  struct sgx_encl;
 
 struct sgx_epc_page {
 	resource_size_t	pa;
-	struct list_head free_list;
+	struct list_head list;
+	struct sgx_encl_page *encl_page;
 };
 
 enum sgx_alloc_flags {
diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/drivers/platform/x86/intel_sgx/sgx.h
index d5cdf96..78d048e 100644
--- a/drivers/platform/x86/intel_sgx/sgx.h
+++ b/drivers/platform/x86/intel_sgx/sgx.h
@@ -107,7 +107,6 @@  struct sgx_encl_page {
 	unsigned long addr;
 	unsigned int flags;
 	struct sgx_epc_page *epc_page;
-	struct list_head load_list;
 	struct sgx_va_page *va_page;
 	unsigned int va_offset;
 };
diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
index 70d7417..0741e6c 100644
--- a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
+++ b/drivers/platform/x86/intel_sgx/sgx_ioctl.c
@@ -267,9 +267,10 @@  static bool sgx_process_add_page_req(struct sgx_add_page_req *req)
 		goto out;
 	}
 
+	epc_page->encl_page = encl_page;
 	encl_page->epc_page = epc_page;
 	sgx_test_and_clear_young(encl_page, encl);
-	list_add_tail(&encl_page->load_list, &encl->load_list);
+	list_add_tail(&epc_page->list, &encl->load_list);
 
 	mutex_unlock(&encl->lock);
 	up_read(&encl->mm->mmap_sem);
diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
index 8e01427..0829ee0 100644
--- a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx/sgx_page_cache.c
@@ -193,7 +193,7 @@  static void sgx_isolate_pages(struct sgx_encl *encl,
 			      struct list_head *dst,
 			      unsigned long nr_to_scan)
 {
-	struct sgx_encl_page *entry;
+	struct sgx_epc_page *entry;
 	int i;
 
 	mutex_lock(&encl->lock);
@@ -206,15 +206,15 @@  static void sgx_isolate_pages(struct sgx_encl *encl,
 			break;
 
 		entry = list_first_entry(&encl->load_list,
-					 struct sgx_encl_page,
-					 load_list);
+					 struct sgx_epc_page,
+					 list);
 
-		if (!sgx_test_and_clear_young(entry, encl) &&
-		    !(entry->flags & SGX_ENCL_PAGE_RESERVED)) {
-			entry->flags |= SGX_ENCL_PAGE_RESERVED;
-			list_move_tail(&entry->load_list, dst);
+		if (!sgx_test_and_clear_young(entry->encl_page, encl) &&
+		    !(entry->encl_page->flags & SGX_ENCL_PAGE_RESERVED)) {
+			entry->encl_page->flags |= SGX_ENCL_PAGE_RESERVED;
+			list_move_tail(&entry->list, dst);
 		} else {
-			list_move_tail(&entry->load_list, &encl->load_list);
+			list_move_tail(&entry->list, &encl->load_list);
 		}
 	}
 out:
@@ -304,25 +304,25 @@  static void sgx_evict_page(struct sgx_encl_page *entry,
 
 static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 {
-	struct sgx_encl_page *entry;
-	struct sgx_encl_page *tmp;
+	struct sgx_epc_page *entry;
+	struct sgx_epc_page *tmp;
 	struct vm_area_struct *vma;
 
 	if (list_empty(src))
 		return;
 
-	entry = list_first_entry(src, struct sgx_encl_page, load_list);
+	entry = list_first_entry(src, struct sgx_epc_page, list);
 
 	mutex_lock(&encl->lock);
 
 	/* EBLOCK */
-	list_for_each_entry_safe(entry, tmp, src, load_list) {
-		vma = sgx_find_vma(encl, entry->addr);
+	list_for_each_entry_safe(entry, tmp, src, list) {
+		vma = sgx_find_vma(encl, entry->encl_page->addr);
 		if (vma) {
-			zap_vma_ptes(vma, entry->addr, PAGE_SIZE);
+			zap_vma_ptes(vma, entry->encl_page->addr, PAGE_SIZE);
 		}
 
-		sgx_eblock(encl, entry->epc_page);
+		sgx_eblock(encl, entry);
 	}
 
 	/* ETRACK */
@@ -330,10 +330,9 @@  static void sgx_write_pages(struct sgx_encl *encl, struct list_head *src)
 
 	/* EWB */
 	while (!list_empty(src)) {
-		entry = list_first_entry(src, struct sgx_encl_page,
-					 load_list);
-		list_del(&entry->load_list);
-		sgx_evict_page(entry, encl);
+		entry = list_first_entry(src, struct sgx_epc_page, list);
+		list_del(&entry->list);
+		sgx_evict_page(entry->encl_page, encl);
 		encl->secs_child_cnt--;
 	}
 
@@ -397,7 +396,7 @@  int sgx_add_epc_bank(resource_size_t start, unsigned long size)
 		new_epc_page->pa = start + i;
 
 		spin_lock(&sgx_free_list_lock);
-		list_add_tail(&new_epc_page->free_list, &sgx_free_list);
+		list_add_tail(&new_epc_page->list, &sgx_free_list);
 		sgx_nr_total_epc_pages++;
 		sgx_nr_free_pages++;
 		spin_unlock(&sgx_free_list_lock);
@@ -407,8 +406,8 @@  int sgx_add_epc_bank(resource_size_t start, unsigned long size)
 err_freelist:
 	list_for_each_safe(parser, temp, &sgx_free_list) {
 		spin_lock(&sgx_free_list_lock);
-		entry = list_entry(parser, struct sgx_epc_page, free_list);
-		list_del(&entry->free_list);
+		entry = list_entry(parser, struct sgx_epc_page, list);
+		list_del(&entry->list);
 		spin_unlock(&sgx_free_list_lock);
 		kfree(entry);
 	}
@@ -432,8 +431,8 @@  void sgx_page_cache_teardown(void)
 
 	spin_lock(&sgx_free_list_lock);
 	list_for_each_safe(parser, temp, &sgx_free_list) {
-		entry = list_entry(parser, struct sgx_epc_page, free_list);
-		list_del(&entry->free_list);
+		entry = list_entry(parser, struct sgx_epc_page, list);
+		list_del(&entry->list);
 		kfree(entry);
 	}
 	spin_unlock(&sgx_free_list_lock);
@@ -447,8 +446,8 @@  static struct sgx_epc_page *sgx_alloc_page_fast(void)
 
 	if (!list_empty(&sgx_free_list)) {
 		entry = list_first_entry(&sgx_free_list, struct sgx_epc_page,
-					 free_list);
-		list_del(&entry->free_list);
+					 list);
+		list_del(&entry->list);
 		sgx_nr_free_pages--;
 	}
 
@@ -533,7 +532,7 @@  int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl)
 	}
 
 	spin_lock(&sgx_free_list_lock);
-	list_add(&entry->free_list, &sgx_free_list);
+	list_add(&entry->list, &sgx_free_list);
 	sgx_nr_free_pages++;
 	spin_unlock(&sgx_free_list_lock);
 
diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c b/drivers/platform/x86/intel_sgx/sgx_util.c
index a8a954a..021e789 100644
--- a/drivers/platform/x86/intel_sgx/sgx_util.c
+++ b/drivers/platform/x86/intel_sgx/sgx_util.c
@@ -110,9 +110,11 @@  struct vm_area_struct *sgx_find_vma(struct sgx_encl *encl, unsigned long addr)
 
 void sgx_zap_tcs_ptes(struct sgx_encl *encl, struct vm_area_struct *vma)
 {
+	struct sgx_epc_page *tmp;
 	struct sgx_encl_page *entry;
 
-	list_for_each_entry(entry, &encl->load_list, load_list) {
+	list_for_each_entry(tmp, &encl->load_list, list) {
+		entry = tmp->encl_page;
 		if ((entry->flags & SGX_ENCL_PAGE_TCS) &&
 		    entry->addr >= vma->vm_start &&
 		    entry->addr < vma->vm_end)
@@ -335,6 +337,7 @@  static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
 	 */
 	encl->secs_child_cnt++;
 
+	epc_page->encl_page = entry;
 	entry->epc_page = epc_page;
 
 	if (reserve)
@@ -342,7 +345,7 @@  static struct sgx_encl_page *sgx_do_fault(struct vm_area_struct *vma,
 
 	/* Do not free */
 	epc_page = NULL;
-	list_add_tail(&entry->load_list, &encl->load_list);
+	list_add_tail(&entry->epc_page->list, &encl->load_list);
 
 	rc = vm_insert_pfn(vma, entry->addr, PFN_DOWN(entry->epc_page->pa));
 	if (rc) {
@@ -399,7 +402,7 @@  void sgx_encl_release(struct kref *ref)
 	radix_tree_for_each_slot(slot, &encl->page_tree, &iter, 0) {
 		entry = *slot;
 		if (entry->epc_page) {
-			list_del(&entry->load_list);
+			list_del(&entry->epc_page->list);
 			sgx_free_page(entry->epc_page, encl);
 		}
 		radix_tree_delete(&encl->page_tree, entry->addr >> PAGE_SHIFT);