3:21 a.m.
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(a)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
3:53 a.m.
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(a)lists.01.org] On Behalf Of Kai Huang
Sent: Thursday, March 30, 2017 13:21
To: intel-sgx-kernel-dev(a)lists.01.org
Cc: Sakkinen, Jarkko <jarkko.sakkinen(a)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(a)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(a)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.
7:13 a.m.
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(a)lists.01.org] On Behalf Of Kai Huang
> Sent: Thursday, March 30, 2017 13:21
> To: intel-sgx-kernel-dev(a)lists.01.org
> Cc: Sakkinen, Jarkko <jarkko.sakkinen(a)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(a)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(a)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(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
8:04 a.m.
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(a)intel.com>; Kai Huang
<kaih.linux(a)gmail.com>; intel-sgx-kernel-dev(a)lists.01.org
Cc: Sakkinen, Jarkko <jarkko.sakkinen(a)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(a)lists.01.org] On Behalf Of Kai Huang
>> Sent: Thursday, March 30, 2017 13:21
>> To: intel-sgx-kernel-dev(a)lists.01.org
>> Cc: Sakkinen, Jarkko <jarkko.sakkinen(a)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(a)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(a)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(a)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.
12:57 a.m.
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(a)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(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
1:01 a.m.
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(a)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
1:25 a.m.
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(a)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(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
1:33 a.m.
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(a)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
1:41 a.m.
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(a)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
1911
days inactive
1918
days old
intel-sgx-kernel-dev@lists.01.org
8 comments
4 participants
participants (4)
-
Ayoun, Serge -
Huang, Kai -
Jarkko Sakkinen -
Kai Huang