* [PATCH for_v23 0/3] x86/sgx: More cleanup for v23
@ 2019-10-22 22:49 Sean Christopherson
2019-10-22 22:49 ` [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation Sean Christopherson
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Sean Christopherson @ 2019-10-22 22:49 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: linux-sgx
Two small enchancements, and a rework of the sgx_free_page() split that
was sent in a previous series.
Sean Christopherson (3):
x86/sgx: Update the free page count in a single operation
x86/sgx: Do not add in-use EPC page to the free page list
x86/sgx: Move reclaim logic out of sgx_free_page()
arch/x86/kernel/cpu/sgx/encl.c | 3 ++-
arch/x86/kernel/cpu/sgx/main.c | 36 +++++++++----------------------
arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++
arch/x86/kernel/cpu/sgx/sgx.h | 3 ++-
4 files changed, 46 insertions(+), 28 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation 2019-10-22 22:49 [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Sean Christopherson @ 2019-10-22 22:49 ` Sean Christopherson 2019-10-23 12:44 ` Jarkko Sakkinen 2019-10-22 22:49 ` [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list Sean Christopherson ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Sean Christopherson @ 2019-10-22 22:49 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx Use atomic_add() instead of running atomic_inc() in a loop to manually do the equivalent addition. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 499a9b0740c8..d45bf6fca0c8 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -195,8 +195,7 @@ static bool __init sgx_alloc_epc_section(u64 addr, u64 size, list_add_tail(&page->list, §ion->unsanitized_page_list); } - for (i = 0; i < nr_pages; i++) - atomic_inc(&sgx_nr_free_pages); + atomic_add(nr_pages, &sgx_nr_free_pages); return true; -- 2.22.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation 2019-10-22 22:49 ` [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation Sean Christopherson @ 2019-10-23 12:44 ` Jarkko Sakkinen 2019-10-24 13:11 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2019-10-23 12:44 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx On Tue, Oct 22, 2019 at 03:49:20PM -0700, Sean Christopherson wrote: > Use atomic_add() instead of running atomic_inc() in a loop to manually > do the equivalent addition. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kernel/cpu/sgx/main.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 499a9b0740c8..d45bf6fca0c8 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -195,8 +195,7 @@ static bool __init sgx_alloc_epc_section(u64 addr, u64 size, > list_add_tail(&page->list, §ion->unsanitized_page_list); > } > > - for (i = 0; i < nr_pages; i++) > - atomic_inc(&sgx_nr_free_pages); > + atomic_add(nr_pages, &sgx_nr_free_pages); > > return true; > > -- > 2.22.0 > There reason I used atomic_inc() was that atomic_add() takes int that could potentially overflow. I'll ignore this as I'll do the revert that I promised to do. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation 2019-10-23 12:44 ` Jarkko Sakkinen @ 2019-10-24 13:11 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2019-10-24 13:11 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx On Wed, Oct 23, 2019 at 03:44:49PM +0300, Jarkko Sakkinen wrote: > On Tue, Oct 22, 2019 at 03:49:20PM -0700, Sean Christopherson wrote: > > Use atomic_add() instead of running atomic_inc() in a loop to manually > > do the equivalent addition. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kernel/cpu/sgx/main.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index 499a9b0740c8..d45bf6fca0c8 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -195,8 +195,7 @@ static bool __init sgx_alloc_epc_section(u64 addr, u64 size, > > list_add_tail(&page->list, §ion->unsanitized_page_list); > > } > > > > - for (i = 0; i < nr_pages; i++) > > - atomic_inc(&sgx_nr_free_pages); > > + atomic_add(nr_pages, &sgx_nr_free_pages); > > > > return true; > > > > -- > > 2.22.0 > > > > There reason I used atomic_inc() was that atomic_add() takes int that > could potentially overflow. > > I'll ignore this as I'll do the revert that I promised to do. static inline unsigned long sgx_nr_free_pages(void) { unsigned long cnt = 0; int i; for (i = 0; i < sgx_nr_epc_sections; i++) cnt += sgx_epc_sections[i].free_cnt; return cnt; } static inline bool sgx_should_reclaim(unsigned long watermark) { return sgx_nr_free_pages() < watermark && !list_empty(&sgx_active_page_list); } I use the latter in all call sites. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list 2019-10-22 22:49 [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Sean Christopherson 2019-10-22 22:49 ` [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation Sean Christopherson @ 2019-10-22 22:49 ` Sean Christopherson 2019-10-23 12:43 ` Jarkko Sakkinen 2019-10-22 22:49 ` [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page() Sean Christopherson 2019-10-24 18:35 ` [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Jarkko Sakkinen 3 siblings, 1 reply; 13+ messages in thread From: Sean Christopherson @ 2019-10-22 22:49 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx Don't add an EPC page to the free page list of EREMOVE fails, as doing so will cause any future attempt to use the EPC page to fail, and likely WARN as well. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kernel/cpu/sgx/main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index d45bf6fca0c8..8e7557d3ff03 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page) spin_unlock(&sgx_active_page_list_lock); ret = __eremove(sgx_epc_addr(page)); - WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret); + if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) + return -EIO; spin_lock(§ion->lock); list_add_tail(&page->list, §ion->page_list); -- 2.22.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list 2019-10-22 22:49 ` [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list Sean Christopherson @ 2019-10-23 12:43 ` Jarkko Sakkinen 2019-10-23 15:23 ` Sean Christopherson 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2019-10-23 12:43 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx On Tue, Oct 22, 2019 at 03:49:21PM -0700, Sean Christopherson wrote: > Don't add an EPC page to the free page list of EREMOVE fails, as doing > so will cause any future attempt to use the EPC page to fail, and likely > WARN as well. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kernel/cpu/sgx/main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index d45bf6fca0c8..8e7557d3ff03 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page) > spin_unlock(&sgx_active_page_list_lock); > > ret = __eremove(sgx_epc_addr(page)); > - WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret); > + if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) > + return -EIO; How did you end up choosing -EIO? Don't immediately have any better suggestion but neither sure if that is the best choice. That is why I'm asking. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list 2019-10-23 12:43 ` Jarkko Sakkinen @ 2019-10-23 15:23 ` Sean Christopherson 0 siblings, 0 replies; 13+ messages in thread From: Sean Christopherson @ 2019-10-23 15:23 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx On Wed, Oct 23, 2019 at 03:43:40PM +0300, Jarkko Sakkinen wrote: > On Tue, Oct 22, 2019 at 03:49:21PM -0700, Sean Christopherson wrote: > > Don't add an EPC page to the free page list of EREMOVE fails, as doing > > so will cause any future attempt to use the EPC page to fail, and likely > > WARN as well. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kernel/cpu/sgx/main.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index d45bf6fca0c8..8e7557d3ff03 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -138,7 +138,8 @@ int sgx_free_page(struct sgx_epc_page *page) > > spin_unlock(&sgx_active_page_list_lock); > > > > ret = __eremove(sgx_epc_addr(page)); > > - WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret); > > + if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) > > + return -EIO; > > How did you end up choosing -EIO? Don't immediately have any better > suggestion but neither sure if that is the best choice. That is why I'm > asking. sgx_edbgrd() and sgx_edbgwr() return -EIO on failure, and they're the only other case I can think of where an ENCLS instruction failure is reported to userspace *and* may or may not be due to a fault. I honestly didn't spend much time thinking about it as the code is dropped in the next path. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page() 2019-10-22 22:49 [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Sean Christopherson 2019-10-22 22:49 ` [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation Sean Christopherson 2019-10-22 22:49 ` [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list Sean Christopherson @ 2019-10-22 22:49 ` Sean Christopherson 2019-10-23 12:42 ` Jarkko Sakkinen 2019-10-24 18:35 ` [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Jarkko Sakkinen 3 siblings, 1 reply; 13+ messages in thread From: Sean Christopherson @ 2019-10-22 22:49 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx Move the reclaim logic out of sgx_free_page() and into a standalone helper to avoid taking sgx_active_page_list_lock when the page is known to be unreclaimable, which is the vast majority of flows that free EPC pages. Movig reclaim logic to a separate function also eliminates any possibility of silently leaking a page because it is unexpectedly reclaimable (and being actively reclaimed). Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- I really don't like the sgx_unmark_...() name, but couldn't come up with anything better. Suggestions welcome... arch/x86/kernel/cpu/sgx/encl.c | 3 ++- arch/x86/kernel/cpu/sgx/main.c | 32 ++++++++----------------------- arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++++++ arch/x86/kernel/cpu/sgx/sgx.h | 3 ++- 4 files changed, 44 insertions(+), 26 deletions(-) diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 8045f1ddfd62..22186d89042a 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -474,9 +474,10 @@ void sgx_encl_destroy(struct sgx_encl *encl) * The page and its radix tree entry cannot be freed * if the page is being held by the reclaimer. */ - if (sgx_free_page(entry->epc_page)) + if (sgx_unmark_page_reclaimable(entry->epc_page)) continue; + sgx_free_page(entry->epc_page); encl->secs_child_cnt--; entry->epc_page = NULL; } diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 8e7557d3ff03..cfd8480ef563 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -108,45 +108,29 @@ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim) * sgx_free_page() - Free an EPC page * @page: pointer a previously allocated EPC page * - * EREMOVE an EPC page and insert it back to the list of free pages. If the - * page is reclaimable, delete it from the active page list. - * - * Return: - * 0 on success, - * -EBUSY if a reclaim is in progress + * EREMOVE an EPC page and insert it back to the list of free pages. The page + * must not be reclaimable. */ -int sgx_free_page(struct sgx_epc_page *page) +void sgx_free_page(struct sgx_epc_page *page) { struct sgx_epc_section *section = sgx_epc_section(page); int ret; /* - * Remove the page from the active list if necessary. If the page - * is actively being reclaimed, i.e. RECLAIMABLE is set but the - * page isn't on the active list, return -EBUSY as we can't free - * the page at this time since it is "owned" by the reclaimer. + * Don't take sgx_active_page_list_lock when asserting the page isn't + * reclaimable, missing a WARN in the very rare case is preferable to + * unnecessarily taking a global lock in the common case. */ - spin_lock(&sgx_active_page_list_lock); - if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) { - if (list_empty(&page->list)) { - spin_unlock(&sgx_active_page_list_lock); - return -EBUSY; - } - list_del(&page->list); - page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE; - } - spin_unlock(&sgx_active_page_list_lock); + WARN_ON_ONCE(page->desc & SGX_EPC_PAGE_RECLAIMABLE); ret = __eremove(sgx_epc_addr(page)); if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) - return -EIO; + return; spin_lock(§ion->lock); list_add_tail(&page->list, §ion->page_list); atomic_inc(&sgx_nr_free_pages); spin_unlock(§ion->lock); - - return 0; } static void __init sgx_free_epc_section(struct sgx_epc_section *section) diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c index 8067ce1915a4..e64c810883ec 100644 --- a/arch/x86/kernel/cpu/sgx/reclaim.c +++ b/arch/x86/kernel/cpu/sgx/reclaim.c @@ -125,6 +125,38 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page) spin_unlock(&sgx_active_page_list_lock); } +/** + * sgx_unmark_page_reclaimable() - Remove a page from the reclaim list + * @page: EPC page + * + * Clear the reclaimable flag and remove the page from the active page list. + * + * Return: + * 0 on success, + * -EBUSY if the page is in the process of being reclaimed + */ +int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) +{ + /* + * Remove the page from the active list if necessary. If the page + * is actively being reclaimed, i.e. RECLAIMABLE is set but the + * page isn't on the active list, return -EBUSY as we can't free + * the page at this time since it is "owned" by the reclaimer. + */ + spin_lock(&sgx_active_page_list_lock); + if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) { + if (list_empty(&page->list)) { + spin_unlock(&sgx_active_page_list_lock); + return -EBUSY; + } + list_del(&page->list); + page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE; + } + spin_unlock(&sgx_active_page_list_lock); + + return 0; +} + static bool sgx_reclaimer_age(struct sgx_epc_page *epc_page) { struct sgx_encl_page *page = epc_page->owner; diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h index 45753f236a83..f6d23ef7c74a 100644 --- a/arch/x86/kernel/cpu/sgx/sgx.h +++ b/arch/x86/kernel/cpu/sgx/sgx.h @@ -81,10 +81,11 @@ extern spinlock_t sgx_active_page_list_lock; bool __init sgx_page_reclaimer_init(void); void sgx_mark_page_reclaimable(struct sgx_epc_page *page); +int sgx_unmark_page_reclaimable(struct sgx_epc_page *page); void sgx_reclaim_pages(void); struct sgx_epc_page *sgx_try_alloc_page(void); struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim); -int sgx_free_page(struct sgx_epc_page *page); +void sgx_free_page(struct sgx_epc_page *page); #endif /* _X86_SGX_H */ -- 2.22.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page() 2019-10-22 22:49 ` [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page() Sean Christopherson @ 2019-10-23 12:42 ` Jarkko Sakkinen 2019-10-23 15:19 ` Sean Christopherson 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2019-10-23 12:42 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx On Tue, Oct 22, 2019 at 03:49:22PM -0700, Sean Christopherson wrote: > Move the reclaim logic out of sgx_free_page() and into a standalone > helper to avoid taking sgx_active_page_list_lock when the page is known > to be unreclaimable, which is the vast majority of flows that free EPC > pages. > > Movig reclaim logic to a separate function also eliminates any > possibility of silently leaking a page because it is unexpectedly > reclaimable (and being actively reclaimed). > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > > I really don't like the sgx_unmark_...() name, but couldn't come up with > anything better. Suggestions welcome... > > arch/x86/kernel/cpu/sgx/encl.c | 3 ++- > arch/x86/kernel/cpu/sgx/main.c | 32 ++++++++----------------------- > arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++++++ > arch/x86/kernel/cpu/sgx/sgx.h | 3 ++- > 4 files changed, 44 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 8045f1ddfd62..22186d89042a 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -474,9 +474,10 @@ void sgx_encl_destroy(struct sgx_encl *encl) > * The page and its radix tree entry cannot be freed > * if the page is being held by the reclaimer. > */ > - if (sgx_free_page(entry->epc_page)) > + if (sgx_unmark_page_reclaimable(entry->epc_page)) > continue; > > + sgx_free_page(entry->epc_page); > encl->secs_child_cnt--; > entry->epc_page = NULL; > } > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 8e7557d3ff03..cfd8480ef563 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -108,45 +108,29 @@ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim) > * sgx_free_page() - Free an EPC page > * @page: pointer a previously allocated EPC page > * > - * EREMOVE an EPC page and insert it back to the list of free pages. If the > - * page is reclaimable, delete it from the active page list. > - * > - * Return: > - * 0 on success, > - * -EBUSY if a reclaim is in progress > + * EREMOVE an EPC page and insert it back to the list of free pages. The page > + * must not be reclaimable. > */ > -int sgx_free_page(struct sgx_epc_page *page) > +void sgx_free_page(struct sgx_epc_page *page) > { > struct sgx_epc_section *section = sgx_epc_section(page); > int ret; > > /* > - * Remove the page from the active list if necessary. If the page > - * is actively being reclaimed, i.e. RECLAIMABLE is set but the > - * page isn't on the active list, return -EBUSY as we can't free > - * the page at this time since it is "owned" by the reclaimer. > + * Don't take sgx_active_page_list_lock when asserting the page isn't > + * reclaimable, missing a WARN in the very rare case is preferable to > + * unnecessarily taking a global lock in the common case. > */ > - spin_lock(&sgx_active_page_list_lock); > - if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) { > - if (list_empty(&page->list)) { > - spin_unlock(&sgx_active_page_list_lock); > - return -EBUSY; > - } > - list_del(&page->list); > - page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE; > - } > - spin_unlock(&sgx_active_page_list_lock); > + WARN_ON_ONCE(page->desc & SGX_EPC_PAGE_RECLAIMABLE); > > ret = __eremove(sgx_epc_addr(page)); > if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) > - return -EIO; > + return; > > spin_lock(§ion->lock); > list_add_tail(&page->list, §ion->page_list); > atomic_inc(&sgx_nr_free_pages); > spin_unlock(§ion->lock); > - > - return 0; > } > > static void __init sgx_free_epc_section(struct sgx_epc_section *section) > diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c > index 8067ce1915a4..e64c810883ec 100644 > --- a/arch/x86/kernel/cpu/sgx/reclaim.c > +++ b/arch/x86/kernel/cpu/sgx/reclaim.c > @@ -125,6 +125,38 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page) > spin_unlock(&sgx_active_page_list_lock); > } > > +/** > + * sgx_unmark_page_reclaimable() - Remove a page from the reclaim list > + * @page: EPC page > + * > + * Clear the reclaimable flag and remove the page from the active page list. > + * > + * Return: > + * 0 on success, > + * -EBUSY if the page is in the process of being reclaimed > + */ > +int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) > +{ > + /* > + * Remove the page from the active list if necessary. If the page > + * is actively being reclaimed, i.e. RECLAIMABLE is set but the > + * page isn't on the active list, return -EBUSY as we can't free > + * the page at this time since it is "owned" by the reclaimer. > + */ > + spin_lock(&sgx_active_page_list_lock); > + if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) { > + if (list_empty(&page->list)) { > + spin_unlock(&sgx_active_page_list_lock); > + return -EBUSY; > + } > + list_del(&page->list); > + page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE; > + } > + spin_unlock(&sgx_active_page_list_lock); Would a WARN_ONCE() make sense when SGX_EPC_PAGE_RECLAIMABLE is not set, or do we have a legit flow where this can happen? /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page() 2019-10-23 12:42 ` Jarkko Sakkinen @ 2019-10-23 15:19 ` Sean Christopherson 0 siblings, 0 replies; 13+ messages in thread From: Sean Christopherson @ 2019-10-23 15:19 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx On Wed, Oct 23, 2019 at 03:42:20PM +0300, Jarkko Sakkinen wrote: > On Tue, Oct 22, 2019 at 03:49:22PM -0700, Sean Christopherson wrote: > > Move the reclaim logic out of sgx_free_page() and into a standalone > > helper to avoid taking sgx_active_page_list_lock when the page is known > > to be unreclaimable, which is the vast majority of flows that free EPC > > pages. > > > > Movig reclaim logic to a separate function also eliminates any > > possibility of silently leaking a page because it is unexpectedly > > reclaimable (and being actively reclaimed). > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > > > I really don't like the sgx_unmark_...() name, but couldn't come up with > > anything better. Suggestions welcome... > > > > arch/x86/kernel/cpu/sgx/encl.c | 3 ++- > > arch/x86/kernel/cpu/sgx/main.c | 32 ++++++++----------------------- > > arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++++++ > > arch/x86/kernel/cpu/sgx/sgx.h | 3 ++- > > 4 files changed, 44 insertions(+), 26 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > > index 8045f1ddfd62..22186d89042a 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -474,9 +474,10 @@ void sgx_encl_destroy(struct sgx_encl *encl) > > * The page and its radix tree entry cannot be freed > > * if the page is being held by the reclaimer. > > */ > > - if (sgx_free_page(entry->epc_page)) > > + if (sgx_unmark_page_reclaimable(entry->epc_page)) > > continue; > > > > + sgx_free_page(entry->epc_page); > > encl->secs_child_cnt--; > > entry->epc_page = NULL; > > } > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index 8e7557d3ff03..cfd8480ef563 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -108,45 +108,29 @@ struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim) > > * sgx_free_page() - Free an EPC page > > * @page: pointer a previously allocated EPC page > > * > > - * EREMOVE an EPC page and insert it back to the list of free pages. If the > > - * page is reclaimable, delete it from the active page list. > > - * > > - * Return: > > - * 0 on success, > > - * -EBUSY if a reclaim is in progress > > + * EREMOVE an EPC page and insert it back to the list of free pages. The page > > + * must not be reclaimable. > > */ > > -int sgx_free_page(struct sgx_epc_page *page) > > +void sgx_free_page(struct sgx_epc_page *page) > > { > > struct sgx_epc_section *section = sgx_epc_section(page); > > int ret; > > > > /* > > - * Remove the page from the active list if necessary. If the page > > - * is actively being reclaimed, i.e. RECLAIMABLE is set but the > > - * page isn't on the active list, return -EBUSY as we can't free > > - * the page at this time since it is "owned" by the reclaimer. > > + * Don't take sgx_active_page_list_lock when asserting the page isn't > > + * reclaimable, missing a WARN in the very rare case is preferable to > > + * unnecessarily taking a global lock in the common case. > > */ > > - spin_lock(&sgx_active_page_list_lock); > > - if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) { > > - if (list_empty(&page->list)) { > > - spin_unlock(&sgx_active_page_list_lock); > > - return -EBUSY; > > - } > > - list_del(&page->list); > > - page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE; > > - } > > - spin_unlock(&sgx_active_page_list_lock); > > + WARN_ON_ONCE(page->desc & SGX_EPC_PAGE_RECLAIMABLE); > > > > ret = __eremove(sgx_epc_addr(page)); > > if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) > > - return -EIO; > > + return; > > > > spin_lock(§ion->lock); > > list_add_tail(&page->list, §ion->page_list); > > atomic_inc(&sgx_nr_free_pages); > > spin_unlock(§ion->lock); > > - > > - return 0; > > } > > > > static void __init sgx_free_epc_section(struct sgx_epc_section *section) > > diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c > > index 8067ce1915a4..e64c810883ec 100644 > > --- a/arch/x86/kernel/cpu/sgx/reclaim.c > > +++ b/arch/x86/kernel/cpu/sgx/reclaim.c > > @@ -125,6 +125,38 @@ void sgx_mark_page_reclaimable(struct sgx_epc_page *page) > > spin_unlock(&sgx_active_page_list_lock); > > } > > > > +/** > > + * sgx_unmark_page_reclaimable() - Remove a page from the reclaim list > > + * @page: EPC page > > + * > > + * Clear the reclaimable flag and remove the page from the active page list. > > + * > > + * Return: > > + * 0 on success, > > + * -EBUSY if the page is in the process of being reclaimed > > + */ > > +int sgx_unmark_page_reclaimable(struct sgx_epc_page *page) > > +{ > > + /* > > + * Remove the page from the active list if necessary. If the page > > + * is actively being reclaimed, i.e. RECLAIMABLE is set but the > > + * page isn't on the active list, return -EBUSY as we can't free > > + * the page at this time since it is "owned" by the reclaimer. > > + */ > > + spin_lock(&sgx_active_page_list_lock); > > + if (page->desc & SGX_EPC_PAGE_RECLAIMABLE) { > > + if (list_empty(&page->list)) { > > + spin_unlock(&sgx_active_page_list_lock); > > + return -EBUSY; > > + } > > + list_del(&page->list); > > + page->desc &= ~SGX_EPC_PAGE_RECLAIMABLE; > > + } > > + spin_unlock(&sgx_active_page_list_lock); > > Would a WARN_ONCE() make sense when SGX_EPC_PAGE_RECLAIMABLE is not set, > or do we have a legit flow where this can happen? No, there's a legit case. sgx_reclaim_pages() clears RECLAIMABLE when it can't get a reference to the enclave, i.e. the enclave is being released. That would incorrectly trigger the WARN when sgx_encl_destroy() frees all pages as part of sgx_encl_release(). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 2019-10-22 22:49 [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Sean Christopherson ` (2 preceding siblings ...) 2019-10-22 22:49 ` [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page() Sean Christopherson @ 2019-10-24 18:35 ` Jarkko Sakkinen 2019-10-24 20:22 ` Sean Christopherson 3 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2019-10-24 18:35 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx On Tue, Oct 22, 2019 at 03:49:19PM -0700, Sean Christopherson wrote: > Two small enchancements, and a rework of the sgx_free_page() split that > was sent in a previous series. > > Sean Christopherson (3): > x86/sgx: Update the free page count in a single operation > x86/sgx: Do not add in-use EPC page to the free page list > x86/sgx: Move reclaim logic out of sgx_free_page() > > arch/x86/kernel/cpu/sgx/encl.c | 3 ++- > arch/x86/kernel/cpu/sgx/main.c | 36 +++++++++---------------------- > arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++ > arch/x86/kernel/cpu/sgx/sgx.h | 3 ++- > 4 files changed, 46 insertions(+), 28 deletions(-) > > -- > 2.22.0 > I think these should be now merged. Always have to do a lot of tweaking to keep diff's straight with these three commits: 1. x86/sgx: Add functions to allocate and free EPC pages 2. x86/sgx: Linux Enclave Driver 3. x86/sgx: Add a page reclaimer If something is different the only reason is most likely that the difference is just side-product of the patch adjustment. I haven't made my own mods on purpose. /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 2019-10-24 18:35 ` [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Jarkko Sakkinen @ 2019-10-24 20:22 ` Sean Christopherson 2019-10-28 20:35 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Sean Christopherson @ 2019-10-24 20:22 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-sgx On Thu, Oct 24, 2019 at 09:35:32PM +0300, Jarkko Sakkinen wrote: > On Tue, Oct 22, 2019 at 03:49:19PM -0700, Sean Christopherson wrote: > > Two small enchancements, and a rework of the sgx_free_page() split that > > was sent in a previous series. > > > > Sean Christopherson (3): > > x86/sgx: Update the free page count in a single operation > > x86/sgx: Do not add in-use EPC page to the free page list > > x86/sgx: Move reclaim logic out of sgx_free_page() > > > > arch/x86/kernel/cpu/sgx/encl.c | 3 ++- > > arch/x86/kernel/cpu/sgx/main.c | 36 +++++++++---------------------- > > arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++ > > arch/x86/kernel/cpu/sgx/sgx.h | 3 ++- > > 4 files changed, 46 insertions(+), 28 deletions(-) > > > > -- > > 2.22.0 > > > > I think these should be now merged. Always have to do a lot of tweaking > to keep diff's straight with these three commits: > > 1. x86/sgx: Add functions to allocate and free EPC pages > 2. x86/sgx: Linux Enclave Driver > 3. x86/sgx: Add a page reclaimer > > If something is different the only reason is most likely that the > difference is just side-product of the patch adjustment. I haven't > made my own mods on purpose. Diff looks good and smoke test ran clean. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 2019-10-24 20:22 ` Sean Christopherson @ 2019-10-28 20:35 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2019-10-28 20:35 UTC (permalink / raw) To: Sean Christopherson; +Cc: linux-sgx On Thu, Oct 24, 2019 at 01:22:22PM -0700, Sean Christopherson wrote: > On Thu, Oct 24, 2019 at 09:35:32PM +0300, Jarkko Sakkinen wrote: > > On Tue, Oct 22, 2019 at 03:49:19PM -0700, Sean Christopherson wrote: > > > Two small enchancements, and a rework of the sgx_free_page() split that > > > was sent in a previous series. > > > > > > Sean Christopherson (3): > > > x86/sgx: Update the free page count in a single operation > > > x86/sgx: Do not add in-use EPC page to the free page list > > > x86/sgx: Move reclaim logic out of sgx_free_page() > > > > > > arch/x86/kernel/cpu/sgx/encl.c | 3 ++- > > > arch/x86/kernel/cpu/sgx/main.c | 36 +++++++++---------------------- > > > arch/x86/kernel/cpu/sgx/reclaim.c | 32 +++++++++++++++++++++++++++ > > > arch/x86/kernel/cpu/sgx/sgx.h | 3 ++- > > > 4 files changed, 46 insertions(+), 28 deletions(-) > > > > > > -- > > > 2.22.0 > > > > > > > I think these should be now merged. Always have to do a lot of tweaking > > to keep diff's straight with these three commits: > > > > 1. x86/sgx: Add functions to allocate and free EPC pages > > 2. x86/sgx: Linux Enclave Driver > > 3. x86/sgx: Add a page reclaimer > > > > If something is different the only reason is most likely that the > > difference is just side-product of the patch adjustment. I haven't > > made my own mods on purpose. > > Diff looks good and smoke test ran clean. Great! /Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-10-28 20:35 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-22 22:49 [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Sean Christopherson 2019-10-22 22:49 ` [PATCH for_v23 1/3] x86/sgx: Update the free page count in a single operation Sean Christopherson 2019-10-23 12:44 ` Jarkko Sakkinen 2019-10-24 13:11 ` Jarkko Sakkinen 2019-10-22 22:49 ` [PATCH for_v23 2/3] x86/sgx: Do not add in-use EPC page to the free page list Sean Christopherson 2019-10-23 12:43 ` Jarkko Sakkinen 2019-10-23 15:23 ` Sean Christopherson 2019-10-22 22:49 ` [PATCH for_v23 3/3] x86/sgx: Move reclaim logic out of sgx_free_page() Sean Christopherson 2019-10-23 12:42 ` Jarkko Sakkinen 2019-10-23 15:19 ` Sean Christopherson 2019-10-24 18:35 ` [PATCH for_v23 0/3] x86/sgx: More cleanup for v23 Jarkko Sakkinen 2019-10-24 20:22 ` Sean Christopherson 2019-10-28 20:35 ` Jarkko Sakkinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.