| Message ID | 20170330102127.21672-1-kai.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
The code was not ready for having several eviction threads and the fix is ok, but ultimately there should be one eviction thread per EPC bank. > -----Original Message----- > From: intel-sgx-kernel-dev [mailto:intel-sgx-kernel-dev- > bounces@lists.01.org] On Behalf Of Kai Huang > Sent: Thursday, March 30, 2017 13:21 > To: intel-sgx-kernel-dev@lists.01.org > Cc: Sakkinen, Jarkko <jarkko.sakkinen@intel.com> > Subject: [intel-sgx-kernel-dev] [PATCH] intel_sgx: refine page cache > initialization for multiple EPC banks > > Currently sgx_page_cache_init is called for each EPC bank in case of > multiple EPC banks, which is wrong because sgx_page_cache_init creates > ksgxswapd kernel thread which should only be created once in global. This > patch refines page cache initlization code by introducing sgx_epc_bank_init > which initializes one EPC bank, and sgx_page_cache_init is changed to take > all EPC banks as parameter and initialize them one by one by calling > sgx_epc_bank_init. And ksgxswapd will be created once in > sgx_page_cache_init. ioremap for EPC bank is also moved to > sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address > handling is also part of page cache management code. > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > --- > drivers/platform/x86/intel_sgx.h | 4 +- > drivers/platform/x86/intel_sgx_main.c | 40 +++------------- > drivers/platform/x86/intel_sgx_page_cache.c | 71 > ++++++++++++++++++++++++++--- > 3 files changed, 73 insertions(+), 42 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx.h > b/drivers/platform/x86/intel_sgx.h > index adb5b17..a98915a 100644 > --- a/drivers/platform/x86/intel_sgx.h > +++ b/drivers/platform/x86/intel_sgx.h > @@ -252,8 +252,8 @@ enum sgx_free_flags { }; > > int ksgxswapd(void *p); > -int sgx_page_cache_init(resource_size_t start, unsigned long size); -void > sgx_page_cache_teardown(void); > +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); void > +sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); > struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, > unsigned int flags); > int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, diff -- > git a/drivers/platform/x86/intel_sgx_main.c > b/drivers/platform/x86/intel_sgx_main.c > index 6c2d240..e242969 100644 > --- a/drivers/platform/x86/intel_sgx_main.c > +++ b/drivers/platform/x86/intel_sgx_main.c > @@ -264,7 +264,6 @@ static int sgx_dev_init(struct device *dev) { > unsigned int wq_flags; > int ret; > - int i; > > pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n"); > > @@ -277,25 +276,9 @@ static int sgx_dev_init(struct device *dev) > > pr_info("intel_sgx: Number of EPCs %d\n", sgx_nr_epc_banks); > > - for (i = 0; i < sgx_nr_epc_banks; i++) { > - pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", > - sgx_epc_banks[i].start, sgx_epc_banks[i].end); > -#ifdef CONFIG_X86_64 > - sgx_epc_banks[i].mem = > ioremap_cache(sgx_epc_banks[i].start, > - sgx_epc_banks[i].end - sgx_epc_banks[i].start); > - if (!sgx_epc_banks[i].mem) { > - sgx_nr_epc_banks = i; > - ret = -ENOMEM; > - goto out_iounmap; > - } > -#endif > - ret = sgx_page_cache_init(sgx_epc_banks[i].start, > - sgx_epc_banks[i].end - sgx_epc_banks[i].start); > - if (ret) { > - sgx_nr_epc_banks = i+1; > - goto out_iounmap; > - } > - } > + ret = sgx_page_cache_init(sgx_epc_banks, sgx_nr_epc_banks); > + if (ret) > + return ret; > > wq_flags = WQ_UNBOUND | WQ_FREEZABLE; > #ifdef WQ_NON_REENETRANT > @@ -305,7 +288,7 @@ static int sgx_dev_init(struct device *dev) > if (!sgx_add_page_wq) { > pr_err("intel_sgx: alloc_workqueue() failed\n"); > ret = -ENOMEM; > - goto out_iounmap; > + goto out_page_cache_init; > } > > sgx_dev.parent = dev; > @@ -318,11 +301,8 @@ static int sgx_dev_init(struct device *dev) > return 0; > out_workqueue: > destroy_workqueue(sgx_add_page_wq); > -out_iounmap: > -#ifdef CONFIG_X86_64 > - for (i = 0; i < sgx_nr_epc_banks; i++) > - iounmap(sgx_epc_banks[i].mem); > -#endif > +out_page_cache_init: > + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); > return ret; > } > > @@ -378,15 +358,9 @@ static int sgx_drv_probe(struct platform_device > *pdev) > > static int sgx_drv_remove(struct platform_device *pdev) { > - int i; > - > misc_deregister(&sgx_dev); > destroy_workqueue(sgx_add_page_wq); > -#ifdef CONFIG_X86_64 > - for (i = 0; i < sgx_nr_epc_banks; i++) > - iounmap(sgx_epc_banks[i].mem); > -#endif > - sgx_page_cache_teardown(); > + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); > > return 0; > } > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c > b/drivers/platform/x86/intel_sgx_page_cache.c > index 852066c..5d87666 100644 > --- a/drivers/platform/x86/intel_sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > @@ -420,17 +420,31 @@ int ksgxswapd(void *p) > return 0; > } > > -int sgx_page_cache_init(resource_size_t start, unsigned long size) > +static int sgx_epc_bank_init(struct sgx_epc_bank *bank) > { > - unsigned long i; > + unsigned long i, size; > struct sgx_epc_page *new_epc_page, *entry; > struct list_head *parser, *temp; > > + pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", > + bank->start, bank->end); > + > + if (!bank->start || !bank->end) > + return -ENODEV; > + > + size = bank->end - bank->start; > + > +#ifdef CONFIG_X86_64 > + bank->mem = ioremap_cache(bank->start, size); > + if (!bank->mem) > + return -ENOMEM; > +#endif > + > for (i = 0; i < size; i += PAGE_SIZE) { > new_epc_page = kzalloc(sizeof(*new_epc_page), > GFP_KERNEL); > if (!new_epc_page) > goto err_freelist; > - new_epc_page->pa = start + i; > + new_epc_page->pa = bank->start + i; > > spin_lock(&sgx_free_list_lock); > list_add_tail(&new_epc_page->free_list, &sgx_free_list); > @@ -439,10 +453,8 @@ int sgx_page_cache_init(resource_size_t start, > unsigned long size) > spin_unlock(&sgx_free_list_lock); > } > > - sgx_nr_high_pages = 2 * sgx_nr_low_pages; > - ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); > - > return 0; > + > err_freelist: > list_for_each_safe(parser, temp, &sgx_free_list) { > spin_lock(&sgx_free_list_lock); > @@ -451,17 +463,62 @@ int sgx_page_cache_init(resource_size_t start, > unsigned long size) > spin_unlock(&sgx_free_list_lock); > kfree(entry); > } > +#ifdef CONFIG_X86_64 > + iounmap(bank->mem); > +#endif > return -ENOMEM; > } > > -void sgx_page_cache_teardown(void) > +static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank) { > + if (!bank->start || !bank->end) > + return; > + > + /* > + * Don't free 'struct sgx_epc_page' for EPC page in this bank. All > + * 'struct sgx_epc_page' for all EPC pages will be destroyed together. > + */ > +#ifdef CONFIG_X86_64 > + if (bank->mem) > + iounmap(bank->mem); > +#endif > +} > + > +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks) { > + int i, ret = -ENODEV; > + > + for (i = 0; i < nr_banks; i++) { > + ret = sgx_epc_bank_init(banks + i); > + if (ret) > + break; > + } > + > + if (!i) > + return ret; > + > + /* > + * We can continue to use EPC banks that have been initialized > + * successfully, even we fail on initializing one particular EPC bank. > + */ > + sgx_nr_high_pages = 2 * sgx_nr_low_pages; > + ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); > + > + return 0; > +} > + > +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks) > { > struct sgx_epc_page *entry; > struct list_head *parser, *temp; > + int i; > > if (ksgxswapd_tsk) > kthread_stop(ksgxswapd_tsk); > > + for (i = 0; i < nr_banks; i++) > + sgx_epc_bank_teardown(banks + i); > + > 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); > -- > 2.9.3 > > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 3/30/2017 11:53 PM, Ayoun, Serge wrote: > The code was not ready for having several eviction threads and the fix is ok, but ultimately there should be one eviction > thread per EPC bank. Would you elaborate? Based on current implementation I don't see why there should be one eviction thread per EPC bank. Are you considering NUMA EPC case? Thanks, -Kai > >> -----Original Message----- >> From: intel-sgx-kernel-dev [mailto:intel-sgx-kernel-dev- >> bounces@lists.01.org] On Behalf Of Kai Huang >> Sent: Thursday, March 30, 2017 13:21 >> To: intel-sgx-kernel-dev@lists.01.org >> Cc: Sakkinen, Jarkko <jarkko.sakkinen@intel.com> >> Subject: [intel-sgx-kernel-dev] [PATCH] intel_sgx: refine page cache >> initialization for multiple EPC banks >> >> Currently sgx_page_cache_init is called for each EPC bank in case of >> multiple EPC banks, which is wrong because sgx_page_cache_init creates >> ksgxswapd kernel thread which should only be created once in global. This >> patch refines page cache initlization code by introducing sgx_epc_bank_init >> which initializes one EPC bank, and sgx_page_cache_init is changed to take >> all EPC banks as parameter and initialize them one by one by calling >> sgx_epc_bank_init. And ksgxswapd will be created once in >> sgx_page_cache_init. ioremap for EPC bank is also moved to >> sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address >> handling is also part of page cache management code. >> >> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> >> --- >> drivers/platform/x86/intel_sgx.h | 4 +- >> drivers/platform/x86/intel_sgx_main.c | 40 +++------------- >> drivers/platform/x86/intel_sgx_page_cache.c | 71 >> ++++++++++++++++++++++++++--- >> 3 files changed, 73 insertions(+), 42 deletions(-) >> >> diff --git a/drivers/platform/x86/intel_sgx.h >> b/drivers/platform/x86/intel_sgx.h >> index adb5b17..a98915a 100644 >> --- a/drivers/platform/x86/intel_sgx.h >> +++ b/drivers/platform/x86/intel_sgx.h >> @@ -252,8 +252,8 @@ enum sgx_free_flags { }; >> >> int ksgxswapd(void *p); >> -int sgx_page_cache_init(resource_size_t start, unsigned long size); -void >> sgx_page_cache_teardown(void); >> +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); void >> +sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); >> struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, >> unsigned int flags); >> int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, diff -- >> git a/drivers/platform/x86/intel_sgx_main.c >> b/drivers/platform/x86/intel_sgx_main.c >> index 6c2d240..e242969 100644 >> --- a/drivers/platform/x86/intel_sgx_main.c >> +++ b/drivers/platform/x86/intel_sgx_main.c >> @@ -264,7 +264,6 @@ static int sgx_dev_init(struct device *dev) { >> unsigned int wq_flags; >> int ret; >> - int i; >> >> pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n"); >> >> @@ -277,25 +276,9 @@ static int sgx_dev_init(struct device *dev) >> >> pr_info("intel_sgx: Number of EPCs %d\n", sgx_nr_epc_banks); >> >> - for (i = 0; i < sgx_nr_epc_banks; i++) { >> - pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", >> - sgx_epc_banks[i].start, sgx_epc_banks[i].end); >> -#ifdef CONFIG_X86_64 >> - sgx_epc_banks[i].mem = >> ioremap_cache(sgx_epc_banks[i].start, >> - sgx_epc_banks[i].end - sgx_epc_banks[i].start); >> - if (!sgx_epc_banks[i].mem) { >> - sgx_nr_epc_banks = i; >> - ret = -ENOMEM; >> - goto out_iounmap; >> - } >> -#endif >> - ret = sgx_page_cache_init(sgx_epc_banks[i].start, >> - sgx_epc_banks[i].end - sgx_epc_banks[i].start); >> - if (ret) { >> - sgx_nr_epc_banks = i+1; >> - goto out_iounmap; >> - } >> - } >> + ret = sgx_page_cache_init(sgx_epc_banks, sgx_nr_epc_banks); >> + if (ret) >> + return ret; >> >> wq_flags = WQ_UNBOUND | WQ_FREEZABLE; >> #ifdef WQ_NON_REENETRANT >> @@ -305,7 +288,7 @@ static int sgx_dev_init(struct device *dev) >> if (!sgx_add_page_wq) { >> pr_err("intel_sgx: alloc_workqueue() failed\n"); >> ret = -ENOMEM; >> - goto out_iounmap; >> + goto out_page_cache_init; >> } >> >> sgx_dev.parent = dev; >> @@ -318,11 +301,8 @@ static int sgx_dev_init(struct device *dev) >> return 0; >> out_workqueue: >> destroy_workqueue(sgx_add_page_wq); >> -out_iounmap: >> -#ifdef CONFIG_X86_64 >> - for (i = 0; i < sgx_nr_epc_banks; i++) >> - iounmap(sgx_epc_banks[i].mem); >> -#endif >> +out_page_cache_init: >> + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); >> return ret; >> } >> >> @@ -378,15 +358,9 @@ static int sgx_drv_probe(struct platform_device >> *pdev) >> >> static int sgx_drv_remove(struct platform_device *pdev) { >> - int i; >> - >> misc_deregister(&sgx_dev); >> destroy_workqueue(sgx_add_page_wq); >> -#ifdef CONFIG_X86_64 >> - for (i = 0; i < sgx_nr_epc_banks; i++) >> - iounmap(sgx_epc_banks[i].mem); >> -#endif >> - sgx_page_cache_teardown(); >> + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); >> >> return 0; >> } >> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c >> b/drivers/platform/x86/intel_sgx_page_cache.c >> index 852066c..5d87666 100644 >> --- a/drivers/platform/x86/intel_sgx_page_cache.c >> +++ b/drivers/platform/x86/intel_sgx_page_cache.c >> @@ -420,17 +420,31 @@ int ksgxswapd(void *p) >> return 0; >> } >> >> -int sgx_page_cache_init(resource_size_t start, unsigned long size) >> +static int sgx_epc_bank_init(struct sgx_epc_bank *bank) >> { >> - unsigned long i; >> + unsigned long i, size; >> struct sgx_epc_page *new_epc_page, *entry; >> struct list_head *parser, *temp; >> >> + pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", >> + bank->start, bank->end); >> + >> + if (!bank->start || !bank->end) >> + return -ENODEV; >> + >> + size = bank->end - bank->start; >> + >> +#ifdef CONFIG_X86_64 >> + bank->mem = ioremap_cache(bank->start, size); >> + if (!bank->mem) >> + return -ENOMEM; >> +#endif >> + >> for (i = 0; i < size; i += PAGE_SIZE) { >> new_epc_page = kzalloc(sizeof(*new_epc_page), >> GFP_KERNEL); >> if (!new_epc_page) >> goto err_freelist; >> - new_epc_page->pa = start + i; >> + new_epc_page->pa = bank->start + i; >> >> spin_lock(&sgx_free_list_lock); >> list_add_tail(&new_epc_page->free_list, &sgx_free_list); >> @@ -439,10 +453,8 @@ int sgx_page_cache_init(resource_size_t start, >> unsigned long size) >> spin_unlock(&sgx_free_list_lock); >> } >> >> - sgx_nr_high_pages = 2 * sgx_nr_low_pages; >> - ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); >> - >> return 0; >> + >> err_freelist: >> list_for_each_safe(parser, temp, &sgx_free_list) { >> spin_lock(&sgx_free_list_lock); >> @@ -451,17 +463,62 @@ int sgx_page_cache_init(resource_size_t start, >> unsigned long size) >> spin_unlock(&sgx_free_list_lock); >> kfree(entry); >> } >> +#ifdef CONFIG_X86_64 >> + iounmap(bank->mem); >> +#endif >> return -ENOMEM; >> } >> >> -void sgx_page_cache_teardown(void) >> +static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank) { >> + if (!bank->start || !bank->end) >> + return; >> + >> + /* >> + * Don't free 'struct sgx_epc_page' for EPC page in this bank. All >> + * 'struct sgx_epc_page' for all EPC pages will be destroyed together. >> + */ >> +#ifdef CONFIG_X86_64 >> + if (bank->mem) >> + iounmap(bank->mem); >> +#endif >> +} >> + >> +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks) { >> + int i, ret = -ENODEV; >> + >> + for (i = 0; i < nr_banks; i++) { >> + ret = sgx_epc_bank_init(banks + i); >> + if (ret) >> + break; >> + } >> + >> + if (!i) >> + return ret; >> + >> + /* >> + * We can continue to use EPC banks that have been initialized >> + * successfully, even we fail on initializing one particular EPC bank. >> + */ >> + sgx_nr_high_pages = 2 * sgx_nr_low_pages; >> + ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); >> + >> + return 0; >> +} >> + >> +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks) >> { >> struct sgx_epc_page *entry; >> struct list_head *parser, *temp; >> + int i; >> >> if (ksgxswapd_tsk) >> kthread_stop(ksgxswapd_tsk); >> >> + for (i = 0; i < nr_banks; i++) >> + sgx_epc_bank_teardown(banks + i); >> + >> 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); >> -- >> 2.9.3 >> >> _______________________________________________ >> intel-sgx-kernel-dev mailing list >> intel-sgx-kernel-dev@lists.01.org >> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev >
Right for numa configuration > -----Original Message----- > From: Huang, Kai [mailto:kai.huang@linux.intel.com] > Sent: Thursday, March 30, 2017 17:14 > To: Ayoun, Serge <serge.ayoun@intel.com>; Kai Huang > <kaih.linux@gmail.com>; intel-sgx-kernel-dev@lists.01.org > Cc: Sakkinen, Jarkko <jarkko.sakkinen@intel.com> > Subject: Re: [intel-sgx-kernel-dev] [PATCH] intel_sgx: refine page cache > initialization for multiple EPC banks > > > > On 3/30/2017 11:53 PM, Ayoun, Serge wrote: > > The code was not ready for having several eviction threads and the fix > > is ok, but ultimately there should be one eviction thread per EPC bank. > > Would you elaborate? Based on current implementation I don't see why > there should be one eviction thread per EPC bank. Are you considering > NUMA EPC case? > > Thanks, > -Kai > > > > >> -----Original Message----- > >> From: intel-sgx-kernel-dev [mailto:intel-sgx-kernel-dev- > >> bounces@lists.01.org] On Behalf Of Kai Huang > >> Sent: Thursday, March 30, 2017 13:21 > >> To: intel-sgx-kernel-dev@lists.01.org > >> Cc: Sakkinen, Jarkko <jarkko.sakkinen@intel.com> > >> Subject: [intel-sgx-kernel-dev] [PATCH] intel_sgx: refine page cache > >> initialization for multiple EPC banks > >> > >> Currently sgx_page_cache_init is called for each EPC bank in case of > >> multiple EPC banks, which is wrong because sgx_page_cache_init > >> creates ksgxswapd kernel thread which should only be created once in > >> global. This patch refines page cache initlization code by > >> introducing sgx_epc_bank_init which initializes one EPC bank, and > >> sgx_page_cache_init is changed to take all EPC banks as parameter and > >> initialize them one by one by calling sgx_epc_bank_init. And > >> ksgxswapd will be created once in sgx_page_cache_init. ioremap for > >> EPC bank is also moved to sgx_epc_bank_init. This is more reasonable, > >> as EPC page's virtual address handling is also part of page cache > management code. > >> > >> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > >> --- > >> drivers/platform/x86/intel_sgx.h | 4 +- > >> drivers/platform/x86/intel_sgx_main.c | 40 +++------------- > >> drivers/platform/x86/intel_sgx_page_cache.c | 71 > >> ++++++++++++++++++++++++++--- > >> 3 files changed, 73 insertions(+), 42 deletions(-) > >> > >> diff --git a/drivers/platform/x86/intel_sgx.h > >> b/drivers/platform/x86/intel_sgx.h > >> index adb5b17..a98915a 100644 > >> --- a/drivers/platform/x86/intel_sgx.h > >> +++ b/drivers/platform/x86/intel_sgx.h > >> @@ -252,8 +252,8 @@ enum sgx_free_flags { }; > >> > >> int ksgxswapd(void *p); > >> -int sgx_page_cache_init(resource_size_t start, unsigned long size); > >> -void sgx_page_cache_teardown(void); > >> +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); > >> +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int > >> +nr_banks); > >> struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, > >> unsigned int flags); > >> int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, > >> diff -- git a/drivers/platform/x86/intel_sgx_main.c > >> b/drivers/platform/x86/intel_sgx_main.c > >> index 6c2d240..e242969 100644 > >> --- a/drivers/platform/x86/intel_sgx_main.c > >> +++ b/drivers/platform/x86/intel_sgx_main.c > >> @@ -264,7 +264,6 @@ static int sgx_dev_init(struct device *dev) { > >> unsigned int wq_flags; > >> int ret; > >> - int i; > >> > >> pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n"); > >> > >> @@ -277,25 +276,9 @@ static int sgx_dev_init(struct device *dev) > >> > >> pr_info("intel_sgx: Number of EPCs %d\n", sgx_nr_epc_banks); > >> > >> - for (i = 0; i < sgx_nr_epc_banks; i++) { > >> - pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", > >> - sgx_epc_banks[i].start, sgx_epc_banks[i].end); > >> -#ifdef CONFIG_X86_64 > >> - sgx_epc_banks[i].mem = > >> ioremap_cache(sgx_epc_banks[i].start, > >> - sgx_epc_banks[i].end - sgx_epc_banks[i].start); > >> - if (!sgx_epc_banks[i].mem) { > >> - sgx_nr_epc_banks = i; > >> - ret = -ENOMEM; > >> - goto out_iounmap; > >> - } > >> -#endif > >> - ret = sgx_page_cache_init(sgx_epc_banks[i].start, > >> - sgx_epc_banks[i].end - sgx_epc_banks[i].start); > >> - if (ret) { > >> - sgx_nr_epc_banks = i+1; > >> - goto out_iounmap; > >> - } > >> - } > >> + ret = sgx_page_cache_init(sgx_epc_banks, sgx_nr_epc_banks); > >> + if (ret) > >> + return ret; > >> > >> wq_flags = WQ_UNBOUND | WQ_FREEZABLE; #ifdef > WQ_NON_REENETRANT @@ > >> -305,7 +288,7 @@ static int sgx_dev_init(struct device *dev) > >> if (!sgx_add_page_wq) { > >> pr_err("intel_sgx: alloc_workqueue() failed\n"); > >> ret = -ENOMEM; > >> - goto out_iounmap; > >> + goto out_page_cache_init; > >> } > >> > >> sgx_dev.parent = dev; > >> @@ -318,11 +301,8 @@ static int sgx_dev_init(struct device *dev) > >> return 0; > >> out_workqueue: > >> destroy_workqueue(sgx_add_page_wq); > >> -out_iounmap: > >> -#ifdef CONFIG_X86_64 > >> - for (i = 0; i < sgx_nr_epc_banks; i++) > >> - iounmap(sgx_epc_banks[i].mem); > >> -#endif > >> +out_page_cache_init: > >> + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); > >> return ret; > >> } > >> > >> @@ -378,15 +358,9 @@ static int sgx_drv_probe(struct platform_device > >> *pdev) > >> > >> static int sgx_drv_remove(struct platform_device *pdev) { > >> - int i; > >> - > >> misc_deregister(&sgx_dev); > >> destroy_workqueue(sgx_add_page_wq); > >> -#ifdef CONFIG_X86_64 > >> - for (i = 0; i < sgx_nr_epc_banks; i++) > >> - iounmap(sgx_epc_banks[i].mem); > >> -#endif > >> - sgx_page_cache_teardown(); > >> + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); > >> > >> return 0; > >> } > >> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c > >> b/drivers/platform/x86/intel_sgx_page_cache.c > >> index 852066c..5d87666 100644 > >> --- a/drivers/platform/x86/intel_sgx_page_cache.c > >> +++ b/drivers/platform/x86/intel_sgx_page_cache.c > >> @@ -420,17 +420,31 @@ int ksgxswapd(void *p) > >> return 0; > >> } > >> > >> -int sgx_page_cache_init(resource_size_t start, unsigned long size) > >> +static int sgx_epc_bank_init(struct sgx_epc_bank *bank) > >> { > >> - unsigned long i; > >> + unsigned long i, size; > >> struct sgx_epc_page *new_epc_page, *entry; > >> struct list_head *parser, *temp; > >> > >> + pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", > >> + bank->start, bank->end); > >> + > >> + if (!bank->start || !bank->end) > >> + return -ENODEV; > >> + > >> + size = bank->end - bank->start; > >> + > >> +#ifdef CONFIG_X86_64 > >> + bank->mem = ioremap_cache(bank->start, size); > >> + if (!bank->mem) > >> + return -ENOMEM; > >> +#endif > >> + > >> for (i = 0; i < size; i += PAGE_SIZE) { > >> new_epc_page = kzalloc(sizeof(*new_epc_page), > GFP_KERNEL); > >> if (!new_epc_page) > >> goto err_freelist; > >> - new_epc_page->pa = start + i; > >> + new_epc_page->pa = bank->start + i; > >> > >> spin_lock(&sgx_free_list_lock); > >> list_add_tail(&new_epc_page->free_list, &sgx_free_list); > >> @@ -439,10 +453,8 @@ int sgx_page_cache_init(resource_size_t start, > >> unsigned long size) > >> spin_unlock(&sgx_free_list_lock); > >> } > >> > >> - sgx_nr_high_pages = 2 * sgx_nr_low_pages; > >> - ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); > >> - > >> return 0; > >> + > >> err_freelist: > >> list_for_each_safe(parser, temp, &sgx_free_list) { > >> spin_lock(&sgx_free_list_lock); > >> @@ -451,17 +463,62 @@ int sgx_page_cache_init(resource_size_t start, > >> unsigned long size) > >> spin_unlock(&sgx_free_list_lock); > >> kfree(entry); > >> } > >> +#ifdef CONFIG_X86_64 > >> + iounmap(bank->mem); > >> +#endif > >> return -ENOMEM; > >> } > >> > >> -void sgx_page_cache_teardown(void) > >> +static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank) { > >> + if (!bank->start || !bank->end) > >> + return; > >> + > >> + /* > >> + * Don't free 'struct sgx_epc_page' for EPC page in this bank. All > >> + * 'struct sgx_epc_page' for all EPC pages will be destroyed together. > >> + */ > >> +#ifdef CONFIG_X86_64 > >> + if (bank->mem) > >> + iounmap(bank->mem); > >> +#endif > >> +} > >> + > >> +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks) { > >> + int i, ret = -ENODEV; > >> + > >> + for (i = 0; i < nr_banks; i++) { > >> + ret = sgx_epc_bank_init(banks + i); > >> + if (ret) > >> + break; > >> + } > >> + > >> + if (!i) > >> + return ret; > >> + > >> + /* > >> + * We can continue to use EPC banks that have been initialized > >> + * successfully, even we fail on initializing one particular EPC bank. > >> + */ > >> + sgx_nr_high_pages = 2 * sgx_nr_low_pages; > >> + ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); > >> + > >> + return 0; > >> +} > >> + > >> +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int > nr_banks) > >> { > >> struct sgx_epc_page *entry; > >> struct list_head *parser, *temp; > >> + int i; > >> > >> if (ksgxswapd_tsk) > >> kthread_stop(ksgxswapd_tsk); > >> > >> + for (i = 0; i < nr_banks; i++) > >> + sgx_epc_bank_teardown(banks + i); > >> + > >> 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); > >> -- > >> 2.9.3 > >> > >> _______________________________________________ > >> intel-sgx-kernel-dev mailing list > >> intel-sgx-kernel-dev@lists.01.org > >> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev > > --------------------------------------------------------------------- > > Intel Israel (74) Limited > > > > This e-mail and any attachments may contain confidential material for > > the sole use of the intended recipient(s). Any review or distribution > > by others is strictly prohibited. If you are not the intended > > recipient, please contact the sender and delete all copies. > > > > _______________________________________________ > > intel-sgx-kernel-dev mailing list > > intel-sgx-kernel-dev@lists.01.org > > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev > > --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Thu, Mar 30, 2017 at 11:21:27PM +1300, Kai Huang wrote: > Currently sgx_page_cache_init is called for each EPC bank in case of multiple > EPC banks, which is wrong because sgx_page_cache_init creates ksgxswapd kernel > thread which should only be created once in global. This patch refines page > cache initlization code by introducing sgx_epc_bank_init which initializes one > EPC bank, and sgx_page_cache_init is changed to take all EPC banks as parameter > and initialize them one by one by calling sgx_epc_bank_init. And ksgxswapd will > be created once in sgx_page_cache_init. ioremap for EPC bank is also moved to > sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address > handling is also part of page cache management code. > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> So why not instead move kthread initialization out of the sgx_page_cache_init? Just a question. Haven't yet looked this in detail. Thanks. /Jarkko > --- > drivers/platform/x86/intel_sgx.h | 4 +- > drivers/platform/x86/intel_sgx_main.c | 40 +++------------- > drivers/platform/x86/intel_sgx_page_cache.c | 71 ++++++++++++++++++++++++++--- > 3 files changed, 73 insertions(+), 42 deletions(-) > > diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h > index adb5b17..a98915a 100644 > --- a/drivers/platform/x86/intel_sgx.h > +++ b/drivers/platform/x86/intel_sgx.h > @@ -252,8 +252,8 @@ enum sgx_free_flags { > }; > > int ksgxswapd(void *p); > -int sgx_page_cache_init(resource_size_t start, unsigned long size); > -void sgx_page_cache_teardown(void); > +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); > +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); > struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, > unsigned int flags); > int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, > diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c > index 6c2d240..e242969 100644 > --- a/drivers/platform/x86/intel_sgx_main.c > +++ b/drivers/platform/x86/intel_sgx_main.c > @@ -264,7 +264,6 @@ static int sgx_dev_init(struct device *dev) > { > unsigned int wq_flags; > int ret; > - int i; > > pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n"); > > @@ -277,25 +276,9 @@ static int sgx_dev_init(struct device *dev) > > pr_info("intel_sgx: Number of EPCs %d\n", sgx_nr_epc_banks); > > - for (i = 0; i < sgx_nr_epc_banks; i++) { > - pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", > - sgx_epc_banks[i].start, sgx_epc_banks[i].end); > -#ifdef CONFIG_X86_64 > - sgx_epc_banks[i].mem = ioremap_cache(sgx_epc_banks[i].start, > - sgx_epc_banks[i].end - sgx_epc_banks[i].start); > - if (!sgx_epc_banks[i].mem) { > - sgx_nr_epc_banks = i; > - ret = -ENOMEM; > - goto out_iounmap; > - } > -#endif > - ret = sgx_page_cache_init(sgx_epc_banks[i].start, > - sgx_epc_banks[i].end - sgx_epc_banks[i].start); > - if (ret) { > - sgx_nr_epc_banks = i+1; > - goto out_iounmap; > - } > - } > + ret = sgx_page_cache_init(sgx_epc_banks, sgx_nr_epc_banks); > + if (ret) > + return ret; > > wq_flags = WQ_UNBOUND | WQ_FREEZABLE; > #ifdef WQ_NON_REENETRANT > @@ -305,7 +288,7 @@ static int sgx_dev_init(struct device *dev) > if (!sgx_add_page_wq) { > pr_err("intel_sgx: alloc_workqueue() failed\n"); > ret = -ENOMEM; > - goto out_iounmap; > + goto out_page_cache_init; > } > > sgx_dev.parent = dev; > @@ -318,11 +301,8 @@ static int sgx_dev_init(struct device *dev) > return 0; > out_workqueue: > destroy_workqueue(sgx_add_page_wq); > -out_iounmap: > -#ifdef CONFIG_X86_64 > - for (i = 0; i < sgx_nr_epc_banks; i++) > - iounmap(sgx_epc_banks[i].mem); > -#endif > +out_page_cache_init: > + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); > return ret; > } > > @@ -378,15 +358,9 @@ static int sgx_drv_probe(struct platform_device *pdev) > > static int sgx_drv_remove(struct platform_device *pdev) > { > - int i; > - > misc_deregister(&sgx_dev); > destroy_workqueue(sgx_add_page_wq); > -#ifdef CONFIG_X86_64 > - for (i = 0; i < sgx_nr_epc_banks; i++) > - iounmap(sgx_epc_banks[i].mem); > -#endif > - sgx_page_cache_teardown(); > + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); > > return 0; > } > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c > index 852066c..5d87666 100644 > --- a/drivers/platform/x86/intel_sgx_page_cache.c > +++ b/drivers/platform/x86/intel_sgx_page_cache.c > @@ -420,17 +420,31 @@ int ksgxswapd(void *p) > return 0; > } > > -int sgx_page_cache_init(resource_size_t start, unsigned long size) > +static int sgx_epc_bank_init(struct sgx_epc_bank *bank) > { > - unsigned long i; > + unsigned long i, size; > struct sgx_epc_page *new_epc_page, *entry; > struct list_head *parser, *temp; > > + pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", > + bank->start, bank->end); > + > + if (!bank->start || !bank->end) > + return -ENODEV; > + > + size = bank->end - bank->start; > + > +#ifdef CONFIG_X86_64 > + bank->mem = ioremap_cache(bank->start, size); > + if (!bank->mem) > + return -ENOMEM; > +#endif > + > for (i = 0; i < size; i += PAGE_SIZE) { > new_epc_page = kzalloc(sizeof(*new_epc_page), GFP_KERNEL); > if (!new_epc_page) > goto err_freelist; > - new_epc_page->pa = start + i; > + new_epc_page->pa = bank->start + i; > > spin_lock(&sgx_free_list_lock); > list_add_tail(&new_epc_page->free_list, &sgx_free_list); > @@ -439,10 +453,8 @@ int sgx_page_cache_init(resource_size_t start, unsigned long size) > spin_unlock(&sgx_free_list_lock); > } > > - sgx_nr_high_pages = 2 * sgx_nr_low_pages; > - ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); > - > return 0; > + > err_freelist: > list_for_each_safe(parser, temp, &sgx_free_list) { > spin_lock(&sgx_free_list_lock); > @@ -451,17 +463,62 @@ int sgx_page_cache_init(resource_size_t start, unsigned long size) > spin_unlock(&sgx_free_list_lock); > kfree(entry); > } > +#ifdef CONFIG_X86_64 > + iounmap(bank->mem); > +#endif > return -ENOMEM; > } > > -void sgx_page_cache_teardown(void) > +static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank) > +{ > + if (!bank->start || !bank->end) > + return; > + > + /* > + * Don't free 'struct sgx_epc_page' for EPC page in this bank. All > + * 'struct sgx_epc_page' for all EPC pages will be destroyed together. > + */ > +#ifdef CONFIG_X86_64 > + if (bank->mem) > + iounmap(bank->mem); > +#endif > +} > + > +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks) > +{ > + int i, ret = -ENODEV; > + > + for (i = 0; i < nr_banks; i++) { > + ret = sgx_epc_bank_init(banks + i); > + if (ret) > + break; > + } > + > + if (!i) > + return ret; > + > + /* > + * We can continue to use EPC banks that have been initialized > + * successfully, even we fail on initializing one particular EPC bank. > + */ > + sgx_nr_high_pages = 2 * sgx_nr_low_pages; > + ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); > + > + return 0; > +} > + > +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks) > { > struct sgx_epc_page *entry; > struct list_head *parser, *temp; > + int i; > > if (ksgxswapd_tsk) > kthread_stop(ksgxswapd_tsk); > > + for (i = 0; i < nr_banks; i++) > + sgx_epc_bank_teardown(banks + i); > + > 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); > -- > 2.9.3 > > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
On Wed, Apr 05, 2017 at 10:57:47AM +0300, Jarkko Sakkinen wrote: > On Thu, Mar 30, 2017 at 11:21:27PM +1300, Kai Huang wrote: > > Currently sgx_page_cache_init is called for each EPC bank in case of multiple > > EPC banks, which is wrong because sgx_page_cache_init creates ksgxswapd kernel > > thread which should only be created once in global. This patch refines page > > cache initlization code by introducing sgx_epc_bank_init which initializes one > > EPC bank, and sgx_page_cache_init is changed to take all EPC banks as parameter > > and initialize them one by one by calling sgx_epc_bank_init. And ksgxswapd will > > be created once in sgx_page_cache_init. ioremap for EPC bank is also moved to > > sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address > > handling is also part of page cache management code. > > > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > > So why not instead move kthread initialization out of the > sgx_page_cache_init? Just a question. Haven't yet looked this in > detail. Thanks. Now you it looks like that you are turning things to a different form (which is not in any way cleaner) in order to fix a simple issue where thread is created in a wrong place. If that is the only reason for this, I will rather just move the thread creation. If there is a pragmatic reason for doing this, please explain it. /Jarkko
On 4/5/2017 8:01 PM, Jarkko Sakkinen wrote: > On Wed, Apr 05, 2017 at 10:57:47AM +0300, Jarkko Sakkinen wrote: >> On Thu, Mar 30, 2017 at 11:21:27PM +1300, Kai Huang wrote: >>> Currently sgx_page_cache_init is called for each EPC bank in case of multiple >>> EPC banks, which is wrong because sgx_page_cache_init creates ksgxswapd kernel >>> thread which should only be created once in global. This patch refines page >>> cache initlization code by introducing sgx_epc_bank_init which initializes one >>> EPC bank, and sgx_page_cache_init is changed to take all EPC banks as parameter >>> and initialize them one by one by calling sgx_epc_bank_init. And ksgxswapd will >>> be created once in sgx_page_cache_init. ioremap for EPC bank is also moved to >>> sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address >>> handling is also part of page cache management code. >>> >>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> >> >> So why not instead move kthread initialization out of the >> sgx_page_cache_init? Just a question. Haven't yet looked this in >> detail. Thanks. > > Now you it looks like that you are turning things to a different > form (which is not in any way cleaner) in order to fix a simple > issue where thread is created in a wrong place. > > If that is the only reason for this, I will rather just move the > thread creation. If there is a pragmatic reason for doing this, > please explain it. Moving kthread creation out is fine. There's no other pragmatic reason for doing this. I did this just because I think this patch makes code more clear. Ex, currently sgx_page_cache_init is called for each bank but sgx_page_cache_teardown is only called once. They are supposed to be called in pair although pragmatically calling sgx_page_cache_teardown once has no issue. I'll drop this patch and rewrite my series for supporting 'unified EPC' based on current code. Thanks, -Kai > > /Jarkko > _______________________________________________ > intel-sgx-kernel-dev mailing list > intel-sgx-kernel-dev@lists.01.org > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev >
On Wed, Apr 05, 2017 at 08:25:52PM +1200, Huang, Kai wrote: > > > On 4/5/2017 8:01 PM, Jarkko Sakkinen wrote: > > On Wed, Apr 05, 2017 at 10:57:47AM +0300, Jarkko Sakkinen wrote: > > > On Thu, Mar 30, 2017 at 11:21:27PM +1300, Kai Huang wrote: > > > > Currently sgx_page_cache_init is called for each EPC bank in case of multiple > > > > EPC banks, which is wrong because sgx_page_cache_init creates ksgxswapd kernel > > > > thread which should only be created once in global. This patch refines page > > > > cache initlization code by introducing sgx_epc_bank_init which initializes one > > > > EPC bank, and sgx_page_cache_init is changed to take all EPC banks as parameter > > > > and initialize them one by one by calling sgx_epc_bank_init. And ksgxswapd will > > > > be created once in sgx_page_cache_init. ioremap for EPC bank is also moved to > > > > sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address > > > > handling is also part of page cache management code. > > > > > > > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > > > > > > So why not instead move kthread initialization out of the > > > sgx_page_cache_init? Just a question. Haven't yet looked this in > > > detail. Thanks. > > > > Now you it looks like that you are turning things to a different > > form (which is not in any way cleaner) in order to fix a simple > > issue where thread is created in a wrong place. > > > > If that is the only reason for this, I will rather just move the > > thread creation. If there is a pragmatic reason for doing this, > > please explain it. > > Moving kthread creation out is fine. There's no other pragmatic reason for > doing this. I did this just because I think this patch makes code more > clear. Ex, currently sgx_page_cache_init is called for each bank but > sgx_page_cache_teardown is only called once. They are supposed to be called > in pair although pragmatically calling sgx_page_cache_teardown once has no > issue. I'll drop this patch and rewrite my series for supporting 'unified > EPC' based on current code. > > Thanks, > -Kai Thank you for spotting this out in any case. It's a regression that was unnoticed in the original commit. I do not want to radically change the driver code before upstreaming it. It always adds risk of new regressions. Minimal changes, like putting allocation functions to arch/x86/include/asm/sgx.h, are absolutely fine in order to help you to move forward with KVM work. /Jarkko
On 4/5/2017 8:33 PM, Jarkko Sakkinen wrote: > On Wed, Apr 05, 2017 at 08:25:52PM +1200, Huang, Kai wrote: >> >> >> On 4/5/2017 8:01 PM, Jarkko Sakkinen wrote: >>> On Wed, Apr 05, 2017 at 10:57:47AM +0300, Jarkko Sakkinen wrote: >>>> On Thu, Mar 30, 2017 at 11:21:27PM +1300, Kai Huang wrote: >>>>> Currently sgx_page_cache_init is called for each EPC bank in case of multiple >>>>> EPC banks, which is wrong because sgx_page_cache_init creates ksgxswapd kernel >>>>> thread which should only be created once in global. This patch refines page >>>>> cache initlization code by introducing sgx_epc_bank_init which initializes one >>>>> EPC bank, and sgx_page_cache_init is changed to take all EPC banks as parameter >>>>> and initialize them one by one by calling sgx_epc_bank_init. And ksgxswapd will >>>>> be created once in sgx_page_cache_init. ioremap for EPC bank is also moved to >>>>> sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address >>>>> handling is also part of page cache management code. >>>>> >>>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> >>>> >>>> So why not instead move kthread initialization out of the >>>> sgx_page_cache_init? Just a question. Haven't yet looked this in >>>> detail. Thanks. >>> >>> Now you it looks like that you are turning things to a different >>> form (which is not in any way cleaner) in order to fix a simple >>> issue where thread is created in a wrong place. >>> >>> If that is the only reason for this, I will rather just move the >>> thread creation. If there is a pragmatic reason for doing this, >>> please explain it. >> >> Moving kthread creation out is fine. There's no other pragmatic reason for >> doing this. I did this just because I think this patch makes code more >> clear. Ex, currently sgx_page_cache_init is called for each bank but >> sgx_page_cache_teardown is only called once. They are supposed to be called >> in pair although pragmatically calling sgx_page_cache_teardown once has no >> issue. I'll drop this patch and rewrite my series for supporting 'unified >> EPC' based on current code. >> >> Thanks, >> -Kai > > Thank you for spotting this out in any case. It's a regression that was > unnoticed in the original commit. > > I do not want to radically change the driver code before upstreaming > it. It always adds risk of new regressions. Minimal changes, like > putting allocation functions to arch/x86/include/asm/sgx.h, are > absolutely fine in order to help you to move forward with KVM work. Understood and thanks. As I said I'll drop unnecessary patches and rewrite the series based on current driver code. Thanks, -Kai > > /Jarkko >
diff --git a/drivers/platform/x86/intel_sgx.h b/drivers/platform/x86/intel_sgx.h index adb5b17..a98915a 100644 --- a/drivers/platform/x86/intel_sgx.h +++ b/drivers/platform/x86/intel_sgx.h @@ -252,8 +252,8 @@ enum sgx_free_flags { }; int ksgxswapd(void *p); -int sgx_page_cache_init(resource_size_t start, unsigned long size); -void sgx_page_cache_teardown(void); +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks); +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks); struct sgx_epc_page *sgx_alloc_page(struct sgx_tgid_ctx *tgid_epc_cnt, unsigned int flags); int sgx_free_page(struct sgx_epc_page *entry, struct sgx_encl *encl, diff --git a/drivers/platform/x86/intel_sgx_main.c b/drivers/platform/x86/intel_sgx_main.c index 6c2d240..e242969 100644 --- a/drivers/platform/x86/intel_sgx_main.c +++ b/drivers/platform/x86/intel_sgx_main.c @@ -264,7 +264,6 @@ static int sgx_dev_init(struct device *dev) { unsigned int wq_flags; int ret; - int i; pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION "\n"); @@ -277,25 +276,9 @@ static int sgx_dev_init(struct device *dev) pr_info("intel_sgx: Number of EPCs %d\n", sgx_nr_epc_banks); - for (i = 0; i < sgx_nr_epc_banks; i++) { - pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", - sgx_epc_banks[i].start, sgx_epc_banks[i].end); -#ifdef CONFIG_X86_64 - sgx_epc_banks[i].mem = ioremap_cache(sgx_epc_banks[i].start, - sgx_epc_banks[i].end - sgx_epc_banks[i].start); - if (!sgx_epc_banks[i].mem) { - sgx_nr_epc_banks = i; - ret = -ENOMEM; - goto out_iounmap; - } -#endif - ret = sgx_page_cache_init(sgx_epc_banks[i].start, - sgx_epc_banks[i].end - sgx_epc_banks[i].start); - if (ret) { - sgx_nr_epc_banks = i+1; - goto out_iounmap; - } - } + ret = sgx_page_cache_init(sgx_epc_banks, sgx_nr_epc_banks); + if (ret) + return ret; wq_flags = WQ_UNBOUND | WQ_FREEZABLE; #ifdef WQ_NON_REENETRANT @@ -305,7 +288,7 @@ static int sgx_dev_init(struct device *dev) if (!sgx_add_page_wq) { pr_err("intel_sgx: alloc_workqueue() failed\n"); ret = -ENOMEM; - goto out_iounmap; + goto out_page_cache_init; } sgx_dev.parent = dev; @@ -318,11 +301,8 @@ static int sgx_dev_init(struct device *dev) return 0; out_workqueue: destroy_workqueue(sgx_add_page_wq); -out_iounmap: -#ifdef CONFIG_X86_64 - for (i = 0; i < sgx_nr_epc_banks; i++) - iounmap(sgx_epc_banks[i].mem); -#endif +out_page_cache_init: + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); return ret; } @@ -378,15 +358,9 @@ static int sgx_drv_probe(struct platform_device *pdev) static int sgx_drv_remove(struct platform_device *pdev) { - int i; - misc_deregister(&sgx_dev); destroy_workqueue(sgx_add_page_wq); -#ifdef CONFIG_X86_64 - for (i = 0; i < sgx_nr_epc_banks; i++) - iounmap(sgx_epc_banks[i].mem); -#endif - sgx_page_cache_teardown(); + sgx_page_cache_teardown(sgx_epc_banks, sgx_nr_epc_banks); return 0; } diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c index 852066c..5d87666 100644 --- a/drivers/platform/x86/intel_sgx_page_cache.c +++ b/drivers/platform/x86/intel_sgx_page_cache.c @@ -420,17 +420,31 @@ int ksgxswapd(void *p) return 0; } -int sgx_page_cache_init(resource_size_t start, unsigned long size) +static int sgx_epc_bank_init(struct sgx_epc_bank *bank) { - unsigned long i; + unsigned long i, size; struct sgx_epc_page *new_epc_page, *entry; struct list_head *parser, *temp; + pr_info("intel_sgx: EPC memory range 0x%lx-0x%lx\n", + bank->start, bank->end); + + if (!bank->start || !bank->end) + return -ENODEV; + + size = bank->end - bank->start; + +#ifdef CONFIG_X86_64 + bank->mem = ioremap_cache(bank->start, size); + if (!bank->mem) + return -ENOMEM; +#endif + for (i = 0; i < size; i += PAGE_SIZE) { new_epc_page = kzalloc(sizeof(*new_epc_page), GFP_KERNEL); if (!new_epc_page) goto err_freelist; - new_epc_page->pa = start + i; + new_epc_page->pa = bank->start + i; spin_lock(&sgx_free_list_lock); list_add_tail(&new_epc_page->free_list, &sgx_free_list); @@ -439,10 +453,8 @@ int sgx_page_cache_init(resource_size_t start, unsigned long size) spin_unlock(&sgx_free_list_lock); } - sgx_nr_high_pages = 2 * sgx_nr_low_pages; - ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); - return 0; + err_freelist: list_for_each_safe(parser, temp, &sgx_free_list) { spin_lock(&sgx_free_list_lock); @@ -451,17 +463,62 @@ int sgx_page_cache_init(resource_size_t start, unsigned long size) spin_unlock(&sgx_free_list_lock); kfree(entry); } +#ifdef CONFIG_X86_64 + iounmap(bank->mem); +#endif return -ENOMEM; } -void sgx_page_cache_teardown(void) +static void sgx_epc_bank_teardown(struct sgx_epc_bank *bank) +{ + if (!bank->start || !bank->end) + return; + + /* + * Don't free 'struct sgx_epc_page' for EPC page in this bank. All + * 'struct sgx_epc_page' for all EPC pages will be destroyed together. + */ +#ifdef CONFIG_X86_64 + if (bank->mem) + iounmap(bank->mem); +#endif +} + +int sgx_page_cache_init(struct sgx_epc_bank *banks, int nr_banks) +{ + int i, ret = -ENODEV; + + for (i = 0; i < nr_banks; i++) { + ret = sgx_epc_bank_init(banks + i); + if (ret) + break; + } + + if (!i) + return ret; + + /* + * We can continue to use EPC banks that have been initialized + * successfully, even we fail on initializing one particular EPC bank. + */ + sgx_nr_high_pages = 2 * sgx_nr_low_pages; + ksgxswapd_tsk = kthread_run(ksgxswapd, NULL, "ksgxswapd"); + + return 0; +} + +void sgx_page_cache_teardown(struct sgx_epc_bank *banks, int nr_banks) { struct sgx_epc_page *entry; struct list_head *parser, *temp; + int i; if (ksgxswapd_tsk) kthread_stop(ksgxswapd_tsk); + for (i = 0; i < nr_banks; i++) + sgx_epc_bank_teardown(banks + i); + 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);
Currently sgx_page_cache_init is called for each EPC bank in case of multiple EPC banks, which is wrong because sgx_page_cache_init creates ksgxswapd kernel thread which should only be created once in global. This patch refines page cache initlization code by introducing sgx_epc_bank_init which initializes one EPC bank, and sgx_page_cache_init is changed to take all EPC banks as parameter and initialize them one by one by calling sgx_epc_bank_init. And ksgxswapd will be created once in sgx_page_cache_init. ioremap for EPC bank is also moved to sgx_epc_bank_init. This is more reasonable, as EPC page's virtual address handling is also part of page cache management code. Signed-off-by: Kai Huang <kai.huang@linux.intel.com> --- drivers/platform/x86/intel_sgx.h | 4 +- drivers/platform/x86/intel_sgx_main.c | 40 +++------------- drivers/platform/x86/intel_sgx_page_cache.c | 71 ++++++++++++++++++++++++++--- 3 files changed, 73 insertions(+), 42 deletions(-)