| Message ID | 1497461858-20309-2-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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
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);
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(-)