10:56 a.m.
Move the SGX code into /arch/x86/kernel/cpu and remove the option
to build the SGX subsystem as a loadable module. Allowing SGX to
be built as a module complicates the implemenation (code and
upstreaming process) of future SGX extensions, e.g. KVM support
and EPC cgroup.
Sean Christopherson (2):
intel_sgx: move files to arch/x86/kernel/cpu
intel_sgx: remove option to build as a module
arch/x86/Kconfig | 20
+++++++++++++++
arch/x86/kernel/cpu/Makefile | 1 +
arch/x86/kernel/cpu/intel_sgx/Makefile | 9 +++++++
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c | 74
+++++++++++++-----------------------------------------
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c | 0
drivers/platform/x86/Kconfig | 2 --
drivers/platform/x86/Makefile | 1 -
drivers/platform/x86/intel_sgx/Kconfig | 19
--------------
drivers/platform/x86/intel_sgx/Makefile | 12 ---------
13 files changed, 47 insertions(+), 91 deletions(-)
10:56 a.m.
New subject: [PATCH 1/2] intel_sgx: move files to arch/x86/kernel/cpu
Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com>
---
arch/x86/Kconfig | 20 ++++++++++++++++++++
arch/x86/kernel/cpu/Makefile | 1 +
.../x86 => arch/x86/kernel/cpu}/intel_sgx/Makefile | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h | 0
.../x86/kernel/cpu}/intel_sgx/sgx_ioctl.c | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c | 0
.../x86/kernel/cpu}/intel_sgx/sgx_page_cache.c | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c | 0
drivers/platform/x86/Kconfig | 2 --
drivers/platform/x86/Makefile | 1 -
drivers/platform/x86/intel_sgx/Kconfig | 19 -------------------
12 files changed, 21 insertions(+), 22 deletions(-)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/Makefile (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c
(100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c (100%)
delete mode 100644 drivers/platform/x86/intel_sgx/Kconfig
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0efb4c9..fb8c91f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1787,6 +1787,26 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
If unsure, say y.
+config INTEL_SGX
+ tristate "Intel(R) SGX Driver"
+ default n
+ depends on X86
+ select MMU_NOTIFIER
+ ---help---
+ Intel(R) SGX is a set of CPU instructions that can be used by
+ applications to set aside private regions of code and data. The
+ code outside the enclave is disallowed to access the memory inside
+ the enclave by the CPU access control.
+
+ The firmware uses PRMRR registers to reserve an area of physical
+ memory called Enclave Page Cache (EPC). There is a hardware unit in
+ the processor called Memory Encryption Engine. The MEE encrypts and
+ decrypts the EPC pages as they enter and leave the processor package.
+
+ For details, see Documentation/x86/intel_sgx.rst
+
+ If unsure, say N.
+
config EFI
bool "EFI runtime service support"
depends on ACPI
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 5200001..490fd60 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_CPU_SUP_UMC_32) += umc.o
obj-$(CONFIG_INTEL_RDT_A) += intel_rdt.o intel_rdt_rdtgroup.o intel_rdt_schemata.o
+obj-$(CONFIG_INTEL_SGX) += intel_sgx/
obj-$(CONFIG_X86_MCE) += mcheck/
obj-$(CONFIG_MTRR) += mtrr/
obj-$(CONFIG_MICROCODE) += microcode/
diff --git a/drivers/platform/x86/intel_sgx/Makefile
b/arch/x86/kernel/cpu/intel_sgx/Makefile
similarity index 100%
rename from drivers/platform/x86/intel_sgx/Makefile
rename to arch/x86/kernel/cpu/intel_sgx/Makefile
diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/arch/x86/kernel/cpu/intel_sgx/sgx.h
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx.h
rename to arch/x86/kernel/cpu/intel_sgx/sgx.h
diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_ioctl.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_ioctl.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_ioctl.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_main.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_main.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_page_cache.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_page_cache.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_page_cache.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_util.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_util.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_util.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_vma.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_vma.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_vma.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_vma.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 6c217ac..8489020 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1110,8 +1110,6 @@ config SILEAD_DMI
with the OS-image for the device. This option supplies the missing
information. Enable this for x86 tablets with Silead touchscreens.
-source "drivers/platform/x86/intel_sgx/Kconfig"
-
endif # X86_PLATFORM_DEVICES
config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index aec6e53..182a3ed 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -80,4 +80,3 @@ obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_MLX_CPLD_PLATFORM) += mlxcpld-hotplug.o
obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
-obj-$(CONFIG_INTEL_SGX) += intel_sgx/
diff --git a/drivers/platform/x86/intel_sgx/Kconfig
b/drivers/platform/x86/intel_sgx/Kconfig
deleted file mode 100644
index 9641728..0000000
--- a/drivers/platform/x86/intel_sgx/Kconfig
+++ /dev/null
@@ -1,19 +0,0 @@
-#
-# Intel SGX
-#
-
-config INTEL_SGX
- tristate "Intel(R) SGX Driver"
- default n
- depends on X86
- select MMU_NOTIFIER
- ---help---
- Intel(R) SGX is a set of CPU instructions that can be used by
- applications to set aside private regions of code and data. The code
- outside the enclave is disallowed to access the memory inside the
- enclave by the CPU access control.
-
- The firmware uses PRMRR registers to reserve an area of physical memory
- called Enclave Page Cache (EPC). There is a hardware unit in the
- processor called Memory Encryption Engine. The MEE encrypts and decrypts
- the EPC pages as they enter and leave the processor package.
--
2.7.4
11:45 a.m.
New subject: [PATCH 1/2] intel_sgx: move files to arch/x86/kernel/cpu
I'll do a more proper review to explain my view.
On Tue, Aug 29, 2017 at 10:56:35AM -0700, Sean Christopherson wrote:
Signed-off-by: Sean Christopherson
<sean.j.christopherson(a)intel.com>
The commit message is missing.
/Jarkko
---
arch/x86/Kconfig | 20 ++++++++++++++++++++
arch/x86/kernel/cpu/Makefile | 1 +
.../x86 => arch/x86/kernel/cpu}/intel_sgx/Makefile | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h | 0
.../x86/kernel/cpu}/intel_sgx/sgx_ioctl.c | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c | 0
.../x86/kernel/cpu}/intel_sgx/sgx_page_cache.c | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c | 0
.../x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c | 0
drivers/platform/x86/Kconfig | 2 --
drivers/platform/x86/Makefile | 1 -
drivers/platform/x86/intel_sgx/Kconfig | 19 -------------------
12 files changed, 21 insertions(+), 22 deletions(-)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/Makefile (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c
(100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c (100%)
rename {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c (100%)
delete mode 100644 drivers/platform/x86/intel_sgx/Kconfig
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0efb4c9..fb8c91f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1787,6 +1787,26 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
If unsure, say y.
+config INTEL_SGX
+ tristate "Intel(R) SGX Driver"
+ default n
+ depends on X86
+ select MMU_NOTIFIER
+ ---help---
+ Intel(R) SGX is a set of CPU instructions that can be used by
+ applications to set aside private regions of code and data. The
+ code outside the enclave is disallowed to access the memory inside
+ the enclave by the CPU access control.
+
+ The firmware uses PRMRR registers to reserve an area of physical
+ memory called Enclave Page Cache (EPC). There is a hardware unit in
+ the processor called Memory Encryption Engine. The MEE encrypts and
+ decrypts the EPC pages as they enter and leave the processor package.
+
+ For details, see Documentation/x86/intel_sgx.rst
+
+ If unsure, say N.
+
config EFI
bool "EFI runtime service support"
depends on ACPI
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 5200001..490fd60 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_CPU_SUP_UMC_32) += umc.o
obj-$(CONFIG_INTEL_RDT_A) += intel_rdt.o intel_rdt_rdtgroup.o intel_rdt_schemata.o
+obj-$(CONFIG_INTEL_SGX) += intel_sgx/
obj-$(CONFIG_X86_MCE) += mcheck/
obj-$(CONFIG_MTRR) += mtrr/
obj-$(CONFIG_MICROCODE) += microcode/
diff --git a/drivers/platform/x86/intel_sgx/Makefile
b/arch/x86/kernel/cpu/intel_sgx/Makefile
similarity index 100%
rename from drivers/platform/x86/intel_sgx/Makefile
rename to arch/x86/kernel/cpu/intel_sgx/Makefile
diff --git a/drivers/platform/x86/intel_sgx/sgx.h b/arch/x86/kernel/cpu/intel_sgx/sgx.h
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx.h
rename to arch/x86/kernel/cpu/intel_sgx/sgx.h
diff --git a/drivers/platform/x86/intel_sgx/sgx_ioctl.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_ioctl.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_ioctl.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_ioctl.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_main.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_main.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_page_cache.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_page_cache.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_page_cache.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_page_cache.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_util.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_util.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_util.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_util.c
diff --git a/drivers/platform/x86/intel_sgx/sgx_vma.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_vma.c
similarity index 100%
rename from drivers/platform/x86/intel_sgx/sgx_vma.c
rename to arch/x86/kernel/cpu/intel_sgx/sgx_vma.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 6c217ac..8489020 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1110,8 +1110,6 @@ config SILEAD_DMI
with the OS-image for the device. This option supplies the missing
information. Enable this for x86 tablets with Silead touchscreens.
-source "drivers/platform/x86/intel_sgx/Kconfig"
-
endif # X86_PLATFORM_DEVICES
config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index aec6e53..182a3ed 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -80,4 +80,3 @@ obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
obj-$(CONFIG_MLX_CPLD_PLATFORM) += mlxcpld-hotplug.o
obj-$(CONFIG_INTEL_TURBO_MAX_3) += intel_turbo_max_3.o
-obj-$(CONFIG_INTEL_SGX) += intel_sgx/
diff --git a/drivers/platform/x86/intel_sgx/Kconfig
b/drivers/platform/x86/intel_sgx/Kconfig
deleted file mode 100644
index 9641728..0000000
--- a/drivers/platform/x86/intel_sgx/Kconfig
+++ /dev/null
@@ -1,19 +0,0 @@
-#
-# Intel SGX
-#
-
-config INTEL_SGX
- tristate "Intel(R) SGX Driver"
- default n
- depends on X86
- select MMU_NOTIFIER
- ---help---
- Intel(R) SGX is a set of CPU instructions that can be used by
- applications to set aside private regions of code and data. The code
- outside the enclave is disallowed to access the memory inside the
- enclave by the CPU access control.
-
- The firmware uses PRMRR registers to reserve an area of physical memory
- called Enclave Page Cache (EPC). There is a hardware unit in the
- processor called Memory Encryption Engine. The MEE encrypts and decrypts
- the EPC pages as they enter and leave the processor package.
--
2.7.4
_______________________________________________
intel-sgx-kernel-dev mailing list
intel-sgx-kernel-dev(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
10:56 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
Build SGX support directly into the kernel, utilizing syscore_ops
instead of dev_pm_ops to hook into the suspend flow.
Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com>
---
arch/x86/Kconfig | 4 +-
arch/x86/kernel/cpu/intel_sgx/Makefile | 13 +++---
arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
3 files changed, 24 insertions(+), 67 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fb8c91f..5f4d414 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1788,8 +1788,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
If unsure, say y.
config INTEL_SGX
- tristate "Intel(R) SGX Driver"
- default n
+ prompt "Intel SGX (Secure Guard Extensions)"
+ def_bool n
depends on X86
select MMU_NOTIFIER
---help---
diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile
b/arch/x86/kernel/cpu/intel_sgx/Makefile
index b87f4e1..71b85b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/Makefile
+++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
@@ -2,11 +2,8 @@
# Intel SGX
#
-obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
-
-intel_sgx-$(CONFIG_INTEL_SGX) += \
- sgx_ioctl.o \
- sgx_main.o \
- sgx_page_cache.o \
- sgx_util.o \
- sgx_vma.o \
+obj-y += sgx_ioctl.o \
+ sgx_main.o \
+ sgx_page_cache.o \
+ sgx_util.o \
+ sgx_vma.o \
diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
index 50d9af8..5f411b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
+++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
@@ -59,22 +59,12 @@
*/
#include "sgx.h"
-#include <linux/acpi.h>
#include <linux/file.h>
#include <linux/highmem.h>
#include <linux/miscdevice.h>
-#include <linux/module.h>
-#include <linux/suspend.h>
#include <linux/hashtable.h>
#include <linux/kthread.h>
-#include <linux/platform_device.h>
-
-#define DRV_DESCRIPTION "Intel SGX Driver"
-#define DRV_VERSION "0.10"
-
-MODULE_DESCRIPTION(DRV_DESCRIPTION);
-MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen(a)linux.intel.com>");
-MODULE_VERSION(DRV_VERSION);
+#include <linux/syscore_ops.h>
/*
* Global data.
@@ -159,7 +149,7 @@ static struct miscdevice sgx_dev = {
.mode = 0666,
};
-static int sgx_pm_suspend(struct device *dev)
+static int sgx_pm_suspend(void)
{
struct sgx_tgid_ctx *ctx;
struct sgx_encl *encl;
@@ -175,9 +165,12 @@ static int sgx_pm_suspend(struct device *dev)
return 0;
}
-static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
-static int sgx_dev_init(struct device *dev)
+static struct syscore_ops sgx_syscore_ops = {
+ .suspend = sgx_pm_suspend,
+};
+
+static int __init sgx_dev_init(void)
{
unsigned int eax, ebx, ecx, edx;
unsigned long pa;
@@ -186,8 +179,6 @@ static int sgx_dev_init(struct device *dev)
int ret;
int i;
- pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION
"\n");
-
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
return -ENODEV;
@@ -199,7 +190,7 @@ static int sgx_dev_init(struct device *dev)
pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
- dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
+ pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
sgx_epc_banks[i].pa = pa;
sgx_epc_banks[i].size = size;
@@ -238,10 +229,11 @@ 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_pagecache;
}
- sgx_dev.parent = dev;
+ register_syscore_ops(&sgx_syscore_ops);
+
ret = misc_register(&sgx_dev);
if (ret) {
pr_err("intel_sgx: misc_register() failed\n");
@@ -250,7 +242,10 @@ static int sgx_dev_init(struct device *dev)
return 0;
out_workqueue:
+ unregister_syscore_ops(&sgx_syscore_ops);
destroy_workqueue(sgx_add_page_wq);
+out_pagecache:
+ sgx_page_cache_teardown();
out_iounmap:
#ifdef CONFIG_X86_64
for (i = 0; i < sgx_nr_epc_banks; i++)
@@ -259,7 +254,7 @@ static int sgx_dev_init(struct device *dev)
return ret;
}
-static int sgx_drv_probe(struct platform_device *pdev)
+static int __init sgx_init(void)
{
unsigned int eax, ebx, ecx, edx;
int i;
@@ -307,42 +302,7 @@ static int sgx_drv_probe(struct platform_device *pdev)
#endif
sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
- return sgx_dev_init(&pdev->dev);
+ return sgx_dev_init();
}
-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((void *)sgx_epc_banks[i].va);
-#endif
- sgx_page_cache_teardown();
-
- return 0;
-}
-
-#ifdef CONFIG_ACPI
-static struct acpi_device_id sgx_device_ids[] = {
- {"INT0E0C", 0},
- {"", 0},
-};
-MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
-#endif
-
-static struct platform_driver sgx_drv = {
- .probe = sgx_drv_probe,
- .remove = sgx_drv_remove,
- .driver = {
- .name = "intel_sgx",
- .pm = &sgx_drv_pm,
- .acpi_match_table = ACPI_PTR(sgx_device_ids),
- },
-};
-
-module_platform_driver(sgx_drv);
-
-MODULE_LICENSE("Dual BSD/GPL");
+device_initcall(sgx_init);
--
2.7.4
9:37 p.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Tue, 2017-08-29 at 10:56 -0700, Sean Christopherson wrote:
Build SGX support directly into the kernel, utilizing syscore_ops
instead of dev_pm_ops to hook into the suspend flow.
Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com>
---
arch/x86/Kconfig | 4 +-
arch/x86/kernel/cpu/intel_sgx/Makefile | 13 +++---
arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------
------------
3 files changed, 24 insertions(+), 67 deletions(-)
...
-
-module_platform_driver(sgx_drv);
-
-MODULE_LICENSE("Dual BSD/GPL");
+device_initcall(sgx_init);
As SGX is not treated as device anymore, maybe using arch_initcall, or
late_initcall would be better?
Using syscore_ops looks good to me.
Thanks,
-Kai
3:05 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
Not going to take this.
/Jarkko
On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
Build SGX support directly into the kernel, utilizing syscore_ops
instead of dev_pm_ops to hook into the suspend flow.
Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com>
---
arch/x86/Kconfig | 4 +-
arch/x86/kernel/cpu/intel_sgx/Makefile | 13 +++---
arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
3 files changed, 24 insertions(+), 67 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fb8c91f..5f4d414 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1788,8 +1788,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
If unsure, say y.
config INTEL_SGX
- tristate "Intel(R) SGX Driver"
- default n
+ prompt "Intel SGX (Secure Guard Extensions)"
+ def_bool n
depends on X86
select MMU_NOTIFIER
---help---
diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile
b/arch/x86/kernel/cpu/intel_sgx/Makefile
index b87f4e1..71b85b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/Makefile
+++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
@@ -2,11 +2,8 @@
# Intel SGX
#
-obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
-
-intel_sgx-$(CONFIG_INTEL_SGX) += \
- sgx_ioctl.o \
- sgx_main.o \
- sgx_page_cache.o \
- sgx_util.o \
- sgx_vma.o \
+obj-y += sgx_ioctl.o \
+ sgx_main.o \
+ sgx_page_cache.o \
+ sgx_util.o \
+ sgx_vma.o \
diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
index 50d9af8..5f411b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
+++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
@@ -59,22 +59,12 @@
*/
#include "sgx.h"
-#include <linux/acpi.h>
#include <linux/file.h>
#include <linux/highmem.h>
#include <linux/miscdevice.h>
-#include <linux/module.h>
-#include <linux/suspend.h>
#include <linux/hashtable.h>
#include <linux/kthread.h>
-#include <linux/platform_device.h>
-
-#define DRV_DESCRIPTION "Intel SGX Driver"
-#define DRV_VERSION "0.10"
-
-MODULE_DESCRIPTION(DRV_DESCRIPTION);
-MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen(a)linux.intel.com>");
-MODULE_VERSION(DRV_VERSION);
+#include <linux/syscore_ops.h>
/*
* Global data.
@@ -159,7 +149,7 @@ static struct miscdevice sgx_dev = {
.mode = 0666,
};
-static int sgx_pm_suspend(struct device *dev)
+static int sgx_pm_suspend(void)
{
struct sgx_tgid_ctx *ctx;
struct sgx_encl *encl;
@@ -175,9 +165,12 @@ static int sgx_pm_suspend(struct device *dev)
return 0;
}
-static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
-static int sgx_dev_init(struct device *dev)
+static struct syscore_ops sgx_syscore_ops = {
+ .suspend = sgx_pm_suspend,
+};
+
+static int __init sgx_dev_init(void)
{
unsigned int eax, ebx, ecx, edx;
unsigned long pa;
@@ -186,8 +179,6 @@ static int sgx_dev_init(struct device *dev)
int ret;
int i;
- pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION
"\n");
-
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
return -ENODEV;
@@ -199,7 +190,7 @@ static int sgx_dev_init(struct device *dev)
pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
- dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
+ pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
sgx_epc_banks[i].pa = pa;
sgx_epc_banks[i].size = size;
@@ -238,10 +229,11 @@ 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_pagecache;
}
- sgx_dev.parent = dev;
+ register_syscore_ops(&sgx_syscore_ops);
+
ret = misc_register(&sgx_dev);
if (ret) {
pr_err("intel_sgx: misc_register() failed\n");
@@ -250,7 +242,10 @@ static int sgx_dev_init(struct device *dev)
return 0;
out_workqueue:
+ unregister_syscore_ops(&sgx_syscore_ops);
destroy_workqueue(sgx_add_page_wq);
+out_pagecache:
+ sgx_page_cache_teardown();
out_iounmap:
#ifdef CONFIG_X86_64
for (i = 0; i < sgx_nr_epc_banks; i++)
@@ -259,7 +254,7 @@ static int sgx_dev_init(struct device *dev)
return ret;
}
-static int sgx_drv_probe(struct platform_device *pdev)
+static int __init sgx_init(void)
{
unsigned int eax, ebx, ecx, edx;
int i;
@@ -307,42 +302,7 @@ static int sgx_drv_probe(struct platform_device *pdev)
#endif
sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
- return sgx_dev_init(&pdev->dev);
+ return sgx_dev_init();
}
-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((void *)sgx_epc_banks[i].va);
-#endif
- sgx_page_cache_teardown();
-
- return 0;
-}
-
-#ifdef CONFIG_ACPI
-static struct acpi_device_id sgx_device_ids[] = {
- {"INT0E0C", 0},
- {"", 0},
-};
-MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
-#endif
-
-static struct platform_driver sgx_drv = {
- .probe = sgx_drv_probe,
- .remove = sgx_drv_remove,
- .driver = {
- .name = "intel_sgx",
- .pm = &sgx_drv_pm,
- .acpi_match_table = ACPI_PTR(sgx_device_ids),
- },
-};
-
-module_platform_driver(sgx_drv);
-
-MODULE_LICENSE("Dual BSD/GPL");
+device_initcall(sgx_init);
--
2.7.4
_______________________________________________
intel-sgx-kernel-dev mailing list
intel-sgx-kernel-dev(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
11:47 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
Build SGX support directly into the kernel, utilizing syscore_ops
instead of dev_pm_ops to hook into the suspend flow.
Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com>
I think you are looking this only from container perspective. For
desktop distribution it *does* make sense to be able to compile
this as a module.
/Jarkko
---
arch/x86/Kconfig | 4 +-
arch/x86/kernel/cpu/intel_sgx/Makefile | 13 +++---
arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
3 files changed, 24 insertions(+), 67 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fb8c91f..5f4d414 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1788,8 +1788,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
If unsure, say y.
config INTEL_SGX
- tristate "Intel(R) SGX Driver"
- default n
+ prompt "Intel SGX (Secure Guard Extensions)"
+ def_bool n
depends on X86
select MMU_NOTIFIER
---help---
diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile
b/arch/x86/kernel/cpu/intel_sgx/Makefile
index b87f4e1..71b85b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/Makefile
+++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
@@ -2,11 +2,8 @@
# Intel SGX
#
-obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
-
-intel_sgx-$(CONFIG_INTEL_SGX) += \
- sgx_ioctl.o \
- sgx_main.o \
- sgx_page_cache.o \
- sgx_util.o \
- sgx_vma.o \
+obj-y += sgx_ioctl.o \
+ sgx_main.o \
+ sgx_page_cache.o \
+ sgx_util.o \
+ sgx_vma.o \
diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
index 50d9af8..5f411b5 100644
--- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
+++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
@@ -59,22 +59,12 @@
*/
#include "sgx.h"
-#include <linux/acpi.h>
#include <linux/file.h>
#include <linux/highmem.h>
#include <linux/miscdevice.h>
-#include <linux/module.h>
-#include <linux/suspend.h>
#include <linux/hashtable.h>
#include <linux/kthread.h>
-#include <linux/platform_device.h>
-
-#define DRV_DESCRIPTION "Intel SGX Driver"
-#define DRV_VERSION "0.10"
-
-MODULE_DESCRIPTION(DRV_DESCRIPTION);
-MODULE_AUTHOR("Jarkko Sakkinen <jarkko.sakkinen(a)linux.intel.com>");
-MODULE_VERSION(DRV_VERSION);
+#include <linux/syscore_ops.h>
/*
* Global data.
@@ -159,7 +149,7 @@ static struct miscdevice sgx_dev = {
.mode = 0666,
};
-static int sgx_pm_suspend(struct device *dev)
+static int sgx_pm_suspend(void)
{
struct sgx_tgid_ctx *ctx;
struct sgx_encl *encl;
@@ -175,9 +165,12 @@ static int sgx_pm_suspend(struct device *dev)
return 0;
}
-static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
-static int sgx_dev_init(struct device *dev)
+static struct syscore_ops sgx_syscore_ops = {
+ .suspend = sgx_pm_suspend,
+};
+
+static int __init sgx_dev_init(void)
{
unsigned int eax, ebx, ecx, edx;
unsigned long pa;
@@ -186,8 +179,6 @@ static int sgx_dev_init(struct device *dev)
int ret;
int i;
- pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION
"\n");
-
if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
return -ENODEV;
@@ -199,7 +190,7 @@ static int sgx_dev_init(struct device *dev)
pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
- dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
+ pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
sgx_epc_banks[i].pa = pa;
sgx_epc_banks[i].size = size;
@@ -238,10 +229,11 @@ 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_pagecache;
}
- sgx_dev.parent = dev;
+ register_syscore_ops(&sgx_syscore_ops);
+
ret = misc_register(&sgx_dev);
if (ret) {
pr_err("intel_sgx: misc_register() failed\n");
@@ -250,7 +242,10 @@ static int sgx_dev_init(struct device *dev)
return 0;
out_workqueue:
+ unregister_syscore_ops(&sgx_syscore_ops);
destroy_workqueue(sgx_add_page_wq);
+out_pagecache:
+ sgx_page_cache_teardown();
out_iounmap:
#ifdef CONFIG_X86_64
for (i = 0; i < sgx_nr_epc_banks; i++)
@@ -259,7 +254,7 @@ static int sgx_dev_init(struct device *dev)
return ret;
}
-static int sgx_drv_probe(struct platform_device *pdev)
+static int __init sgx_init(void)
{
unsigned int eax, ebx, ecx, edx;
int i;
@@ -307,42 +302,7 @@ static int sgx_drv_probe(struct platform_device *pdev)
#endif
sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
- return sgx_dev_init(&pdev->dev);
+ return sgx_dev_init();
}
-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((void *)sgx_epc_banks[i].va);
-#endif
- sgx_page_cache_teardown();
-
- return 0;
-}
-
-#ifdef CONFIG_ACPI
-static struct acpi_device_id sgx_device_ids[] = {
- {"INT0E0C", 0},
- {"", 0},
-};
-MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
-#endif
-
-static struct platform_driver sgx_drv = {
- .probe = sgx_drv_probe,
- .remove = sgx_drv_remove,
- .driver = {
- .name = "intel_sgx",
- .pm = &sgx_drv_pm,
- .acpi_match_table = ACPI_PTR(sgx_device_ids),
- },
-};
-
-module_platform_driver(sgx_drv);
-
-MODULE_LICENSE("Dual BSD/GPL");
+device_initcall(sgx_init);
--
2.7.4
_______________________________________________
intel-sgx-kernel-dev mailing list
intel-sgx-kernel-dev(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
9:52 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> Build SGX support directly into the kernel, utilizing syscore_ops
> instead of dev_pm_ops to hook into the suspend flow.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com>
I think you are looking this only from container perspective. For
desktop distribution it *does* make sense to be able to compile
this as a module.
/Jarkko
I don't disagree that building SGX as module is valuable. Even for
server/container/VM environments it would be nice to have the driver
built as a loadable module.
The core issue is that taking a dependency on a loadable module is
Simply not possible for a cgroup, and while possible for KVM, is not
ideal. Various workarounds are possible, but they are either ugly,
i.e. will never be accepted upstream, or don't really solve the
problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
basically forces distros to decide between the cgroup and building
SGX as a module.
I think Kai's idea to split (some of) the EPC management and the
core functionality, e.g. programing FLC MSRs, from the userspace
facing driver is probably the right long term direction. This would
allow the EPC cgroup and KVM to take a dependency only on the core
SGX code. Though as Kai said, this is a fairly significant code
change. On the plus side, this change could be done after the
initial upstreaming of the SGX driver as it wouldn't be a breaking
change, e.g. distros wouldn't need to make changes to their kernel
configs.
If you're amenable (or at least not wholly opposed) to this idea,
I'll start working on a RFC patch.
> ---
> arch/x86/Kconfig | 4 +-
> arch/x86/kernel/cpu/intel_sgx/Makefile | 13 +++---
> arch/x86/kernel/cpu/intel_sgx/sgx_main.c | 74 ++++++++------------------------
> 3 files changed, 24 insertions(+), 67 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index fb8c91f..5f4d414 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1788,8 +1788,8 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS
> If unsure, say y.
>
> config INTEL_SGX
> - tristate "Intel(R) SGX Driver"
> - default n
> + prompt "Intel SGX (Secure Guard Extensions)"
> + def_bool n
> depends on X86
> select MMU_NOTIFIER
> ---help---
> diff --git a/arch/x86/kernel/cpu/intel_sgx/Makefile
b/arch/x86/kernel/cpu/intel_sgx/Makefile
> index b87f4e1..71b85b5 100644
> --- a/arch/x86/kernel/cpu/intel_sgx/Makefile
> +++ b/arch/x86/kernel/cpu/intel_sgx/Makefile
> @@ -2,11 +2,8 @@
> # Intel SGX
> #
>
> -obj-$(CONFIG_INTEL_SGX) += intel_sgx.o
> -
> -intel_sgx-$(CONFIG_INTEL_SGX) += \
> - sgx_ioctl.o \
> - sgx_main.o \
> - sgx_page_cache.o \
> - sgx_util.o \
> - sgx_vma.o \
> +obj-y += sgx_ioctl.o \
> + sgx_main.o \
> + sgx_page_cache.o \
> + sgx_util.o \
> + sgx_vma.o \
> diff --git a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> index 50d9af8..5f411b5 100644
> --- a/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> +++ b/arch/x86/kernel/cpu/intel_sgx/sgx_main.c
> @@ -59,22 +59,12 @@
> */
>
> #include "sgx.h"
> -#include <linux/acpi.h>
> #include <linux/file.h>
> #include <linux/highmem.h>
> #include <linux/miscdevice.h>
> -#include <linux/module.h>
> -#include <linux/suspend.h>
> #include <linux/hashtable.h>
> #include <linux/kthread.h>
> -#include <linux/platform_device.h>
> -
> -#define DRV_DESCRIPTION "Intel SGX Driver"
> -#define DRV_VERSION "0.10"
> -
> -MODULE_DESCRIPTION(DRV_DESCRIPTION);
> -MODULE_AUTHOR("Jarkko Sakkinen
<jarkko.sakkinen(a)linux.intel.com>");
> -MODULE_VERSION(DRV_VERSION);
> +#include <linux/syscore_ops.h>
>
> /*
> * Global data.
> @@ -159,7 +149,7 @@ static struct miscdevice sgx_dev = {
> .mode = 0666,
> };
>
> -static int sgx_pm_suspend(struct device *dev)
> +static int sgx_pm_suspend(void)
> {
> struct sgx_tgid_ctx *ctx;
> struct sgx_encl *encl;
> @@ -175,9 +165,12 @@ static int sgx_pm_suspend(struct device *dev)
> return 0;
> }
>
> -static SIMPLE_DEV_PM_OPS(sgx_drv_pm, sgx_pm_suspend, NULL);
>
> -static int sgx_dev_init(struct device *dev)
> +static struct syscore_ops sgx_syscore_ops = {
> + .suspend = sgx_pm_suspend,
> +};
> +
> +static int __init sgx_dev_init(void)
> {
> unsigned int eax, ebx, ecx, edx;
> unsigned long pa;
> @@ -186,8 +179,6 @@ static int sgx_dev_init(struct device *dev)
> int ret;
> int i;
>
> - pr_info("intel_sgx: " DRV_DESCRIPTION " v" DRV_VERSION
"\n");
> -
> if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> return -ENODEV;
>
> @@ -199,7 +190,7 @@ static int sgx_dev_init(struct device *dev)
> pa = ((u64)(ebx & 0xfffff) << 32) + (u64)(eax & 0xfffff000);
> size = ((u64)(edx & 0xfffff) << 32) + (u64)(ecx & 0xfffff000);
>
> - dev_info(dev, "EPC bank 0x%lx-0x%lx\n", pa, pa + size);
> + pr_info("intel_sgx: EPC bank 0x%lx-0x%lx\n", pa, pa + size);
>
> sgx_epc_banks[i].pa = pa;
> sgx_epc_banks[i].size = size;
> @@ -238,10 +229,11 @@ 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_pagecache;
> }
>
> - sgx_dev.parent = dev;
> + register_syscore_ops(&sgx_syscore_ops);
> +
> ret = misc_register(&sgx_dev);
> if (ret) {
> pr_err("intel_sgx: misc_register() failed\n");
> @@ -250,7 +242,10 @@ static int sgx_dev_init(struct device *dev)
>
> return 0;
> out_workqueue:
> + unregister_syscore_ops(&sgx_syscore_ops);
> destroy_workqueue(sgx_add_page_wq);
> +out_pagecache:
> + sgx_page_cache_teardown();
> out_iounmap:
> #ifdef CONFIG_X86_64
> for (i = 0; i < sgx_nr_epc_banks; i++)
> @@ -259,7 +254,7 @@ static int sgx_dev_init(struct device *dev)
> return ret;
> }
>
> -static int sgx_drv_probe(struct platform_device *pdev)
> +static int __init sgx_init(void)
> {
> unsigned int eax, ebx, ecx, edx;
> int i;
> @@ -307,42 +302,7 @@ static int sgx_drv_probe(struct platform_device *pdev)
> #endif
> sgx_encl_size_max_32 = 1ULL << (edx & 0xFF);
>
> - return sgx_dev_init(&pdev->dev);
> + return sgx_dev_init();
> }
>
> -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((void *)sgx_epc_banks[i].va);
> -#endif
> - sgx_page_cache_teardown();
> -
> - return 0;
> -}
> -
> -#ifdef CONFIG_ACPI
> -static struct acpi_device_id sgx_device_ids[] = {
> - {"INT0E0C", 0},
> - {"", 0},
> -};
> -MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
> -#endif
> -
> -static struct platform_driver sgx_drv = {
> - .probe = sgx_drv_probe,
> - .remove = sgx_drv_remove,
> - .driver = {
> - .name = "intel_sgx",
> - .pm = &sgx_drv_pm,
> - .acpi_match_table = ACPI_PTR(sgx_device_ids),
> - },
> -};
> -
> -module_platform_driver(sgx_drv);
> -
> -MODULE_LICENSE("Dual BSD/GPL");
> +device_initcall(sgx_init);
> --
> 2.7.4
>
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev(a)lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
3:28 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > Build SGX support directly into the kernel, utilizing syscore_ops
> > instead of dev_pm_ops to hook into the suspend flow.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson(a)intel.com>
>
> I think you are looking this only from container perspective. For
> desktop distribution it *does* make sense to be able to compile
> this as a module.
>
> /Jarkko
I don't disagree that building SGX as module is valuable. Even for
server/container/VM environments it would be nice to have the driver
built as a loadable module.
The core issue is that taking a dependency on a loadable module is
Simply not possible for a cgroup, and while possible for KVM, is not
ideal. Various workarounds are possible, but they are either ugly,
i.e. will never be accepted upstream, or don't really solve the
problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
basically forces distros to decide between the cgroup and building
SGX as a module.
IMA depends on the TPM driver and requires it to be linked directly to
the kernel if it is enabled i.e. use 'y'. It's a much better solution
and there are already existing examples of this in the mainline kernel.
/Jarkko
6:42 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J
wrote:
> On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > instead of dev_pm_ops to hook into the suspend flow.
> > >
> > > Signed-off-by: Sean Christopherson
<sean.j.christopherson(a)intel.com>
> >
> > I think you are looking this only from container perspective. For
> > desktop distribution it *does* make sense to be able to compile
> > this as a module.
> >
> > /Jarkko
>
> I don't disagree that building SGX as module is valuable. Even for
> server/container/VM environments it would be nice to have the driver
> built as a loadable module.
>
> The core issue is that taking a dependency on a loadable module is
> Simply not possible for a cgroup, and while possible for KVM, is not
> ideal. Various workarounds are possible, but they are either ugly,
> i.e. will never be accepted upstream, or don't really solve the
> problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> basically forces distros to decide between the cgroup and building
> SGX as a module.
IMA depends on the TPM driver and requires it to be linked directly to
the kernel if it is enabled i.e. use 'y'. It's a much better solution
and there are already existing examples of this in the mainline kernel.
It's a simpler solution, but I think taking that route will result in all
distros compiling the SGX driver as a built-in, which defeats the purpose
of allowing the SGX driver to be a loadable module.
12:14 p.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J wrote:
On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > Build SGX support directly into the kernel, utilizing syscore_ops
> > > > instead of dev_pm_ops to hook into the suspend flow.
> > > >
> > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson(a)intel.com>
> > >
> > > I think you are looking this only from container perspective. For
> > > desktop distribution it *does* make sense to be able to compile
> > > this as a module.
> > >
> > > /Jarkko
> >
> > I don't disagree that building SGX as module is valuable. Even for
> > server/container/VM environments it would be nice to have the driver
> > built as a loadable module.
> >
> > The core issue is that taking a dependency on a loadable module is
> > Simply not possible for a cgroup, and while possible for KVM, is not
> > ideal. Various workarounds are possible, but they are either ugly,
> > i.e. will never be accepted upstream, or don't really solve the
> > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > basically forces distros to decide between the cgroup and building
> > SGX as a module.
>
> IMA depends on the TPM driver and requires it to be linked directly to
> the kernel if it is enabled i.e. use 'y'. It's a much better solution
> and there are already existing examples of this in the mainline kernel.
It's a simpler solution, but I think taking that route will result in all
distros compiling the SGX driver as a built-in, which defeats the purpose
of allowing the SGX driver to be a loadable module.
So happens with TPM driver for most of the distros. It still gives
some flexibility and does not cause much harm.
It will also easier for me to maintain SGX tree, collect patches and
send pull requests for upper layer maintainers for future kernel
releases.
Sprinkling properly encapsulated code is really an antipattern.
/Jarkko
12:28 p.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
wrote:
> On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > > Build SGX support directly into the kernel, utilizing
syscore_ops
> > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > >
> > > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson(a)intel.com>
> > > >
> > > > I think you are looking this only from container perspective. For
> > > > desktop distribution it *does* make sense to be able to compile
> > > > this as a module.
> > > >
> > > > /Jarkko
> > >
> > > I don't disagree that building SGX as module is valuable. Even for
> > > server/container/VM environments it would be nice to have the driver
> > > built as a loadable module.
> > >
> > > The core issue is that taking a dependency on a loadable module is
> > > Simply not possible for a cgroup, and while possible for KVM, is not
> > > ideal. Various workarounds are possible, but they are either ugly,
> > > i.e. will never be accepted upstream, or don't really solve the
> > > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > > basically forces distros to decide between the cgroup and building
> > > SGX as a module.
> >
> > IMA depends on the TPM driver and requires it to be linked directly to
> > the kernel if it is enabled i.e. use 'y'. It's a much better
solution
> > and there are already existing examples of this in the mainline kernel.
>
> It's a simpler solution, but I think taking that route will result in all
> distros compiling the SGX driver as a built-in, which defeats the purpose
> of allowing the SGX driver to be a loadable module.
So happens with TPM driver for most of the distros. It still gives
some flexibility and does not cause much harm.
It will also easier for me to maintain SGX tree, collect patches and
send pull requests for upper layer maintainers for future kernel
releases.
Sprinkling properly encapsulated code is really an antipattern.
/Jarkko
There's also things when you move into maintainer mode like backporting
bug fixes for stable kernels and stuff like this. Distro maintainers and
Greg K-H will probably like the current organization more...
/Jarkko
4:18 p.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> wrote:
> > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > J wrote:
> > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > wrote:
> > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > Christopherson wrote:
> > > > > > Build SGX support directly into the kernel, utilizing
> > > > > > syscore_ops
> > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > >
> > > > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson
> > > > > > @intel.com>
> > > > >
> > > > > I think you are looking this only from container
> > > > > perspective. For
> > > > > desktop distribution it *does* make sense to be able to
> > > > > compile
> > > > > this as a module.
> > > > >
> > > > > /Jarkko
> > > >
> > > > I don't disagree that building SGX as module is
> > > > valuable. Even for
> > > > server/container/VM environments it would be nice to have the
> > > > driver
> > > > built as a loadable module.
> > > >
> > > > The core issue is that taking a dependency on a loadable
> > > > module is
> > > > Simply not possible for a cgroup, and while possible for KVM,
> > > > is not
> > > > ideal. Various workarounds are possible, but they are either
> > > > ugly,
> > > > i.e. will never be accepted upstream, or don't really solve
> > > > the
> > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > cgroup
> > > > basically forces distros to decide between the cgroup and
> > > > building
> > > > SGX as a module.
> > >
> > > IMA depends on the TPM driver and requires it to be linked
> > > directly to
> > > the kernel if it is enabled i.e. use 'y'. It's a much better
> > > solution
> > > and there are already existing examples of this in the mainline
> > > kernel.
> >
> > It's a simpler solution, but I think taking that route will
> > result in all
> > distros compiling the SGX driver as a built-in, which defeats the
> > purpose
> > of allowing the SGX driver to be a loadable module.
>
> So happens with TPM driver for most of the distros. It still gives
> some flexibility and does not cause much harm.
>
> It will also easier for me to maintain SGX tree, collect patches
> and
> send pull requests for upper layer maintainers for future kernel
> releases.
>
> Sprinkling properly encapsulated code is really an antipattern.
>
> /Jarkko
There's also things when you move into maintainer mode like
backporting
bug fixes for stable kernels and stuff like this. Distro maintainers
and
Greg K-H will probably like the current organization more...
Hi Jarkko,
From maintain's view, I think there's already plenty examples
that one
maintainer maintains files under several folders. For example, KVM
maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
there any particular difficulty if part of SGX code is under, ex,
arch/x86/kernel/cpu/ ?
I am not sure about TPM but for SGX, but KVM actually only needs to
depend on part of your SGX code. Moving some core SGX functions to
arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
meantime, provides more flexibility (ie, still allow SGX driver to be a
loadable module).
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
8:41 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Wed, Sep 06, 2017 at 11:18:42AM +1200, Kai Huang wrote:
On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> > wrote:
> > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > > J wrote:
> > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > > Christopherson wrote:
> > > > > > > Build SGX support directly into the kernel,
utilizing
> > > > > > > syscore_ops
> > > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > > >
> > > > > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson
> > > > > > > @intel.com>
> > > > > >
> > > > > > I think you are looking this only from container
> > > > > > perspective. For
> > > > > > desktop distribution it *does* make sense to be able to
> > > > > > compile
> > > > > > this as a module.
> > > > > >
> > > > > > /Jarkko
> > > > >
> > > > > I don't disagree that building SGX as module is
> > > > > valuable. Even for
> > > > > server/container/VM environments it would be nice to have the
> > > > > driver
> > > > > built as a loadable module.
> > > > >
> > > > > The core issue is that taking a dependency on a loadable
> > > > > module is simply not possible for a cgroup, and while possible
for KVM,
> > > > > is not
> > > > > ideal. Various workarounds are possible, but they are either
> > > > > ugly,
> > > > > i.e. will never be accepted upstream, or don't really
solve
> > > > > the
> > > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > > cgroup
> > > > > basically forces distros to decide between the cgroup and
> > > > > building
> > > > > SGX as a module.
> > > >
> > > > IMA depends on the TPM driver and requires it to be linked
> > > > directly to
> > > > the kernel if it is enabled i.e. use 'y'. It's a much
better
> > > > solution
> > > > and there are already existing examples of this in the mainline
> > > > kernel.
> > >
> > > It's a simpler solution, but I think taking that route will
> > > result in all
> > > distros compiling the SGX driver as a built-in, which defeats the
> > > purpose
> > > of allowing the SGX driver to be a loadable module.
> >
> > So happens with TPM driver for most of the distros. It still gives
> > some flexibility and does not cause much harm.
> >
> > It will also easier for me to maintain SGX tree, collect patches
> > and
> > send pull requests for upper layer maintainers for future kernel
> > releases.
> >
> > Sprinkling properly encapsulated code is really an antipattern.
> >
> > /Jarkko
>
> There's also things when you move into maintainer mode like
> backporting
> bug fixes for stable kernels and stuff like this. Distro maintainers
> and
> Greg K-H will probably like the current organization more...
Hi Jarkko,
From maintain's view, I think there's already plenty examples that one
maintainer maintains files under several folders. For example, KVM
maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
there any particular difficulty if part of SGX code is under, ex,
arch/x86/kernel/cpu/ ?
I am not sure about TPM but for SGX, but KVM actually only needs to
depend on part of your SGX code. Moving some core SGX functions to
arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
meantime, provides more flexibility (ie, still allow SGX driver to be a
loadable module).
Removing the dependency on the SGX driver will also allow KVM to virtualize
SGX without SGX being exposed to the native userspace. I think we need to
support this usage model it provides the most robust environment for static
EPC partitioning for VMs, i.e. the VMM doesn't need to take additional steps
to ensure native userspace enclaves don't steal EPC pages.
9:23 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Wed, Sep 06, 2017 at 03:41:24PM +0000, Christopherson, Sean J wrote:
On Wed, Sep 06, 2017 at 11:18:42AM +1200, Kai Huang wrote:
> On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> > > wrote:
> > > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > > > J wrote:
> > > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > > > wrote:
> > > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > > > Christopherson wrote:
> > > > > > > > Build SGX support directly into the kernel,
utilizing
> > > > > > > > syscore_ops
> > > > > > > > instead of dev_pm_ops to hook into the suspend
flow.
> > > > > > > >
> > > > > > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson
> > > > > > > > @intel.com>
> > > > > > >
> > > > > > > I think you are looking this only from container
> > > > > > > perspective. For
> > > > > > > desktop distribution it *does* make sense to be able
to
> > > > > > > compile
> > > > > > > this as a module.
> > > > > > >
> > > > > > > /Jarkko
> > > > > >
> > > > > > I don't disagree that building SGX as module is
> > > > > > valuable. Even for
> > > > > > server/container/VM environments it would be nice to have
the
> > > > > > driver
> > > > > > built as a loadable module.
> > > > > >
> > > > > > The core issue is that taking a dependency on a loadable
> > > > > > module is simply not possible for a cgroup, and while
possible for KVM,
> > > > > > is not
> > > > > > ideal. Various workarounds are possible, but they are
either
> > > > > > ugly,
> > > > > > i.e. will never be accepted upstream, or don't really
solve
> > > > > > the
> > > > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > > > cgroup
> > > > > > basically forces distros to decide between the cgroup and
> > > > > > building
> > > > > > SGX as a module.
> > > > >
> > > > > IMA depends on the TPM driver and requires it to be linked
> > > > > directly to
> > > > > the kernel if it is enabled i.e. use 'y'. It's a
much better
> > > > > solution
> > > > > and there are already existing examples of this in the mainline
> > > > > kernel.
> > > >
> > > > It's a simpler solution, but I think taking that route will
> > > > result in all
> > > > distros compiling the SGX driver as a built-in, which defeats the
> > > > purpose
> > > > of allowing the SGX driver to be a loadable module.
> > >
> > > So happens with TPM driver for most of the distros. It still gives
> > > some flexibility and does not cause much harm.
> > >
> > > It will also easier for me to maintain SGX tree, collect patches
> > > and
> > > send pull requests for upper layer maintainers for future kernel
> > > releases.
> > >
> > > Sprinkling properly encapsulated code is really an antipattern.
> > >
> > > /Jarkko
> >
> > There's also things when you move into maintainer mode like
> > backporting
> > bug fixes for stable kernels and stuff like this. Distro maintainers
> > and
> > Greg K-H will probably like the current organization more...
>
> Hi Jarkko,
>
> From maintain's view, I think there's already plenty examples that one
> maintainer maintains files under several folders. For example, KVM
> maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
> there any particular difficulty if part of SGX code is under, ex,
> arch/x86/kernel/cpu/ ?
>
> I am not sure about TPM but for SGX, but KVM actually only needs to
> depend on part of your SGX code. Moving some core SGX functions to
> arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
> meantime, provides more flexibility (ie, still allow SGX driver to be a
> loadable module).
Removing the dependency on the SGX driver will also allow KVM to virtualize
SGX without SGX being exposed to the native userspace. I think we need to
support this usage model it provides the most robust environment for static
EPC partitioning for VMs, i.e. the VMM doesn't need to take additional steps
to ensure native userspace enclaves don't steal EPC pages.
Not sure if it makes sense to complicate things for things that do not
exist yet... I'll ignore this discussion up until there's an upstream
driver.
/Jarkko
9:15 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Wed, Sep 06, 2017 at 11:18:42AM +1200, Kai Huang wrote:
On Tue, 2017-09-05 at 22:28 +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
> > wrote:
> > > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean
> > > > J wrote:
> > > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean
> > > > > > Christopherson wrote:
> > > > > > > Build SGX support directly into the kernel, utilizing
> > > > > > > syscore_ops
> > > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > > >
> > > > > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson
> > > > > > > @intel.com>
> > > > > >
> > > > > > I think you are looking this only from container
> > > > > > perspective. For
> > > > > > desktop distribution it *does* make sense to be able to
> > > > > > compile
> > > > > > this as a module.
> > > > > >
> > > > > > /Jarkko
> > > > >
> > > > > I don't disagree that building SGX as module is
> > > > > valuable. Even for
> > > > > server/container/VM environments it would be nice to have the
> > > > > driver
> > > > > built as a loadable module.
> > > > >
> > > > > The core issue is that taking a dependency on a loadable
> > > > > module is
> > > > > Simply not possible for a cgroup, and while possible for KVM,
> > > > > is not
> > > > > ideal. Various workarounds are possible, but they are either
> > > > > ugly,
> > > > > i.e. will never be accepted upstream, or don't really solve
> > > > > the
> > > > > problem, e.g. forcing SGX to be built-in to enable the EPC
> > > > > cgroup
> > > > > basically forces distros to decide between the cgroup and
> > > > > building
> > > > > SGX as a module.
> > > >
> > > > IMA depends on the TPM driver and requires it to be linked
> > > > directly to
> > > > the kernel if it is enabled i.e. use 'y'. It's a much
better
> > > > solution
> > > > and there are already existing examples of this in the mainline
> > > > kernel.
> > >
> > > It's a simpler solution, but I think taking that route will
> > > result in all
> > > distros compiling the SGX driver as a built-in, which defeats the
> > > purpose
> > > of allowing the SGX driver to be a loadable module.
> >
> > So happens with TPM driver for most of the distros. It still gives
> > some flexibility and does not cause much harm.
> >
> > It will also easier for me to maintain SGX tree, collect patches
> > and
> > send pull requests for upper layer maintainers for future kernel
> > releases.
> >
> > Sprinkling properly encapsulated code is really an antipattern.
> >
> > /Jarkko
>
> There's also things when you move into maintainer mode like
> backporting
> bug fixes for stable kernels and stuff like this. Distro maintainers
> and
> Greg K-H will probably like the current organization more...
Hi Jarkko,
From maintain's view, I think there's already plenty examples that one
maintainer maintains files under several folders. For example, KVM
maintainer maintains files both under virt/kvm/ and arch/*/kvm/. Is
there any particular difficulty if part of SGX code is under, ex,
arch/x86/kernel/cpu/ ?
There must have been a good reason to do this for kvm. There's not
reason to complicate thing for SGX.
I am not sure about TPM but for SGX, but KVM actually only needs to
depend on part of your SGX code. Moving some core SGX functions to
arch/x86/kernel/cpu/ simplifies KVM and cgroup's implementation and at
meantime, provides more flexibility (ie, still allow SGX driver to be a
loadable module).
Thanks,
-Kai
Still don't understand why you can't just require to link SGX to vmlinux
when needed.
/Jarkko
8:32 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J
wrote:
> On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson wrote:
> > > > > Build SGX support directly into the kernel, utilizing
syscore_ops
> > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > >
> > > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson(a)intel.com>
> > > >
> > > > I think you are looking this only from container perspective. For
> > > > desktop distribution it *does* make sense to be able to compile
> > > > this as a module.
> > > >
> > > > /Jarkko
> > >
> > > I don't disagree that building SGX as module is valuable. Even for
> > > server/container/VM environments it would be nice to have the driver
> > > built as a loadable module.
> > >
> > > The core issue is that taking a dependency on a loadable module is
> > > Simply not possible for a cgroup, and while possible for KVM, is not
> > > ideal. Various workarounds are possible, but they are either ugly,
> > > i.e. will never be accepted upstream, or don't really solve the
> > > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > > basically forces distros to decide between the cgroup and building
> > > SGX as a module.
> >
> > IMA depends on the TPM driver and requires it to be linked directly to
> > the kernel if it is enabled i.e. use 'y'. It's a much better
solution
> > and there are already existing examples of this in the mainline kernel.
>
> It's a simpler solution, but I think taking that route will result in all
> distros compiling the SGX driver as a built-in, which defeats the purpose
> of allowing the SGX driver to be a loadable module.
So happens with TPM driver for most of the distros. It still gives
some flexibility and does not cause much harm.
It will also easier for me to maintain SGX tree, collect patches and
send pull requests for upper layer maintainers for future kernel
releases.
Sprinkling properly encapsulated code is really an antipattern.
Once more SGX features are added I don't think it will be possible to fully
encapsulate the SGX driver as it stands today, e.g. VMM EPC oversubscription
needs different flows for swapping pages to/from the EPC. Splitting out the
high-level EPC control code to a separate built-in module allows the native
driver and KVM to coexist without having to make invasive changes or break
encapsulation of the native driver. It also gives the EPC cgroup a single
touchpoint without the native SGX driver having to be aware of KVM.
9:17 a.m.
New subject: [PATCH 2/2] intel_sgx: remove option to build as a module
On Wed, Sep 06, 2017 at 03:32:27PM +0000, Christopherson, Sean J wrote:
On Tue, Sep 05, 2017 at 10:14:11PM +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 05, 2017 at 01:42:30PM +0000, Christopherson, Sean J wrote:
> > On Sat, Sep 02, 2017 at 01:28:54PM +0300, Jarkko Sakkinen wrote:
> > > On Thu, Aug 31, 2017 at 04:52:17PM +0000, Christopherson, Sean J wrote:
> > > > On Wed, Aug 30, 2017 at 09:47:05PM +0300, Jarkko Sakkinen wrote:
> > > > > On Tue, Aug 29, 2017 at 10:56:36AM -0700, Sean Christopherson
wrote:
> > > > > > Build SGX support directly into the kernel, utilizing
syscore_ops
> > > > > > instead of dev_pm_ops to hook into the suspend flow.
> > > > > >
> > > > > > Signed-off-by: Sean Christopherson
<sean.j.christopherson(a)intel.com>
> > > > >
> > > > > I think you are looking this only from container perspective.
For
> > > > > desktop distribution it *does* make sense to be able to compile
> > > > > this as a module.
> > > > >
> > > > > /Jarkko
> > > >
> > > > I don't disagree that building SGX as module is valuable. Even
for
> > > > server/container/VM environments it would be nice to have the driver
> > > > built as a loadable module.
> > > >
> > > > The core issue is that taking a dependency on a loadable module is
> > > > Simply not possible for a cgroup, and while possible for KVM, is not
> > > > ideal. Various workarounds are possible, but they are either ugly,
> > > > i.e. will never be accepted upstream, or don't really solve the
> > > > problem, e.g. forcing SGX to be built-in to enable the EPC cgroup
> > > > basically forces distros to decide between the cgroup and building
> > > > SGX as a module.
> > >
> > > IMA depends on the TPM driver and requires it to be linked directly to
> > > the kernel if it is enabled i.e. use 'y'. It's a much better
solution
> > > and there are already existing examples of this in the mainline kernel.
> >
> > It's a simpler solution, but I think taking that route will result in all
> > distros compiling the SGX driver as a built-in, which defeats the purpose
> > of allowing the SGX driver to be a loadable module.
>
> So happens with TPM driver for most of the distros. It still gives
> some flexibility and does not cause much harm.
>
> It will also easier for me to maintain SGX tree, collect patches and
> send pull requests for upper layer maintainers for future kernel
> releases.
>
> Sprinkling properly encapsulated code is really an antipattern.
Once more SGX features are added I don't think it will be possible to fully
encapsulate the SGX driver as it stands today, e.g. VMM EPC oversubscription
needs different flows for swapping pages to/from the EPC. Splitting out the
high-level EPC control code to a separate built-in module allows the native
driver and KVM to coexist without having to make invasive changes or break
encapsulation of the native driver. It also gives the EPC cgroup a single
touchpoint without the native SGX driver having to be aware of KVM.
Right now there is no need to complicate the architecture. You can
require SGX to be linked to vmlinux.
/Jarkko
9:34 p.m.
Hi Sean,
Overall I think it's good move, but I am not sure whether we should
move entire driver to arch/x86/kernel/cpu. :)
My original thinking was we should move SGX detection, EPC management
(may be enclave management as well) code to kernel and leave staff
serving host SGX app (ex, IOCTL) in driver (which means basically to
split SGX driver into two parts). But this requires significant code
change to SGX driver.
Thanks,
-Kai
On Tue, 2017-08-29 at 10:56 -0700, Sean Christopherson wrote:
Move the SGX code into /arch/x86/kernel/cpu and remove the option
to build the SGX subsystem as a loadable module. Allowing SGX to
be built as a module complicates the implemenation (code and
upstreaming process) of future SGX extensions, e.g. KVM support
and EPC cgroup.
Sean Christopherson (2):
intel_sgx: move files to arch/x86/kernel/cpu
intel_sgx: remove option to build as a module
arch/x86/Kconfig
| 20 +++++++++++++++
arch/x86/kernel/cpu/Makefile
| 1 +
arch/x86/kernel/cpu/intel_sgx/Makefile
| 9 +++++++
{drivers/platform/x86 =>
arch/x86/kernel/cpu}/intel_sgx/sgx.h | 0
{drivers/platform/x86 =>
arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c | 0
{drivers/platform/x86 =>
arch/x86/kernel/cpu}/intel_sgx/sgx_main.c | 74 +++++++++++++---
--------------------------------------
{drivers/platform/x86 =>
arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c | 0
{drivers/platform/x86 =>
arch/x86/kernel/cpu}/intel_sgx/sgx_util.c | 0
{drivers/platform/x86 =>
arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c | 0
drivers/platform/x86/Kconfig
| 2 --
drivers/platform/x86/Makefile
| 1 -
drivers/platform/x86/intel_sgx/Kconfig
| 19 --------------
drivers/platform/x86/intel_sgx/Makefile
| 12 ---------
13 files changed, 47 insertions(+), 91 deletions(-)
_______________________________________________
intel-sgx-kernel-dev mailing list
intel-sgx-kernel-dev(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
11:44 a.m.
On Wed, Aug 30, 2017 at 04:34:03PM +1200, Kai Huang wrote:
Hi Sean,
Overall I think it's good move, but I am not sure whether we should
move entire driver to arch/x86/kernel/cpu. :)
My original thinking was we should move SGX detection, EPC management
(may be enclave management as well) code to kernel and leave staff
serving host SGX app (ex, IOCTL) in driver (which means basically to
split SGX driver into two parts). But this requires significant code
change to SGX driver.
Thanks,
-Kai
Hmm... they are already in the kernel. They just can be compile also as
a module. I'm not getting your point, I'm sorry.
/Jarkko
3:04 a.m.
On Tue, Aug 29, 2017 at 10:56:34AM -0700, Sean Christopherson wrote:
Move the SGX code into /arch/x86/kernel/cpu and remove the option
to build the SGX subsystem as a loadable module. Allowing SGX to
be built as a module complicates the implemenation (code and
upstreaming process) of future SGX extensions, e.g. KVM support
and EPC cgroup.
Sean Christopherson (2):
intel_sgx: move files to arch/x86/kernel/cpu
intel_sgx: remove option to build as a module
arch/x86/Kconfig | 20
+++++++++++++++
arch/x86/kernel/cpu/Makefile | 1 +
arch/x86/kernel/cpu/intel_sgx/Makefile | 9 +++++++
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c | 74
+++++++++++++-----------------------------------------
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c | 0
drivers/platform/x86/Kconfig | 2 --
drivers/platform/x86/Makefile | 1 -
drivers/platform/x86/intel_sgx/Kconfig | 19
--------------
drivers/platform/x86/intel_sgx/Makefile | 12 ---------
13 files changed, 47 insertions(+), 91 deletions(-)
_______________________________________________
intel-sgx-kernel-dev mailing list
intel-sgx-kernel-dev(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Not going to take this at this point. Keeping it where it is for
my patch set.
/Jarkko
6:20 a.m.
On Wed, Aug 30, 2017 at 01:04:25PM +0300, Jarkko Sakkinen wrote:
On Tue, Aug 29, 2017 at 10:56:34AM -0700, Sean Christopherson wrote:
> Move the SGX code into /arch/x86/kernel/cpu and remove the option
> to build the SGX subsystem as a loadable module. Allowing SGX to
> be built as a module complicates the implemenation (code and
> upstreaming process) of future SGX extensions, e.g. KVM support
> and EPC cgroup.
>
> Sean Christopherson (2):
> intel_sgx: move files to arch/x86/kernel/cpu
> intel_sgx: remove option to build as a module
>
> arch/x86/Kconfig | 20
+++++++++++++++
> arch/x86/kernel/cpu/Makefile | 1 +
> arch/x86/kernel/cpu/intel_sgx/Makefile | 9
+++++++
> {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h | 0
> {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c | 0
> {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c | 74
+++++++++++++-----------------------------------------
> {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c | 0
> {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c | 0
> {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c | 0
> drivers/platform/x86/Kconfig | 2 --
> drivers/platform/x86/Makefile | 1 -
> drivers/platform/x86/intel_sgx/Kconfig | 19
--------------
> drivers/platform/x86/intel_sgx/Makefile | 12
---------
> 13 files changed, 47 insertions(+), 91 deletions(-)
>
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev(a)lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
Not going to take this at this point. Keeping it where it is for
my patch set.
Can you elaborate on why you don't to bring SGX into the kernel? This may be
a blocking issue for me.
11:53 a.m.
On Wed, Aug 30, 2017 at 01:20:10PM +0000, Christopherson, Sean J wrote:
On Wed, Aug 30, 2017 at 01:04:25PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 29, 2017 at 10:56:34AM -0700, Sean Christopherson wrote:
> > Move the SGX code into /arch/x86/kernel/cpu and remove the option
> > to build the SGX subsystem as a loadable module. Allowing SGX to
> > be built as a module complicates the implemenation (code and
> > upstreaming process) of future SGX extensions, e.g. KVM support
> > and EPC cgroup.
> >
> > Sean Christopherson (2):
> > intel_sgx: move files to arch/x86/kernel/cpu
> > intel_sgx: remove option to build as a module
> >
> > arch/x86/Kconfig | 20
+++++++++++++++
> > arch/x86/kernel/cpu/Makefile | 1
+
> > arch/x86/kernel/cpu/intel_sgx/Makefile | 9
+++++++
> > {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h |
0
> > {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c |
0
> > {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c |
74 +++++++++++++-----------------------------------------
> > {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c |
0
> > {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c |
0
> > {drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c |
0
> > drivers/platform/x86/Kconfig | 2
--
> > drivers/platform/x86/Makefile | 1
-
> > drivers/platform/x86/intel_sgx/Kconfig | 19
--------------
> > drivers/platform/x86/intel_sgx/Makefile | 12
---------
> > 13 files changed, 47 insertions(+), 91 deletions(-)
> >
> > _______________________________________________
> > intel-sgx-kernel-dev mailing list
> > intel-sgx-kernel-dev(a)lists.01.org
> > https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
>
> Not going to take this at this point. Keeping it where it is for
> my patch set.
Can you elaborate on why you don't to bring SGX into the kernel? This may be
a blocking issue for me.
I just wrote better explanations.
You should rather explain the exact bottleneck than try to push turn
everything upside down patches. Then we need to find a solution where
the code can be still compile as a module.
/Jarkko
11:48 a.m.
On Tue, Aug 29, 2017 at 10:56:34AM -0700, Sean Christopherson wrote:
Move the SGX code into /arch/x86/kernel/cpu and remove the option
to build the SGX subsystem as a loadable module. Allowing SGX to
be built as a module complicates the implemenation (code and
upstreaming process) of future SGX extensions, e.g. KVM support
and EPC cgroup.
I'm sure we find solutions case by case as they come. You could argue
that for any driver.
/Jarkko
Sean Christopherson (2):
intel_sgx: move files to arch/x86/kernel/cpu
intel_sgx: remove option to build as a module
arch/x86/Kconfig | 20
+++++++++++++++
arch/x86/kernel/cpu/Makefile | 1 +
arch/x86/kernel/cpu/intel_sgx/Makefile | 9 +++++++
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx.h | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_ioctl.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_main.c | 74
+++++++++++++-----------------------------------------
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_page_cache.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_util.c | 0
{drivers/platform/x86 => arch/x86/kernel/cpu}/intel_sgx/sgx_vma.c | 0
drivers/platform/x86/Kconfig | 2 --
drivers/platform/x86/Makefile | 1 -
drivers/platform/x86/intel_sgx/Kconfig | 19
--------------
drivers/platform/x86/intel_sgx/Makefile | 12 ---------
13 files changed, 47 insertions(+), 91 deletions(-)
_______________________________________________
intel-sgx-kernel-dev mailing list
intel-sgx-kernel-dev(a)lists.01.org
https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
1757
days inactive
1766
days old
intel-sgx-kernel-dev@lists.01.org
23 comments
4 participants
participants (4)
-
Christopherson, Sean J -
Jarkko Sakkinen -
Kai Huang -
Sean Christopherson